From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-00069f02.pphosted.com (mx0a-00069f02.pphosted.com [205.220.165.32]) by sourceware.org (Postfix) with ESMTPS id B727B3858D28 for ; Thu, 2 Nov 2023 08:15:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B727B3858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=oracle.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=oracle.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org B727B3858D28 Authentication-Results: server2.sourceware.org; arc=pass smtp.remote-ip=205.220.165.32 ARC-Seal: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1698912948; cv=pass; b=Nemx4Sum/Dfjr6RedFRMlxl7Tvun+L9BTy65nmytytRo9cNRXeMYWK+Pha4y0mpAZQnNPpmFMYh7eGJHctskzVmuJgR80L3z3ielArJmNCaNkiZVlNorhstdYE1NUxXEHtCVFAQlm006DzcVlm5u6xvJoxOtS/5/j+Zr/078MJg= ARC-Message-Signature: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1698912948; c=relaxed/simple; bh=jZMZSAq61vz04WWTfk8c7fkNBZIpQ1Cekhg7HfjsYDQ=; h=DKIM-Signature:DKIM-Signature:Message-ID:Date:From:Subject:To: MIME-Version; b=S3kTT3APF3YV5eUW2isuAIY3ORvgdmAW0/YnbmjpyZ9ZDy/Zv9GhX/iIjLFfcNaNGTAOBrWlieLeyCDrKRLm21EF3JZ2YA58W7qJX2CmarJHP2FkLGw8H1PTXuiqWVusyCfHyXIsuTQ3i0SiUsAFWo4Srm2URLXI8HJC4OMwyxM= ARC-Authentication-Results: i=2; server2.sourceware.org Received: from pps.filterd (m0246627.ppops.net [127.0.0.1]) by mx0b-00069f02.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 3A283NHr024470; Thu, 2 Nov 2023 08:15:35 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=message-id : date : from : subject : to : cc : references : in-reply-to : content-type : content-transfer-encoding : mime-version; s=corp-2023-03-30; bh=AkwtBmSUUhLNj8xZ/ZYxbwwhccic6aGPA9Orl33Ea7A=; b=DCBSL3n1ja6d3UyWzJXU/uPr+4mCVgo8+j9Mrv4ucg0CFvs+NlBVCB0orA2ie+qQXnxn VpOiyhGRPH6qqKjh7rfxsAa3a9H0ffj4HEJ1Mye8O3H5i9qoQ/7ExbUhi3oovuwb5GVa Fj7l6RKnIehHoi6v9tjnUhUc1rwA+sppJekytp+jz1TBpC9rcjgI2CzETKzIwAz3QRQq IikHk9pYRj4+MIikAKoL43SBXxIgET9381wRY/dNP6uQe2BQyVyEBOVIRkQI03JtnhQL 4XJN+ADgBkxUkW15bEE56CkTasn2LtxbsbYTOCDBSL55uenG27gcmdviPXh4F+w40Xcx Ug== Received: from iadpaimrmta01.imrmtpd1.prodappiadaev1.oraclevcn.com (iadpaimrmta01.appoci.oracle.com [130.35.100.223]) by mx0b-00069f02.pphosted.com (PPS) with ESMTPS id 3u0rw2931c-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 02 Nov 2023 08:15:34 +0000 Received: from pps.filterd (iadpaimrmta01.imrmtpd1.prodappiadaev1.oraclevcn.com [127.0.0.1]) by iadpaimrmta01.imrmtpd1.prodappiadaev1.oraclevcn.com (8.17.1.19/8.17.1.19) with ESMTP id 3A27gLT0038064; Thu, 2 Nov 2023 08:15:33 GMT Received: from nam12-bn8-obe.outbound.protection.outlook.com (mail-bn8nam12lp2168.outbound.protection.outlook.com [104.47.55.168]) by iadpaimrmta01.imrmtpd1.prodappiadaev1.oraclevcn.com (PPS) with ESMTPS id 3u0rreekd8-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 02 Nov 2023 08:15:33 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=T50zHIGx8kwSa+v391q7AuFW+i6E2TcI9Lkl/EqQMpX9U4n9cgwnoiaONOnNlTaOUd9o0zqWtGm+dXkTmoJaaH5nWY7CJup2jPkTwm3Mw0UHkDO/tQHQ6BIfzkDr2zyx9HVQTKRmo3nANyz2s5/BDQ4nMl7rKfcBmCdfpnlJR8sluX1jEtNUHDXgkggnINK6WsygM/Ka4hSLE5HXG9Xk955RtrHL9D29XpZe9K976VnlXCVTq/kKNELZ3oeWGIsdDujK65EQpXhJvNrbR6+U/l09ZUnbhjn6n+1JjmbAsQdEjog5I3jVepiXYnvQINVD8bXWbaX86nG6ueNKKxUuyQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=AkwtBmSUUhLNj8xZ/ZYxbwwhccic6aGPA9Orl33Ea7A=; b=CjIkUuVYbPfWPvnxLknXvn9mDa41k5JOGaeFsNe4SZTVvpkBGY5u+1nJyJdoo7eUBJrXjPGafJo7FqAQcokIOgJVvkRpFkoegTnAYKpPW3SDVem21FrjpYPZ5TQwCvMcIcb7IU+FZ/OfzVWLWeATktWx/jZayRu3ieT8Y3svIjsrRjuasCXm9wPjfDFwdV8kuGxjJZMbQw/AhCOgLFwtoIqlIfTGo3y/gRtZn7kjMRzTaIbBNCFfKHrzl1jtFejtRVUty6wNxNb4li9gfT49ButLa3D13wzXOB0ReyuO/O7kkvjizdV7HGNjB8tp6psN8V6rP+rdEyzvehsOZUGebQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=oracle.com; dmarc=pass action=none header.from=oracle.com; dkim=pass header.d=oracle.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.onmicrosoft.com; s=selector2-oracle-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=AkwtBmSUUhLNj8xZ/ZYxbwwhccic6aGPA9Orl33Ea7A=; b=xScdmJNMLtOqUoJNUtF/T50l+ZXulQdxbps4cnkh0ze/zpDDFjd1ucsHZ93QJiS6Lw3gOIr122jwqJ7b6H8/gNUZvjLb5mKSWCpcpYsbY+5kufSAkOM4E39m3Dt5mpGknF0aViK1/fbVZbmvFmauMVABbWF6wtm8x0cE8p05f58= Received: from BN6PR1001MB2147.namprd10.prod.outlook.com (2603:10b6:405:2e::26) by DS7PR10MB4942.namprd10.prod.outlook.com (2603:10b6:5:3ab::24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6954.21; Thu, 2 Nov 2023 08:15:30 +0000 Received: from BN6PR1001MB2147.namprd10.prod.outlook.com ([fe80::7c13:fa73:7ea8:8126]) by BN6PR1001MB2147.namprd10.prod.outlook.com ([fe80::7c13:fa73:7ea8:8126%4]) with mapi id 15.20.6954.021; Thu, 2 Nov 2023 08:15:30 +0000 Message-ID: <7a101e5c-fe1d-4b3f-afdb-0ee11cf6a7c9@oracle.com> Date: Thu, 2 Nov 2023 01:15:26 -0700 User-Agent: Mozilla Thunderbird From: Indu Bhagat Subject: Re: [PATCH, V2 07/10] gas: synthesize CFI for hand-written asm To: Jan Beulich Cc: binutils@sourceware.org References: <20231030165137.2570939-1-indu.bhagat@oracle.com> <20231030165137.2570939-8-indu.bhagat@oracle.com> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: MWH0EPF00056D11.namprd21.prod.outlook.com (2603:10b6:30f:fff2:0:1:0:14) To BN6PR1001MB2147.namprd10.prod.outlook.com (2603:10b6:405:2e::26) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: BN6PR1001MB2147:EE_|DS7PR10MB4942:EE_ X-MS-Office365-Filtering-Correlation-Id: 8ff7648d-1d74-42e8-afce-08dbdb7be0f6 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: tn3OUdn3G/ZPsC3Zq2fgZ4AZgFdhOQGTNWMVB72j8GfQQTAsFFDv7tTMiKZ+yPdf/VFin2l+Rmri4j1eencH8MVBFT+zz+NT0HNyOdmWYfH0t3IhmUs/YWuUfpo9olwGOLv9v00k7gWaktkGdVjudiOKpH1+rCZzLPH2jyge8PYkQaBJMmrlVOlZlKoav0SFqT8hiM1icZ+hplwa93414wY2st6NRpsSMC2Pq/yDm6P5ucHly7E1vgB/qjmc2tdZ+3Gagm+fZczaLuoUfur8M/OIL7VrDH4hD146fXHu/CRCMROBz3zShf30gxsDTJdfV3kipa2CN49i3tORLi3cwivxx8yfHotPHq+PdjnNF+Phu8eYAjBbperl5w83ABre8B7zcWk6iGN/vRAgiAtnIYjLMXBuw4/c8xaVSXtDjZr+fKKlc6+IUxqxfQYyZh5brv4yQqXahR7bMMrt6x5EIlTfktjxn5jbBXdVY190sizT3dAf3aTJAqJDRgJwvgfLAUYRN2pUKBgXn4vZwoDTDztl/InUNHAdPSKfmeJYyxnU2Wn+N+xNCweELFhpbEZOd0VRS/0Ee+1XVO1iP523vphGXRhkhyk9M3C/PLlL1Hs0vc0nL8rGh6i+4+MW/1GNbTQs7ydfuHoa9sxb28IcBJPrY+yhNBMlEcm47GqsmfbR4/sqiPNa7ljJdy7/ygGt X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BN6PR1001MB2147.namprd10.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230031)(346002)(39860400002)(136003)(376002)(396003)(366004)(230273577357003)(230922051799003)(230173577357003)(64100799003)(1800799009)(186009)(451199024)(31686004)(66899024)(6512007)(2616005)(38100700002)(31696002)(86362001)(36756003)(2906002)(30864003)(83380400001)(478600001)(6666004)(6506007)(53546011)(4326008)(8676002)(6486002)(66476007)(66556008)(6916009)(316002)(8936002)(41300700001)(44832011)(66946007)(5660300002)(43740500002)(45980500001)(579004);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZVNaT2dVc1JueXh0UTdrTHRya2VHNWdsVGFxSkRiSGdtYzZNbElkSDdiRWV1?= =?utf-8?B?TlNKb21KdFNNODZOYnZ5MkdpdU1Id3BYblA1eGN3RWNhcy8yWWhrMTROZ3Y2?= =?utf-8?B?Z2UwTzZnTjVHQzBKdWZUZW9VMHAxSno5bkxlMW1mR3I3aXJlejQ0UUlsSGRE?= =?utf-8?B?d3ZsUlZhRUMvQ0l3bm90OWR2dE8wZlFXYUpzcW02RzBpVGFjakhVY0VqOG1F?= =?utf-8?B?WXBxajBjZDVua3pGanl3MmtFR0FrYjBkWWZQaVpOa29xRW04YWM4ZUJNTlQ1?= =?utf-8?B?RkNjdkZpZElUdnhlNWM4elExcEZYeU83SHk0SUExc3UrMC9CTXZuckU3aDBL?= =?utf-8?B?M08vL2RZbDJBT0FhZGZuTnc2NGRuNEhCZEtTdnJqeVpMNEE4SlFQc3J0NWd0?= =?utf-8?B?Y3NGVXpNaE9EMlRaY05WdjBYRTZudHkxcVl1cStCNi84VmpFeWZPVjlSbDFl?= =?utf-8?B?TThseWxEUVVrcFo4N0ZqTVk0MEhFU2Z1U09SVFpwTU1NS2szZW9hQnlkdG4y?= =?utf-8?B?OGRvb0ZMSjM2d09EK1VYRGx0VGpUWVJiclRrbDh0bU52MnFIakt4cHhoTDZy?= =?utf-8?B?WnNSUSthYlRaK2xBODdkVm1ZTys3QWRSc0VMdlpSY0E1SGROOER5WWRTdW1V?= =?utf-8?B?dEhvT21EUUFIT09NUStlYjFuVWdKVG9SOGVseFFMeHIvcEV5WjJnQ1puQmRD?= =?utf-8?B?Rjd2bW9kcG1tQVQ2SC9UWHJwZ1NYZEpDUFloOUVzVzB2aTNBZ2pxb096ODdC?= =?utf-8?B?aGVPeXZscTAvMlROcm9KeGFEZlJJTDJ1REk4Q2dSK2Jyd2pSQWc3QkdKbnIx?= =?utf-8?B?MzBCNHl6ZTc2Ykg0d1RyVGtNdUFRdWk4OXNKaXM4RFpTWEMrUnhiT1ZLbkZm?= =?utf-8?B?cnVBaUtRa3dINmdrMVA4WUZUTFI1SGxkMzNFT2xjWERwYmY3UWRWTjhvU0lR?= =?utf-8?B?Szk5UkEzeE5ZVEozM1RrVWFwMFJ0KzhMbk41UklLMHEzRnA3alJUcWZsTDZW?= =?utf-8?B?S01ydDh0TXhsYk54K0IrRzVjZ1IrcUVmNmtOdCsza0pxNmdyeWRUTHFOMmZR?= =?utf-8?B?SVAxL2lnQVFxSExYOVlZR3pqMGFLVGRiYi9KNkF1N216VlBYSklnNzhUREJ6?= =?utf-8?B?d2ZJbHh6a2R4MjRQTU5mNzF3QU1WaitBTURaakEzWGpTWWpnV3Z0d2crSEU1?= =?utf-8?B?cFBhdndLQVI4akcyVUZ2Z0V6RVVOVHhYcXpUc1ZUWWlrYWdmMms1TklzNzdn?= =?utf-8?B?RlpUKy9tcFY4aUVRaHF1cjlVOVZHb0M0SUUreHp3NjFIQlJMazROWVRmbnVQ?= =?utf-8?B?Sk1kU2tOU1g1cjdSQ2I4bjNDY25YMkU0S2pUalhaUi90OGxMenNuaGd0U3E2?= =?utf-8?B?NFg1UWNubXhqc2JOZDRsdFRURWxIMGxGcEZtcXZjeWV3eDFwWlIvYmtTZWwv?= =?utf-8?B?TzhObURUd0RnWHgxbVorSTJyRHFiRjVhTDBYeGlwYzFGQzladGJlaTlYS1p0?= =?utf-8?B?Vk82YTZzZ3czRmR5NjN5Rk9qYWlJN1ZEcmcvQ0JydTc3RkltZ3kwMkMxeWJy?= =?utf-8?B?U1lWckVlK3ZqanUxVXdpZlFNN1lmOHVuRE0rSmNmSFloUERpZjFyS1dSMDBx?= =?utf-8?B?My8rQ2ptd1Qwd0RlNkI0Y1kvODA5MFlsaHc1SWtxRmNYeXc4RDE4czZNUVR5?= =?utf-8?B?ZDIwZk10UTVxRVF5TEhYd2xDSmhnOE9WdFZ1VHRaMUFubzV2SGx6NVNKWnFu?= =?utf-8?B?UTJ4UjUyMHRBclFJSDl6N01kdnUwSEplZlY4S3BoeTZBWGJBWTlaLzZvLzQy?= =?utf-8?B?NUR0Zklrblc3bTFFaHpEOE95QjBIYnczcU9OZXBvYk1LOUR4TUNENjViU2U3?= =?utf-8?B?SG5lazZpRTY2UGkrTEs4MzZPamcwM3hSb2FBWnZFVE5odXdYMUhySEdHdG8y?= =?utf-8?B?WWdJd1hoaEF2SnNIRU5aTlkyWWdQVVNGdnErYzcyRnNKZjl0a2FweDYvVjJT?= =?utf-8?B?YmpFaklIOXNQb1pDbmhnRlpMM1FjTTM4T3UvQnB1Zy9oanB2VkFWM3pwWVJk?= =?utf-8?B?RDE4alR1dnNjdWV1LzNEVGx2dFZYYm1Idm4rQm5GTDJFK1J2dGMvVE1zRmtm?= =?utf-8?B?blYrb1FFTDM2RmcyY2pvQUhwNVJKbTM5WmIrSmdWazhVanA2Z21Lam85cFU1?= =?utf-8?Q?LSosBFc/+SpXhvE9rxHhjIw=3D?= X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: h/9NB9Mqje48v4pNbOQwQQlD6sJwpBg1kGkFisb9XJ+O3ZpvRVbmTSh3q4IM8RkM9TSn5PB0xE2GyGlgPb69hc+pyCuqbDgC2ppOaM+MuDAetwGAAHRPt3JsN55SBaUeyJSvmxmpnYteB4zTLssKpopUTfyRMXeD7giGhznG5u6uIrujm20p5iyX1EOyGaDSqarEr5MxebJTvw5ROYNwUAfS996v8KGQlIm0N5lHA6HJqfNKM6J7cBTuxwTAisN17LDF/6mp2uQUl6crdxYFjbHRHqrnQYBqQFAA9dmh4Bjqw2Y/yvWBLHVYxXFAujKyuIKLCJDIkKzPCwmPN8hskI4zeQnmwJhFZhnUiuIZM3r8zSfZ1zB6gdKaouz6+54B0wYsjAcU6lgIzht0xMn1/KX75XSIMzxUWxfPhRiE8NE14KwXYsEh3rLzCmkG3lDl0g7cH+ZJZ+mzVBBn/Mi7BlZMjpCsOjRA4dsoIFuRxKP8HwWGZQ6eTeBfFyCAQasJkZsYTWrcq/4/ENezwjah8thU3d0Okvbdt4ZBCJ75mPSJ+PZAGc1BxUgw3hp1pYhdxIDoY9ComOpRBfF5FgsnaXYukW8UxYTVNyMl4TJP6Kj/72MIUMBlhzSe9I8d1+h8mf7L//yY5nreUmf0K2Kvi/NSTTXR7SB6fpTA79/bYgWhenkHwrxBa5ZnYoELh4cCTxWNQhx7VdL+BpaabwrRgGNO0Tx072J43deyHSPkkTfXUeZNblXDDcFHSc3FEe59px+prsjHUuMWXjxZFhd0mDQ4vOZWW+QCkcV32GWpJS4= X-OriginatorOrg: oracle.com X-MS-Exchange-CrossTenant-Network-Message-Id: 8ff7648d-1d74-42e8-afce-08dbdb7be0f6 X-MS-Exchange-CrossTenant-AuthSource: BN6PR1001MB2147.namprd10.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Nov 2023 08:15:30.7063 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: QLVmcz+Tm+FFl3dhTF/8ogC+EMHmZAoGahGS6gALEX5gnN6PsQKut3CrQ8338gtcjkPTGUy5NWb4LlMDnQpaXg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB4942 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.272,Aquarius:18.0.987,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-11-01_23,2023-11-01_02,2023-05-22_02 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 malwarescore=0 bulkscore=0 mlxscore=0 adultscore=0 mlxlogscore=999 phishscore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2310240000 definitions=main-2311020064 X-Proofpoint-GUID: yK3j_yGY1J6zk-Z4hjLkOrLj4mNr5dDa X-Proofpoint-ORIG-GUID: yK3j_yGY1J6zk-Z4hjLkOrLj4mNr5dDa X-Spam-Status: No, score=-7.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Jan, Thanks for reviewing. On 10/31/23 07:10, Jan Beulich wrote: > On 30.10.2023 17:51, Indu Bhagat wrote: >> gas/ >> * Makefile.am: Add new files. >> * Makefile.in: Regenerated. >> * as.c (defined): Handle documentation and listing option for >> ginsns and SCFI. >> * config/obj-elf.c (obj_elf_size): Invoke ginsn_data_end. >> (obj_elf_type): Invoke ginsn_data_begin. >> * config/tc-i386.c (ginsn_new): New functionality to generate >> ginsns. >> (x86_scfi_callee_saved_p): New function. >> (ginsn_dw2_regnum): Likewise. >> (ginsn_set_where): Likewise. >> (x86_ginsn_alu): Likewise. >> (x86_ginsn_move): Likewise. >> (x86_ginsn_lea): Likewise. >> (x86_ginsn_jump): Likewise. >> (x86_ginsn_jump_cond): Likewise. >> (md_assemble): Invoke ginsn_new. >> (s_insn): Likewise. >> (i386_target_format): Add hard error for usage of --scfi with non AMD64 ABIs. >> * config/tc-i386.h (TARGET_USE_GINSN): New definition. >> (TARGET_USE_SCFI): Likewise. >> (SCFI_NUM_REGS): Likewise. >> (REG_FP): Likewise. >> (REG_SP): Likewise. >> (SCFI_INIT_CFA_OFFSET): Likewise. >> (SCFI_CALLEE_SAVED_REG_P): Likewise. >> (x86_scfi_callee_saved_p): Likewise. > > For this arch-specific code there's a fundamental question of maintenance > cost here: It doesn't look very reasonable to me to demand of people adding > support for new ISA extensions to also take into consideration all of this > new machinery. Yet if any such addition affects SCFI, things will go out- > of-sync without updating this code as well. It may not be very often that > such updating is necessary, but right now there's APX work in progress > which I expect will need dealing with here as well. > I understand your concerns. FWIW, for SCFI, we will need to translate only a subset of instructions into ginsns (change of flow insns, save/restore and arith on REG_SP/REG_FP). For APX, I see that there is are new instructions that fall in this set unfortunately. Hopefully the set remains small. But in general, for future additions (and for APX currently), there will be time to act as SCFI is for hand-written asm for which synthesizing CFI is desired, not for all code that GAS necessarily has to deal with. At the moment, our target is to see if the kernel can use SCFI for a subset of their hand-written asm for x86_64 and aarch64. >> --- a/gas/as.c >> +++ b/gas/as.c >> @@ -45,6 +45,7 @@ >> #include "codeview.h" >> #include "bfdver.h" >> #include "write.h" >> +#include "ginsn.h" >> >> #ifdef HAVE_ITBL_CPU >> #include "itbl-ops.h" >> @@ -245,6 +246,7 @@ Options:\n\ >> d omit debugging directives\n\ >> g include general info\n\ >> h include high-level source\n\ >> + i include ginsn and synthesized CFI info\n\ >> l include assembly\n\ >> m include macro expansions\n\ >> n omit forms processing\n\ > > Please can you make indentation match neighboring code? > OK. >> --- a/gas/config/obj-elf.c >> +++ b/gas/config/obj-elf.c >> @@ -24,6 +24,7 @@ >> #include "subsegs.h" >> #include "obstack.h" >> #include "dwarf2dbg.h" >> +#include "ginsn.h" >> >> #ifndef ECOFF_DEBUGGING >> #define ECOFF_DEBUGGING 0 >> @@ -2302,6 +2303,13 @@ obj_elf_size (int ignore ATTRIBUTE_UNUSED) >> symbol_get_obj (sym)->size = XNEW (expressionS); >> *symbol_get_obj (sym)->size = exp; >> } >> + >> + /* If the symbol in the directive matches the current function being >> + processed, indicate end of the current stream of ginsns. */ >> + if (flag_synth_cfi >> + && S_IS_FUNCTION (sym) && sym == ginsn_data_func_symbol ()) >> + ginsn_data_end (symbol_temp_new_now ()); >> + >> demand_empty_rest_of_line (); >> } > > Besides the restrictions mentioned, presence of this just in obj-elf.c > means non-ELF targets also won't be able to use this. You'll therefore > want to adjust the config/tc-i386.h such that e.g. COFF targets don't > needlessly have a large set of dead code compiled in. > OK. >> --- a/gas/config/tc-i386.c >> +++ b/gas/config/tc-i386.c >> @@ -30,6 +30,7 @@ >> #include "subsegs.h" >> #include "dwarf2dbg.h" >> #include "dw2gencfi.h" >> +#include "scfi.h" >> #include "gen-sframe.h" >> #include "sframe.h" >> #include "elf/x86-64.h" >> @@ -193,8 +194,11 @@ static unsigned int x86_isa_1_used; >> static unsigned int x86_feature_2_used; >> /* Generate x86 used ISA and feature properties. */ >> static unsigned int x86_used_note = DEFAULT_X86_USED_NOTE; >> + >> #endif > > Nit: Stray change? > >> +static ginsnS *ginsn_new (symbolS *sym, enum ginsn_gen_mode gmode); > > I don't see the need for this: The function definition lives ahead of any > caller afaics. > OK to both. >> @@ -5116,6 +5120,716 @@ static INLINE bool may_need_pass2 (const insn_template *t) >> && t->base_opcode == 0x63); >> } >> >> +bool >> +x86_scfi_callee_saved_p (uint32_t dw2reg_num) >> +{ >> + if (dw2reg_num == 3 /* rbx. */ >> + || dw2reg_num == REG_FP /* rbp. */ >> + || dw2reg_num == REG_SP /* rsp. */ >> + || (dw2reg_num >= 12 && dw2reg_num <= 15) /* r12 - r15. */) >> + return true; > > Non-GPRs aren't of interest here? > > What about the Windows ABI, which is also used in the EFI world? > Windows ABI can be worked out later, if there is interest. The current set of abstractions used in SCFI machinery are being worked out while ensuring that they work for at least two ABIs at the minimum - AMD64 and AAPCS64. So adding support for another ABI, hopefully gets some help. >> + return false; >> +} >> + >> +static uint32_t >> +ginsn_dw2_regnum (const reg_entry *ireg) >> +{ >> + /* PS: Note the data type here as int32_t, because of Dw2Inval (-1). */ >> + int32_t dwarf_reg = Dw2Inval; >> + const reg_entry *temp; >> + >> + if (ireg->dw2_regnum[0] == Dw2Inval && ireg->dw2_regnum[1] == Dw2Inval) >> + return dwarf_reg; >> + >> + dwarf_reg = ireg->dw2_regnum[flag_code >> 1]; > > flag_code == CODE_64BIT would likely be more robust. > > Also are all calls here synchronous, i.e. happening right when a > particular insn is processed? Otherwise what about flag_code changing > in between? > Looks like I havent factored that in. I think we will need to error out for now if flag_code changes in between. I will need to take a look at the instructions and ABI differences to see what changes are necessary to support this. > Plus in how far does it make sense to handle other than CODE_64BIT when > you tie yourself to the SysV 64-bit ABI? > >> + if (dwarf_reg == Dw2Inval) >> + { >> + temp = ireg + 16; >> + dwarf_reg = ginsn_dw2_regnum (temp); >> + } > > What is this about? This clearly needs a comment, and it may also need > a comment in opcodes/i386-reg.tbl to clarify that certain ordering > requirements have to be retained. It also wants to be clarified that > the recursion here won't be indefinite. > This code is indeed relying on the ordering of the the entries in i386_regtab []. Its not a good way to achieve what it is doing, but I found no other way at that time. I will add code comments. >> + if (dwarf_reg == Dw2Inval) >> + gas_assert (1); /* Needs to be addressed. */ > > This is dead code (you appear to mean 0 instead of 1). Yet even then > still gas_assert (dwarf_reg != Dw2Inval) please. > Will fix. >> + return (uint32_t) dwarf_reg; >> +} >> + >> +static void >> +ginsn_set_where (ginsnS* ginsn) > > Nit: Misplaced *. > OK. >> +{ >> + const char *file; >> + unsigned int line; >> + file = as_where (&line); >> + ginsn_set_file_line (ginsn, file, line); >> +} > > This function, also from its name, isn't x86-specific, is it? As such > it wants to live in ginsn.[ch]. > OK. >> +static ginsnS * >> +x86_ginsn_alu (i386_insn insn, symbolS *insn_end_sym) >> +{ >> + offsetT src_imm; >> + uint32_t dw2_regnum; >> + enum ginsn_src_type src_type; >> + enum ginsn_dst_type dst_type; >> + ginsnS *ginsn = NULL; >> + >> + /* FIXME - create ginsn for REG_SP target only ? */ >> + /* Map for insn.tm.extension_opcode >> + 000 ADD 100 AND >> + 001 OR 101 SUB >> + 010 ADC 110 XOR >> + 011 SBB 111 CMP */ >> + >> + /* add/sub imm, %reg. >> + and imm, %reg only at this time for SCFI. */ >> + if (!(insn.tm.extension_opcode == 0 >> + || insn.tm.extension_opcode == 4 >> + || insn.tm.extension_opcode == 5)) >> + return ginsn; > > Why is AND permitted, but OR/XOR aren't? > > Also this is about ALU insns with immediate operands only, yet that > fact isn't reflected in the function name. > OR/XOR should be handled. I will fix this. This function is handling opcodes with imm only, true. ADD/SUB with reg are handled elsewhere (with opcode == 0x1 and opcode == 0x29). >> + /* TBD_GINSN_REPRESENTATION_LIMIT: There is no representation for when a >> + symbol is used as an operand, like so: >> + addq $simd_cmp_op+8, %rdx >> + Skip generating any ginsn for this. */ >> + if (insn.imm_operands == 1 >> + && insn.op[0].imms->X_op == O_symbol) >> + return ginsn; >> + >> + gas_assert (insn.imm_operands == 1 >> + && insn.op[0].imms->X_op == O_constant); > > Perhaps less fragile if you use != O_constant in the preceding if()? > The remaining half could the move ahead of that if(), allowing half > of its condition to also be dropped. > >> + src_imm = insn.op[0].imms->X_add_number; >> + /* The second operand may be a register or indirect access. */ >> + if (insn.mem_operands == 1 && insn.base_reg) >> + { >> + dw2_regnum = ginsn_dw2_regnum (insn.base_reg); >> + src_type = GINSN_SRC_INDIRECT; >> + dst_type = GINSN_DST_INDIRECT; > > The possibly in use index register isn't of interest in this case? > Nor the displacement in the memory operand, ... > Not of interest, No. When src_type and dst_type is INDIRECT. >> + } >> + else if (insn.mem_operands == 1 && insn.index_reg) >> + { >> + dw2_regnum = ginsn_dw2_regnum (insn.index_reg); >> + src_type = GINSN_SRC_INDIRECT; >> + dst_type = GINSN_DST_INDIRECT; > > ... similarly applicable here? Nor a segment override? Segment override probably is not of interest for SCFI. > >> + } >> + else >> + { >> + gas_assert (insn.reg_operands == 1); > > Afaict this will trigger when the memory operand has neither base > nor index. > memory operand with neither base nor index ? An asm instruction example will be helpful. >> + dw2_regnum = ginsn_dw2_regnum (insn.op[1].regs); >> + src_type = GINSN_SRC_REG; >> + dst_type = GINSN_DST_REG; >> + } >> + >> + /* For ginsn, keep the imm as second src operand. */ >> + if (insn.tm.extension_opcode == 5) >> + ginsn = ginsn_new_sub (insn_end_sym, true, >> + src_type, dw2_regnum, 0, >> + GINSN_SRC_IMM, 0, src_imm, >> + dst_type, dw2_regnum, 0); >> + else if (insn.tm.extension_opcode == 4) >> + ginsn = ginsn_new_and (insn_end_sym, true, >> + src_type, dw2_regnum, 0, >> + GINSN_SRC_IMM, 0, src_imm, >> + dst_type, dw2_regnum, 0); >> + else if (insn.tm.extension_opcode == 0) >> + ginsn = ginsn_new_add (insn_end_sym, true, >> + src_type, dw2_regnum, 0, >> + GINSN_SRC_IMM, 0, src_imm, >> + dst_type, dw2_regnum, 0); > > I think this would benefit from setting a function pointer near the > top of the function. Else can this please at least be put in switch() > form? > In this case, function pointer will also work. >> + ginsn_set_where (ginsn); >> + >> + return ginsn; >> +} >> + >> +static ginsnS * >> +x86_ginsn_move (i386_insn insn, symbolS *insn_end_sym) >> +{ >> + ginsnS *ginsn; >> + uint16_t opcode; >> + uint32_t dst_reg; >> + uint32_t src_reg; >> + offsetT dst_disp; >> + offsetT src_disp; >> + const reg_entry *dst = NULL; >> + const reg_entry *src = NULL; >> + enum ginsn_dst_type dst_type; >> + enum ginsn_src_type src_type; >> + >> + opcode = insn.tm.base_opcode; >> + src_type = GINSN_SRC_REG; >> + src_disp = dst_disp = 0; >> + dst_type = GINSN_DST_REG; > > Please can such be initializers of their variables? > OK. >> + if (opcode == 0x8b) >> + { >> + /* mov disp(%reg), %reg. */ >> + if (insn.mem_operands && insn.base_reg) >> + { >> + src = insn.base_reg; >> + if (insn.disp_operands == 1) >> + src_disp = insn.op[0].disps->X_add_number; >> + src_type = GINSN_SRC_INDIRECT; >> + } >> + else >> + src = insn.op[0].regs; > > What about disp(%reg,%reg) or disp(,%reg) kinds of memory operands? > I will take a look. >> + dst = insn.op[1].regs; >> + } >> + else if (opcode == 0x89 || opcode == 0x88) > > How come 0x88 is handled here, but 0x8a isn't handled above? > It should have been. I will take a look. >> + { >> + /* mov %reg, disp(%reg). */ >> + src = insn.op[0].regs; >> + if (insn.mem_operands && insn.base_reg) >> + { >> + dst = insn.base_reg; >> + if (insn.disp_operands == 1) >> + dst_disp = insn.op[1].disps->X_add_number; >> + dst_type = GINSN_DST_INDIRECT; >> + } >> + else >> + dst = insn.op[1].regs; >> + } >> + >> + src_reg = ginsn_dw2_regnum (src); >> + dst_reg = ginsn_dw2_regnum (dst); >> + >> + ginsn = ginsn_new_mov (insn_end_sym, true, >> + src_type, src_reg, src_disp, >> + dst_type, dst_reg, dst_disp); >> + ginsn_set_where (ginsn); >> + >> + return ginsn; >> +} >> + >> +static ginsnS * >> +x86_ginsn_lea (i386_insn insn, symbolS *insn_end_sym) >> +{ >> + offsetT src_disp = 0; >> + ginsnS *ginsn = NULL; >> + uint32_t base_reg; >> + uint32_t index_reg; >> + offsetT index_scale; >> + uint32_t dst_reg; >> + >> + if (!insn.index_reg && !insn.base_reg) >> + { >> + /* lea symbol, %rN. */ >> + dst_reg = ginsn_dw2_regnum (insn.op[1].regs); >> + /* FIXME - Skip encoding information about the symbol. >> + This is TBD_GINSN_INFO_LOSS, but it is fine if the mode is >> + GINSN_GEN_SCFI. */ >> + ginsn = ginsn_new_mov (insn_end_sym, false, >> + GINSN_SRC_IMM, 0xf /* arbitrary const. */, 0, >> + GINSN_DST_REG, dst_reg, 0); >> + } >> + else if (insn.base_reg && !insn.index_reg) >> + { >> + /* lea -0x2(%base),%dst. */ >> + base_reg = ginsn_dw2_regnum (insn.base_reg); >> + dst_reg = ginsn_dw2_regnum (insn.op[1].regs); >> + >> + if (insn.disp_operands) >> + src_disp = insn.op[0].disps->X_add_number; > > What if the displacement expression is other than O_constant? > For SCFI, a "complex" lea instruction will imply some untraceable change to the destination reg. For SCFI, the extent of untraceable change is not of interest, hence, in general, some information loss in ginsn is tolerable. I will have to take a look at such instructions to see what the options are. If you have a example handy, it will be helpful. >> + if (src_disp) >> + /* Generate an ADD ginsn. */ >> + ginsn = ginsn_new_add (insn_end_sym, true, >> + GINSN_SRC_REG, base_reg, 0, >> + GINSN_SRC_IMM, 0, src_disp, >> + GINSN_DST_REG, dst_reg, 0); >> + else >> + /* Generate a MOV ginsn. */ >> + ginsn = ginsn_new_mov (insn_end_sym, true, >> + GINSN_SRC_REG, base_reg, 0, >> + GINSN_DST_REG, dst_reg, 0); >> + } >> + else if (!insn.base_reg && insn.index_reg) >> + { >> + /* lea (,%index,imm), %dst. */ >> + /* FIXME - Skip encoding an explicit multiply operation, instead use >> + GINSN_TYPE_OTHER. This is TBD_GINSN_INFO_LOSS, but it is fine if >> + the mode is GINSN_GEN_SCFI. */ >> + index_scale = insn.log2_scale_factor; >> + index_reg = ginsn_dw2_regnum (insn.index_reg); >> + dst_reg = ginsn_dw2_regnum (insn.op[1].regs); >> + ginsn = ginsn_new_other (insn_end_sym, true, >> + GINSN_SRC_REG, index_reg, >> + GINSN_SRC_IMM, index_scale, >> + GINSN_DST_REG, dst_reg); >> + } >> + else >> + { >> + /* lea disp(%base,%index,imm) %dst. */ >> + /* FIXME - Skip encoding information about the disp and imm for index >> + reg. This is TBD_GINSN_INFO_LOSS, but it is fine if the mode is >> + GINSN_GEN_SCFI. */ >> + base_reg = ginsn_dw2_regnum (insn.base_reg); >> + index_reg = ginsn_dw2_regnum (insn.index_reg); >> + dst_reg = ginsn_dw2_regnum (insn.op[1].regs); >> + /* Generate an ADD ginsn. */ >> + ginsn = ginsn_new_add (insn_end_sym, true, >> + GINSN_SRC_REG, base_reg, 0, >> + GINSN_SRC_REG, index_reg, 0, >> + GINSN_DST_REG, dst_reg, 0); > > The comment mentions the displacement, but it's not passed on? In the earlier > "else if" block you also pay attention to the scla factor, but here you don't? > That said, I think I'm confused anyway about the FIXME comments. > Ah, This is what I was hinting at in the previous comment. So whats going on here is the following - First, An "add reg1, reg2, reg3" (reg3 being the destination) ginsn is only interesting for SCFI if the reg3 is either REG_FP or REG_SP. We still generate them all nevertheless for now. Second, We say that "add reg1, reg2, reg3" makes reg3 "untraceable". This is based on the observation that static stack usage in a function will use imm based arithmetic insns. So, if reg3 is REG_SP, REG_FP, these instructions make REG_SP / REG_FP "untraceable". Note that, We are not allowing all registers for CFA tracking. (Now _this_ is a constraint, yes. And which is why we dont support DRAP usage currently. But its a specific pattern that we can accommodate in future.) Hence, tracking more information for such (untraceable) instructions is unnecessary for SCFI purposes. If and when GAS uses ginsn (in the i386 backend) for other purposes, such information loss will need to be revisited. >> + } >> + >> + ginsn_set_where (ginsn); >> + >> + return ginsn; >> +} >> + >> +static ginsnS * >> +x86_ginsn_jump (i386_insn insn, symbolS *insn_end_sym) >> +{ >> + ginsnS *ginsn = NULL; >> + symbolS *src_symbol; >> + >> + gas_assert (insn.disp_operands == 1); >> + >> + if (insn.op[0].disps->X_op == O_symbol) >> + { >> + src_symbol = insn.op[0].disps->X_add_symbol; >> + /* The jump target is expected to be a symbol with 0 addend. >> + Assert for now to see if this assumption is true. */ >> + gas_assert (insn.op[0].disps->X_add_number == 0); > > If you assert this means elsewhere this is being checked and a call > here avoided in case the value is non-zero. I can't spot such a > check, though. > No, this assert was for me to take action if this assumption is not true. Just to make sure I see what needs to be done if there are other encodings to take care of. Have not hit this yet in my tests. >> + ginsn = ginsn_new_jump (insn_end_sym, true, >> + GINSN_SRC_SYMBOL, 0, src_symbol); >> + >> + ginsn_set_where (ginsn); >> + } >> + >> + return ginsn; >> +} > > Further, what about XABORT transferring control to what XBEGIN has > supplied? (XBEGIN can, in a sense, also be considered a [conditional] > branch.) > My plan after writing this x86 -> ginsn translator was: There are and will remain many machine instructions that may need to be supported depending on hand-written assmebly programmers use. This is the reason I had left some asserts towards the end in the ginsn_new () API: if any instructions which affect the control flow, or where the destination is REG_FP / REG_SP, or push* / pop* are not handled, we will have an assert fail and GAS will need to handle it. >> +static ginsnS * >> +x86_ginsn_jump_cond (i386_insn insn, symbolS *insn_end_sym) >> +{ >> + ginsnS *ginsn = NULL; >> + symbolS *src_symbol; >> + >> + /* TBD_GINSN_GEN_NOT_SCFI: Ignore move to or from xmm reg for mode. */ >> + if (i.tm.opcode_space == SPACE_0F) >> + return ginsn; > > What is the comment about? And what about SPACE_0F-encoded conditional > jumps (Jcc )? And where are LOOP and J{E,R}CXZ handled? > These need to be handled. I will take a look. >> + gas_assert (insn.disp_operands == 1); >> + >> + if (insn.op[0].disps->X_op == O_symbol) >> + { >> + src_symbol = insn.op[0].disps->X_add_symbol; >> + /* The jump target is expected to be a symbol with 0 addend. >> + Assert for now to see if this assumption is true. */ >> + gas_assert (insn.op[0].disps->X_add_number == 0); >> + ginsn = ginsn_new_jump_cond (insn_end_sym, true, >> + GINSN_SRC_SYMBOL, 0, src_symbol); >> + ginsn_set_where (ginsn); >> + } >> + else >> + /* Catch them for now so we know what we are dealing with. */ >> + gas_assert (0); > > I'm going from the assumption that this (and alike) will be addressed > before this series is committed? > I will work on the ones you have raised above in the review. My plan was to exercise this code in different ways after commmitting and address the failures as I run into them. >> + return ginsn; >> +} >> + >> +/* Generate one or more GAS instructions for the current machine dependent >> + instruction. >> + >> + Returns the head of linked list of ginsn(s) added, if success; >> + Returns NULL if failure. */ >> + >> +static ginsnS * >> +ginsn_new (symbolS *insn_end_sym, enum ginsn_gen_mode gmode) > > x86_ginsn_new()? > Yes, this looks better. >> +{ >> + uint16_t opcode; >> + uint32_t dw2_regnum; >> + uint32_t src2_dw2_regnum; >> + int32_t gdisp = 0; >> + ginsnS *ginsn = NULL; >> + ginsnS *ginsn_next = NULL; >> + ginsnS *ginsn_last = NULL; >> + >> + /* FIXME - Need a way to check whether the decoding is sane. The specific >> + checks around i.tm.opcode_space were added as issues were seen. Likely >> + insufficient. */ > > Furthermore opcode encoding space (SPACE_...) need to be taken into > account in all cases. > >> + /* Currently supports generation of selected ginsns, sufficient for >> + the use-case of SCFI only. To remove this condition will require >> + work on this target-specific process of creation of ginsns. Some >> + of such places are tagged with TBD_GINSN_GEN_NOT_SCFI to serve as >> + examples. */ >> + if (gmode != GINSN_GEN_SCFI) >> + return ginsn; >> + >> + opcode = i.tm.base_opcode; >> + >> + switch (opcode) >> + { >> + case 0x1: >> + /* add reg, reg. */ >> + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); > > You don't care about opcode 0 (byte operation). Then what about 16-bit > operand size? Or, since we're talking of a 64-bit-ABI-only feature, > even 32-bit operand size? > Operand size in some cases does not affect SCFI. So we dont keep information about operand sizes. The case that I should handle operand size is when they are pushed to / popped from stack. > Also what about opcode 0x3? > LSL instruction ? It doesnt look like it can affect DWARF CFI of a function. Please correct me if this is wrong. >> + if (i.reg_operands == 2) >> + { >> + src2_dw2_regnum = ginsn_dw2_regnum (i.op[1].regs); >> + ginsn = ginsn_new_add (insn_end_sym, true, >> + GINSN_SRC_REG, dw2_regnum, 0, >> + GINSN_SRC_REG, src2_dw2_regnum, 0, >> + GINSN_DST_REG, src2_dw2_regnum, 0); >> + ginsn_set_where (ginsn); >> + } >> + else if (i.mem_operands && i.base_reg) >> + { >> + src2_dw2_regnum = ginsn_dw2_regnum (i.base_reg); >> + if (i.disp_operands == 1) >> + gdisp = i.op[1].disps->X_add_number; >> + >> + ginsn = ginsn_new_add (insn_end_sym, true, >> + GINSN_SRC_REG, dw2_regnum, 0, >> + GINSN_SRC_INDIRECT, src2_dw2_regnum, gdisp, >> + GINSN_DST_INDIRECT, src2_dw2_regnum, gdisp); >> + ginsn_set_where (ginsn); >> + } >> + else >> + /* Catch them for now so we know what we are dealing with. */ >> + gas_assert (0); >> + >> + break; >> + case 0x29: >> + /* If opcode_space == SPACE_0F, this is a movaps insn. Skip it >> + for GINSN_GEN_SCFI. */ >> + if (i.tm.opcode_space == SPACE_0F) >> + break; > > Extending on the earlier related comment: Why would you exclude just SPACE_0F? > Perhaps here I should check that space == SPACE_BASE and only allow that? And also for others. >> + /* sub reg, reg. */ >> + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); >> + >> + if (i.reg_operands == 2) >> + { >> + src2_dw2_regnum = ginsn_dw2_regnum (i.op[1].regs); >> + ginsn = ginsn_new_sub (insn_end_sym, true, >> + GINSN_SRC_REG, dw2_regnum, 0, >> + GINSN_SRC_REG, src2_dw2_regnum, 0, >> + GINSN_DST_REG, src2_dw2_regnum, 0); >> + ginsn_set_where (ginsn); >> + } >> + else if (i.mem_operands && i.base_reg) >> + { >> + src2_dw2_regnum = ginsn_dw2_regnum (i.base_reg); >> + if (i.disp_operands == 1) >> + gdisp = i.op[1].disps->X_add_number; >> + >> + ginsn = ginsn_new_sub (insn_end_sym, true, >> + GINSN_SRC_REG, dw2_regnum, 0, >> + GINSN_SRC_INDIRECT, src2_dw2_regnum, gdisp, >> + GINSN_DST_INDIRECT, src2_dw2_regnum, gdisp); >> + ginsn_set_where (ginsn); >> + } >> + else >> + /* Catch them for now so we know what we are dealing with. */ >> + gas_assert (0); >> + >> + break; >> + case 0xa0: >> + case 0xa8: > > Since individual case blocks are non-trivial, can there please be blank > lines between any two non-fall-through case blocks? > OK. >> + /* If opcode_space != SPACE_0F, this is a test insn. Skip it >> + for GINSN_GEN_SCFI. */ >> + if (i.tm.opcode_space != SPACE_0F) >> + break; > > While here you properly use !=, now the comment is wrong. > >> + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); >> + /* push fs / push gs. */ >> + ginsn = ginsn_new_sub (insn_end_sym, false, >> + GINSN_SRC_REG, REG_SP, 0, >> + GINSN_SRC_IMM, 0, 8, > > Note how the 8 here assumes flag_code == CODE_64BIT. (You further assume > no operand size override here.) > Hmm. Yes, I do assume no operand size override here. I will need to understand how to calculate the operand size here. Looks like I need to check for instruction prefixes 66H or REX.W. Is there an API in the backend that be used for this ? >> + GINSN_DST_REG, REG_SP, 0); >> + ginsn_set_where (ginsn); >> + >> + ginsn_next = ginsn_new_store (insn_end_sym, false, >> + GINSN_SRC_REG, dw2_regnum, >> + GINSN_DST_INDIRECT, REG_SP, 0); >> + ginsn_set_where (ginsn_next); >> + >> + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); >> + break; >> + case 0xa1: >> + case 0xa9: >> + /* If opcode_space != SPACE_0F, this is test insn. Skip it >> + for GINSN_GEN_SCFI. */ >> + if (i.tm.opcode_space != SPACE_0F) >> + break; >> + >> + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); >> + /* pop fs / pop gs. */ >> + ginsn = ginsn_new_load (insn_end_sym, false, >> + GINSN_SRC_INDIRECT, REG_SP, 0, >> + GINSN_DST_REG, dw2_regnum); >> + ginsn_set_where (ginsn); >> + >> + ginsn_next = ginsn_new_add (insn_end_sym, false, >> + GINSN_SRC_REG, REG_SP, 0, >> + GINSN_SRC_IMM, 0, 8, >> + GINSN_DST_REG, REG_SP, 0); >> + ginsn_set_where (ginsn_next); >> + >> + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); >> + break; >> + case 0x50 ... 0x57: >> + /* push reg. */ >> + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); >> + ginsn = ginsn_new_sub (insn_end_sym, false, >> + GINSN_SRC_REG, REG_SP, 0, >> + GINSN_SRC_IMM, 0, 8, >> + GINSN_DST_REG, REG_SP, 0); >> + ginsn_set_where (ginsn); >> + >> + ginsn_next = ginsn_new_store (insn_end_sym, false, >> + GINSN_SRC_REG, dw2_regnum, >> + GINSN_DST_INDIRECT, REG_SP, 0); >> + ginsn_set_where (ginsn_next); >> + >> + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); >> + break; >> + case 0x58 ... 0x5f: >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + /* pop reg. */ >> + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); >> + ginsn = ginsn_new_load (insn_end_sym, false, >> + GINSN_SRC_INDIRECT, REG_SP, 0, >> + GINSN_DST_REG, dw2_regnum); >> + ginsn_set_where (ginsn); >> + >> + ginsn_next = ginsn_new_add (insn_end_sym, false, >> + GINSN_SRC_REG, REG_SP, 0, >> + GINSN_SRC_IMM, 0, 8, >> + GINSN_DST_REG, REG_SP, 0); >> + ginsn_set_where (ginsn_next); >> + >> + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); >> + break; >> + case 0x68: >> + case 0x6a: >> + /* push imm. */ >> + /* Skip getting the value of imm from machine instruction >> + because for ginsn generation this is not important. */ > > In (e.g.) kernel code model it is possible to do > > push
> pop %rbp > > (could even be %rsp). How is such a frame (or stack) pointer change > not relevant? > For SCFI, we are tracking _where_ the saves and restore are done. The values are not important. In the above example, push
must tell the SCFI machinery that stack pointer has been updated. What value was pushed to stack is not important. Now rbp is a callee-saved register, so one must have saved to stack before reovering it (speaking of ABI/calling convention compliant asm). So in the sequence above, the SCFI machinery must complain that this looks fishy. >> + ginsn = ginsn_new_sub (insn_end_sym, false, >> + GINSN_SRC_REG, REG_SP, 0, >> + GINSN_SRC_IMM, 0, 8, >> + GINSN_DST_REG, REG_SP, 0); >> + ginsn_set_where (ginsn); >> + >> + ginsn_next = ginsn_new_store (insn_end_sym, false, >> + GINSN_SRC_IMM, 0, >> + GINSN_DST_INDIRECT, REG_SP, 0); >> + ginsn_set_where (ginsn_next); >> + >> + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); >> + break; >> + case 0x70 ... 0x7f: >> + ginsn = x86_ginsn_jump_cond (i, insn_end_sym); >> + break; >> + case 0x81: >> + case 0x83: >> + ginsn = x86_ginsn_alu (i, insn_end_sym); >> + break; >> + case 0x8b: >> + /* Move r/m64 to r64. */ >> + case 0x88: >> + case 0x89: >> + /* mov reg, reg/mem. */ >> + ginsn = x86_ginsn_move (i, insn_end_sym); >> + break; >> + case 0x8d: >> + /* lea disp(%src), %dst */ >> + ginsn = x86_ginsn_lea (i, insn_end_sym); >> + break; >> + case 0x8f: >> + /* pop to mem. */ >> + gas_assert (i.base_reg); > > This opcode can in principle also have a register operand. We > _currently_ have no way for a user to request to use this > alternative encoding, but that's still a latent issue. In any > event, like elsewhere, all memory operand forms are possible > here, not just (%reg). > >> + dw2_regnum = ginsn_dw2_regnum (i.base_reg); >> + ginsn = ginsn_new_load (insn_end_sym, false, >> + GINSN_SRC_INDIRECT, REG_SP, 0, >> + GINSN_DST_INDIRECT, dw2_regnum); > > When both operands are "indirect", what's the difference between > move, load, and store? IOW if the above is permitted, can't all > three be folded into a single ginsn kind? > In theory, any of mov/load/store ginsn type may be used if both operands are indirect mem access. >> + ginsn_set_where (ginsn); >> + >> + ginsn_next = ginsn_new_add (insn_end_sym, false, >> + GINSN_SRC_REG, REG_SP, 0, >> + GINSN_SRC_IMM, 0, 8, >> + GINSN_DST_REG, REG_SP, 0); >> + ginsn_set_where (ginsn_next); >> + >> + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); >> + break; >> + case 0x9c: >> + /* pushf / pushfd / pushfq. >> + Tracking EFLAGS register by number is not necessary. */ >> + ginsn = ginsn_new_sub (insn_end_sym, false, >> + GINSN_SRC_REG, REG_SP, 0, >> + GINSN_SRC_IMM, 0, 8, > > Why does the comment mention PUSHFD when you assume PUSHFQ here? > Pushfd should not be stated here. I see pushfd is not valid for 64-bit op mode. Will fix it. >> + GINSN_DST_REG, REG_SP, 0); >> + ginsn_set_where (ginsn); >> + >> + ginsn_next = ginsn_new_store (insn_end_sym, false, >> + GINSN_SRC_IMM, 0, >> + GINSN_DST_INDIRECT, REG_SP, 0); >> + ginsn_set_where (ginsn_next); >> + >> + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); >> + >> + break; > > Where's POPFD? > Missed it! popf / popfq opcode == 0x9d will be handled. >> + case 0xff: >> + /* push from mem. */ >> + if (i.tm.extension_opcode == 6) >> + { >> + ginsn = ginsn_new_sub (insn_end_sym, false, >> + GINSN_SRC_REG, REG_SP, 0, >> + GINSN_SRC_IMM, 0, 8, >> + GINSN_DST_REG, REG_SP, 0); >> + ginsn_set_where (ginsn); >> + >> + /* These instructions have no imm, only indirect access. */ >> + gas_assert (i.base_reg); >> + dw2_regnum = ginsn_dw2_regnum (i.base_reg); >> + ginsn_next = ginsn_new_store (insn_end_sym, false, >> + GINSN_SRC_INDIRECT, dw2_regnum, >> + GINSN_DST_INDIRECT, REG_SP, 0); >> + ginsn_set_where (ginsn_next); >> + >> + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); >> + } >> + else if (i.tm.extension_opcode == 4) >> + { >> + /* jmp r/m. E.g., notrack jmp *%rax. */ >> + if (i.reg_operands) >> + { >> + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); >> + ginsn = ginsn_new_jump (insn_end_sym, true, >> + GINSN_SRC_REG, dw2_regnum, NULL); >> + ginsn_set_where (ginsn); >> + } >> + else if (i.mem_operands && i.index_reg) >> + { >> + /* jmp *0x0(,%rax,8). */ >> + dw2_regnum = ginsn_dw2_regnum (i.index_reg); >> + ginsn = ginsn_new_jump (insn_end_sym, true, >> + GINSN_SRC_REG, dw2_regnum, NULL); >> + ginsn_set_where (ginsn); >> + } >> + else if (i.mem_operands && i.base_reg) >> + { >> + dw2_regnum = ginsn_dw2_regnum (i.base_reg); >> + ginsn = ginsn_new_jump (insn_end_sym, true, >> + GINSN_SRC_REG, dw2_regnum, NULL); >> + ginsn_set_where (ginsn); >> + } >> + else >> + /* Catch them for now so we know what we are dealing with. */ >> + gas_assert (0); >> + } >> + else if (i.tm.extension_opcode == 2) >> + { >> + /* 0xFF /2 (call). */ >> + if (i.reg_operands) >> + { >> + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); >> + ginsn = ginsn_new_call (insn_end_sym, true, >> + GINSN_SRC_REG, dw2_regnum, NULL); >> + ginsn_set_where (ginsn); >> + } >> + else if (i.mem_operands && i.base_reg) >> + { >> + dw2_regnum = ginsn_dw2_regnum (i.base_reg); >> + ginsn = ginsn_new_call (insn_end_sym, true, >> + GINSN_SRC_REG, dw2_regnum, NULL); >> + ginsn_set_where (ginsn); >> + } >> + else >> + /* Catch them for now so we know what we are dealing with. */ >> + gas_assert (0); >> + } >> + else >> + /* Catch them for now so we know what we are dealing with. */ >> + gas_assert (0); >> + break; >> + case 0xc2: >> + case 0xc3: >> + /* Near ret. */ >> + ginsn = ginsn_new_return (insn_end_sym, true); >> + ginsn_set_where (ginsn); >> + break; > > Where did the immediate operand of 0xC2 go? And what about far return > (and more generally far branches)? > A return ginsn for SCFI can afford this information loss. >> + case 0xc9: >> + /* The 'leave' instruction copies the contents of the RBP register >> + into the RSP register to release all stack space allocated to the >> + procedure. */ >> + ginsn = ginsn_new_mov (insn_end_sym, false, >> + GINSN_SRC_REG, REG_FP, 0, >> + GINSN_DST_REG, REG_SP, 0); >> + ginsn_set_where (ginsn); >> + >> + /* Then it restores the old value of the RBP register from the stack. */ >> + ginsn_next = ginsn_new_load (insn_end_sym, false, >> + GINSN_SRC_INDIRECT, REG_SP, 0, >> + GINSN_DST_REG, REG_FP); >> + ginsn_set_where (ginsn_next); >> + >> + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); >> + ginsn_last = ginsn_new_add (insn_end_sym, false, >> + GINSN_SRC_REG, REG_SP, 0, >> + GINSN_SRC_IMM, 0, 8, >> + GINSN_DST_REG, REG_SP, 0); >> + ginsn_set_where (ginsn_next); >> + >> + gas_assert (!ginsn_link_next (ginsn_next, ginsn_last)); >> + break; > > You handle LEAVE, but not ENTER? > I should have. From the three variants ([Enter imm16,0], [ENTER imm16,1] and [ENTER imm16, imm8], the last one is not super clear currently. Is it possible for you to pass down some understanding to me on that ? >> + case 0xe8: >> + /* PS: SCFI machinery does not care about which func is being >> + called. OK to skip that info. */ >> + ginsn = ginsn_new_call (insn_end_sym, true, >> + GINSN_SRC_SYMBOL, 0, NULL); >> + ginsn_set_where (ginsn); >> + break; >> + case 0xe9: >> + case 0xeb: >> + /* If opcode_space == SPACE_0F, this is a psubw por insn. Skip it >> + for GINSN_GEN_SCFI. */ >> + if (i.tm.opcode_space == SPACE_0F) >> + break; >> + >> + /* Unconditional jmp. */ >> + ginsn = x86_ginsn_jump (i, insn_end_sym); >> + ginsn_set_where (ginsn); >> + break; >> + /* Fall Through. */ >> + default: > > There's noo fall-through here. > >> + /* TBD_GINSN_GEN_NOT_SCFI: Keep a warning, for now, to find out about >> + possibly missed instructions affecting REG_SP or REG_FP. These >> + checks may not be completely exhaustive as they do not involve >> + index / base reg. */ > > Cases with early "break" should probably come here as well. > OK. >> + if (i.op[0].regs) >> + { >> + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); > > This isn't valid without first having checked that operand 0 is a > register operand: "regs" is a member of a union, and you may not > pass expressionS * into this function. > >> + if (dw2_regnum == REG_SP || dw2_regnum == REG_FP) >> + as_warn_where (last_insn.file, last_insn.line, >> + _("SCFI: unhandled op 0x%x may cause incorrect CFI"), >> + i.tm.base_opcode); >> + } >> + if (i.op[1].regs) >> + { >> + dw2_regnum = ginsn_dw2_regnum (i.op[1].regs); >> + if (dw2_regnum == REG_SP || dw2_regnum == REG_FP) >> + as_warn_where (last_insn.file, last_insn.line, >> + _("SCFI: unhandled op 0x%x may cause incorrect CFI"), >> + i.tm.base_opcode); >> + } > > What about insns with 3 GPR operands? > > Also, why do you use as_warn_where() here? You're still in the > context of synchonously processing the current input line. > >> + /* Keep an eye on other instructions affecting control flow. */ >> + gas_assert (!i.tm.opcode_modifier.jump); > > Again this cannot remain like this, even if here there's no FIXME > remark. > My thinking was to exercise the code more soon and fix the cases as I run into them. And do this in iterations after the patches go in. >> + /* TBD_GINSN_GEN_NOT_SCFI: Skip all other opcodes uninteresting for >> + GINSN_GEN_SCFI mode. */ > > "uninteresting" is an interesting term when dealing with hand-written > assembly. How do you know what an assembly writer is going to do? > While the patch description mentions a few constraints, having come > here I'm under the impression that there are quite a few more of them. > Maybe I simply didn't spot where all caveats are fully spelled out? > I have tried to clear up some things in my comments above. Perhaps scfi.c will make things clearer. >> @@ -5128,6 +5842,7 @@ md_assemble (char *line) >> const char *end, *pass1_mnem = NULL; >> enum i386_error pass1_err = 0; >> const insn_template *t; >> + ginsnS *ginsn; >> >> /* Initialize globals. */ >> current_templates = NULL; >> @@ -5659,6 +6374,13 @@ md_assemble (char *line) >> /* We are ready to output the insn. */ >> output_insn (); >> >> + /* At this time, SCFI is enabled only for AMD64 ABI. */ >> + if (flag_synth_cfi && x86_elf_abi == X86_64_ABI) >> + { >> + ginsn = ginsn_new (symbol_temp_new_now (), frch_ginsn_gen_mode ()); >> + frch_ginsn_data_append (ginsn); >> + } > > Why would this not work for the x32 ABI? > As per my understanding, all of this should work for X86_64_X32_ABI. Its just that I would like to double-check and run tests. >> @@ -10904,6 +11626,7 @@ s_insn (int dummy ATTRIBUTE_UNUSED) >> valueT val; >> bool vex = false, xop = false, evex = false; >> static const templates tt = { &i.tm, &i.tm + 1 }; >> + ginsnS *ginsn; >> >> init_globals (); >> >> @@ -11658,7 +12381,14 @@ s_insn (int dummy ATTRIBUTE_UNUSED) >> >> output_insn (); >> >> - done: >> + /* At this time, SCFI is enabled only for AMD64 ABI. */ >> + if (flag_synth_cfi && x86_elf_abi == X86_64_ABI) >> + { >> + ginsn = ginsn_new (symbol_temp_new_now (), frch_ginsn_gen_mode ()); >> + frch_ginsn_data_append (ginsn); >> + } >> + >> +done: > > Please can you leave label indentation alone? > Oops, will fix. >> @@ -15293,6 +16023,9 @@ i386_target_format (void) >> else >> as_fatal (_("unknown architecture")); >> >> + if (flag_synth_cfi && x86_elf_abi != X86_64_ABI) >> + as_fatal (_("Synthesizing CFI is not supported for this ABI")); > > With this, what use are the x86_elf_abi checks in md_assemble() and > s_insn()? > Yes, there is redundancy; I could remove from md_assemble () and s_insn (). >> --- a/gas/config/tc-i386.h >> +++ b/gas/config/tc-i386.h >> @@ -373,6 +373,27 @@ extern int i386_elf_section_type (const char *, size_t); >> extern void i386_solaris_fix_up_eh_frame (segT); >> #endif >> >> +#define TARGET_USE_GINSN 1 >> +/* Allow GAS to synthesize DWARF CFI for hand-written asm. >> + PS: TARGET_USE_CFIPOP is a pre-condition. */ >> +#define TARGET_USE_SCFI 1 >> +/* Identify the maximum DWARF register number of all the registers being >> + tracked for SCFI. This is the last DWARF register number of the set >> + of SP, BP, and all callee-saved registers. For AMD64, this means >> + R15 (15). Use SCFI_CALLEE_SAVED_REG_P to identify which registers >> + are callee-saved from this set. */ >> +#define SCFI_NUM_REGS 15 > > Is this "number of" (and then having a value of 16 to permit covering > register 15), or is this "maximum register number" (and then needing a > different name)? > This is "maximum register number". I will rename this to SCFI_MAX_REG_ID. >> --- /dev/null >> +++ b/gas/ginsn.c > > I'll see about replying on the non-x86 parts of this separately. > Thanks again for reviewing. The reviews have been very helpful.