From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-00069f02.pphosted.com (mx0b-00069f02.pphosted.com [205.220.177.32]) by sourceware.org (Postfix) with ESMTPS id 8D3A33858C52 for ; Wed, 10 Jan 2024 06:10:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8D3A33858C52 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 8D3A33858C52 Authentication-Results: server2.sourceware.org; arc=pass smtp.remote-ip=205.220.177.32 ARC-Seal: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1704867011; cv=pass; b=GZC7cHSfH/NH3AmXlVAcGMaStv7fgDJI45xB1nvJJ05vmTjQcII4xZxGYqyoCV0AvQeoJtYC4NDwBnT+lepsRgqB+BPk7ph/IIA6J63M9MOspqtm/dkovdZjPwlf2ZieTJ6TjuOqmUwTT2kkndeKe6CrG2ZYHsGBHgVim1fakRg= ARC-Message-Signature: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1704867011; c=relaxed/simple; bh=jJ3u+adOvO61B/jRgPZ3ETaxjHwsE42+loPiJwHo6Cs=; h=DKIM-Signature:DKIM-Signature:Message-ID:Date:Subject:To:From: MIME-Version; b=U7IRdLgsLyaTSWnrLBh3RRhrXi+BmXVfF5gMqL6UmPRWkKNqExY7fFuhWEHb0gyl/ayoK4e+QcYN3qS8R9H/PxXeoxsbWaXWcuMABlAC/5juXgCmliQISa3ci818AxJ5uXKpkTh8Vkjt2+JNGfztenscU1vUItpKwzfI7n6kO/Q= ARC-Authentication-Results: i=2; server2.sourceware.org Received: from pps.filterd (m0246630.ppops.net [127.0.0.1]) by mx0b-00069f02.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 40A5uuUf021901; Wed, 10 Jan 2024 06:10:07 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=message-id : date : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding : mime-version; s=corp-2023-11-20; bh=xquTuZVnCooAcb2iNFJ8ICs05Q+rGWKB48sRWLYsohw=; b=L9EAToaCjmAA7/pv1H48dlW/r8FKoJUNXqaoRiOg1Lqj3Os+QCSfLLZTvyonN9uDQ9+t /g2DfBezsBb8GwHS3u9S730aQV8qrFS7Mx9Yadj7qb2+uQZOBwOhlAuUSZRuqbG9/A65 Rb//Y0CIg/6lK/MMSruQlD0cYk5Z0K7wJRoG9Slcbte9OvShM1HA32q5y/iAHxKOhunz 8mqghO2/bgbXE/xW1c8W3hlsQdkOuCuehpkoCpGfiDTfWcdH1IPdRDR5Y1X3daaJyQyn 4mJWwLgiTTa4NdHTmZOimUjZre3kAv1z6JhbwfNIQpaowyEhhMkS7qhZr8tp057ntv67 XQ== Received: from iadpaimrmta01.imrmtpd1.prodappiadaev1.oraclevcn.com (iadpaimrmta01.appoci.oracle.com [130.35.100.223]) by mx0b-00069f02.pphosted.com (PPS) with ESMTPS id 3vhnhdr0wb-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 10 Jan 2024 06:10:06 +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 40A4tcVx035903; Wed, 10 Jan 2024 06:10:06 GMT Received: from nam11-dm6-obe.outbound.protection.outlook.com (mail-dm6nam11lp2169.outbound.protection.outlook.com [104.47.57.169]) by iadpaimrmta01.imrmtpd1.prodappiadaev1.oraclevcn.com (PPS) with ESMTPS id 3vfuu5kyqn-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 10 Jan 2024 06:10:05 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=D0lY17y2rnwjcj1zwEYLfT1VXnfyUlcUQfaqyubjXYlRR/8b37uPnM2LbAz8rzc/ha36qfT0FeBJkLtzvlPzGCy1vJTftxCajN53lzz+b575btjoGBXi9lPSw8b5xEbSvrJ0r1ZgXkuORv0dLrOWAODUgspTS7RalUFFU8GWJXSCnTE02r5NpQKETOr7UjdrSjSPkh/y0xSBRr3EIx/p6JRUzrQ3T5qwuYuLucKgJ45nq3S5XzGDuvswTaey25OhpXhNbt4+0ZCWNNlEnl5d3oqrLAOgidykZt7JWeYGRp9JETHwgZ1PiVmOTEitTo/r9orKqW2xzZTzQC2o/c9toQ== 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=xquTuZVnCooAcb2iNFJ8ICs05Q+rGWKB48sRWLYsohw=; b=n+xOpCs25DvzhzR8C5ZULvKvwfL5p3wlEhmRhUE9PSR5u9E0VX5o+9ECK0Ncx7Q6iEA4PQA/UWlcKjzo+JU3o0hIfBVoIrEkK9MA+F1EDBAjYpaVLA59Z/gog4QwYQ9B+aUrbFAz5mEBtmg5Ztq6gxJJyZJOC8B7b5eYeRuO86ALrUcNQDLO4oGNs5J7Pii53yfe5Zm80lJ7YTvcFK1pS61fmjlFdc5unIJMoO3A56o6hQH35IIBITo2thYAR3c/ZRxLl6oVmSC9uyxQPu6+M1EPC5rme1+2utRzBRbXYO9fK9ZL+76XD8QFmbeQweshqK7yKXE7gYziGAu7ijr/UQ== 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=xquTuZVnCooAcb2iNFJ8ICs05Q+rGWKB48sRWLYsohw=; b=CrLCkqPBv5oZdZB8Hbpx7m5767bGOFVOwALQmCfsAJhn0XMtvAJZ+L4xZG5qlPQktEm8cLTJVuMKUonpPSYStBXW2XKhRtRkw7iXAyYi+sHETaNpord2VqEtpfUe25GrNAOD8EfDdXVt3SdqEY7P0V68Z8ywz/pdj1OOfkL2y+0= Received: from MWHPR1001MB2158.namprd10.prod.outlook.com (2603:10b6:301:2d::17) by BL3PR10MB6019.namprd10.prod.outlook.com (2603:10b6:208:3b2::7) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7159.27; Wed, 10 Jan 2024 06:10:02 +0000 Received: from MWHPR1001MB2158.namprd10.prod.outlook.com ([fe80::fde7:fb92:8ea1:a5ac]) by MWHPR1001MB2158.namprd10.prod.outlook.com ([fe80::fde7:fb92:8ea1:a5ac%4]) with mapi id 15.20.7159.020; Wed, 10 Jan 2024 06:10:02 +0000 Message-ID: <78b9f98f-2030-4675-af0a-8f47d195711b@oracle.com> Date: Tue, 9 Jan 2024 22:10:00 -0800 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH,V4 10/14] gas: synthesize CFI for hand-written asm Content-Language: en-US To: Jan Beulich Cc: binutils@sourceware.org References: <20240103071526.3846985-1-indu.bhagat@oracle.com> <20240103071526.3846985-11-indu.bhagat@oracle.com> <0ecd9240-0700-4072-91d4-ccf9bdb56071@suse.com> <055b92ae-b781-41e8-bd34-4ad68bdc5f6f@suse.com> From: Indu Bhagat In-Reply-To: <055b92ae-b781-41e8-bd34-4ad68bdc5f6f@suse.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: MW2PR2101CA0032.namprd21.prod.outlook.com (2603:10b6:302:1::45) To MWHPR1001MB2158.namprd10.prod.outlook.com (2603:10b6:301:2d::17) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: MWHPR1001MB2158:EE_|BL3PR10MB6019:EE_ X-MS-Office365-Filtering-Correlation-Id: a3c3bba1-405d-483f-ee42-08dc11a2c831 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 7h1cjdhgQBFPLu714lM95QQdFWBiE3Wgvfb2KHNRn5L7oNuD5DFDKJ2OXPG6ngtsGbdhFobjOXTEpZwMUbQztzpi5LvsBf9Cu5Ruf7TJPsnFmecXeKPpfle64vnRBHlu3H82wXP3uWWrjtJsg/ARL2yB7Enc1Aj9svekjTj27h8V/rFtvpcmBTW7zFvpwEyaqIBEfh3oaRWa9i83/kpmdvVkzkXyT5/qvRZ/Sb8VVaBL8OJlb54q5ySPpn1NjjrFT6Svx8K1KcZpMJ56FhIAFre61cnEvdreQvRzZ7jXMJWuA6118O4hojdo/HG/gLyVNVUfGjInG9zAH8Ij/WxBrIlfI3v9Vq+2tXtboypCoNoTL4+ubj9Z7jlhe5IRrnFJPszCLIdE/8Bedb7JRfxBuIkO0whxEpjUQDxWiAec3sLhgvCl+ipfcyqnLzRO8+ZrGqoqXSM9UlBMbgqK1XQmNyEje4j7PTaU/xY7ZqEw+IwbV/tlY/Zti48NnOckRx3fNgl0kw6zFMBAXRO3wP2R2xWcPTLh3usA5hAgFX8gSdBxiDumLo9aT6jlAN5VMGfMa7cH//0JIJIMxuEPE7KlA/GaJu7v0sT3XQccKRlhzqMS65mT+W/b7ZdCU6nP1MRJVuqWV+NzxM/M8JUqVrDzLQ== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:MWHPR1001MB2158.namprd10.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230031)(39860400002)(136003)(366004)(376002)(346002)(396003)(230922051799003)(451199024)(64100799003)(186009)(1800799012)(83380400001)(6512007)(2616005)(38100700002)(53546011)(44832011)(4326008)(8676002)(30864003)(5660300002)(2906002)(6486002)(6506007)(478600001)(8936002)(6916009)(66476007)(66556008)(316002)(66946007)(41300700001)(36756003)(31696002)(86362001)(31686004)(66899024)(45980500001)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?amNMUWpjYXlBUWc4SXJJOXRoSmF3ZERRQTFlNHh3MmR5NVc0emsybzZNM2lz?= =?utf-8?B?OGF4dWpiamxPU21VWGtva0N3b1U5S3Riak1JOTdtNjZTSFM1YUkvNlBMc3Nl?= =?utf-8?B?VnBVTWswUUR0cEt4cDZiWnQ3b3h4ZHloYmREV3FhbmtYbElENVlUcVdNSWZu?= =?utf-8?B?cHM2MTNUUi9LTTB4NEpoTDVEY1pPMG9xQVpWeEFCYXJvL244S2hHNCt6S3Jx?= =?utf-8?B?ZGxPMTRLYllXZjhTN1VtNnR0bVJGSEFQU3BSVk5qcTBEMElKeGRYYVFYWnVH?= =?utf-8?B?UVp2RjVReW4wUEJQOTZXZDZZOGdpNDJuNmhLRmJ0dDEvOU8xUGxWdmpJdXZ4?= =?utf-8?B?UUlobkRYMHIxMnZqeTB0dTVJV2lDbEptOXZzMlhTUFhDQmR0eGtpSVFJdTV1?= =?utf-8?B?bWFOOVI2bWdWWmdNWWxUcjJNUllMYkpNUEtzRTFoUW5EQnE4dFNaRklhU1FI?= =?utf-8?B?bURaNGVsbHhDZW9OYWxTMlZ1dmJEYWVraHdBOWh1RjJidGRhWm51MDV0dWZ2?= =?utf-8?B?QzJDNlEyQVA1SytlQTFScFZMRlhyVktrVDloQ25FcHhFdllzQWFuQXpzcWJS?= =?utf-8?B?Sk1WN2pLWWMzZk9vWFMrbjM4QkhrT2RVdmJueTkvWjQ5UXgzU0NrTFBFeWRt?= =?utf-8?B?UmUxUVdMaGdLQTh2STRpN2YrNFVnVWFZV25JNUJFNEJOUzdPei80TlBpNVk2?= =?utf-8?B?Rk4waXYvM3ZlZ2JVdlRDeTdML0I4cGRuTWxwWitJRUU5T2VGRThVc0tnclVw?= =?utf-8?B?UnpNbGRkdWhNVjlWNHd1cjJVbnZHdklDMDRMb0FyN1hqbi9aWDZhZ2ZIYzhM?= =?utf-8?B?ODdTT3lzSW93RGxPdTc1a0g2eGV3V3lqRGdDelVJWi90WTRCRFNMWUorQXo1?= =?utf-8?B?QUd6SFBJaUFqZjdtVVJtRzFrbmlZb3lrNm81TE1CUzNkaGNoc2xudDhIQklx?= =?utf-8?B?TjBBR29YYUY0d3R0Ukw2RUljUkVoTW8rMU5RLzlRY2J3ekg1Y1hqY0M1MzZ5?= =?utf-8?B?N2RlMmdUUlhrL2dzS0Yzd3FVNXJ2RG1VTW5YV3M2Mkt2Nk91RUFvVGZrZWNa?= =?utf-8?B?TjAyMU9XcWlRVUYyNGh4U1liLzVLK3lENzJNZ3JGdnBUVSt3YkNXZjd3QXc0?= =?utf-8?B?S0dOSnRRa2NaKzdCc20wY0Z0bldpdE1iNGpHTGFTcGd1dG1DL0xUalBDdUsr?= =?utf-8?B?WW05MUNUWTJVcEdkM202aTFwcXFHeHpyZEtBSzE4dCtxTEhnd3l5bzZFMjVq?= =?utf-8?B?ZkFVanFmNEp5NDVQdkwxVG5SMDc5ZU5xc2xVK2dTaFRGTWIxUmphT1BQTzg3?= =?utf-8?B?M0hjWlAycG54eWpSc0o3VVNoZ202M0VCVEZ4NWRkU0lsdzZJbFpWV29Oczd3?= =?utf-8?B?SEJiKzRidTdPVHlLZ0tWVzc0VklkR3BtdzNYNG5ZalV1ekxHN3BqUGxiZ01U?= =?utf-8?B?UkdwOXhNUGpQMHh5MUtWNG5xOStZKzBXSXlmMUZIYlR4U0dRclQrdXUvWURr?= =?utf-8?B?NGtJaThvMHBGSEUvYkxEZUZwQlVLbTVRZDFYblN5blhRT1FqWGxZU3lKTUl4?= =?utf-8?B?OFAvdHRtQWVUQ1MwTmJ0SlRSNWp1c3JNWVVHYjJhd3dlUi80dTd1cXJ3Vkxn?= =?utf-8?B?OG0wMWpjeFRFYUVRQVBBWjVEdVpUQnZzanJrN29ueFNNL2ZCZ1AxSWx3M2gv?= =?utf-8?B?M29vMmJGNmR1WEdsMjdnWEZuZkVXVFQ4WmtXQ0grSHZvRlFnMVFBZWc0M3lN?= =?utf-8?B?NnJrUHRvQ3ZKREZrOEJwanJ0YkUrcjNNQzU0UjYyZnB6UXJoV0M1eXQ5YnZi?= =?utf-8?B?a1BTWlpScHdmNm56bnZGYitSRkZXYXJSWWZXVkp3dGU3VitPbW5DZmRuVWEv?= =?utf-8?B?Z2w5Yy8rRlNwcjNXOHoxNnEyQkxPUEROa0RXZ2w4Q0RRRnVDL2FHK3BtZzVv?= =?utf-8?B?VFFwQm5OaWNVdUhnMTQ3cjRReTgwalBLMVk0STEwaFJPTjB1OWFlN3FwVG9x?= =?utf-8?B?cmJNa3liRDdXemYwbW5SRlNGcmpzcFl2UnRWcEJVcVhsSjV1MGhsb1NCa215?= =?utf-8?B?Mmc4YVc0cXBXMk1qWUNERy9mQXNMZ1lKYkx1M3pBRzFkNkJUMUptUXE4dVZx?= =?utf-8?B?QUoyQ2N1SjA4Rm9NdTYzM1UySHErcE1qb3Zrdng3UEw3UTExUVltMUUwbkYv?= =?utf-8?Q?eevHsONKKPTb39VfafmQ3lM=3D?= X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: AyjWYE9m/5iWyjDx6qQWsFCEji2FYZ4gA5370WB40AN9mSB1yzKSV8IULKR7No7ZZaio19nIJ9n1uP9/yVcRzskpPd8HNYwqqykCQc4miCawpqB/muzRWmXnATzrRNPWncYM4QAKKJFjTBWTf+xB9xG4S3R1vaTwfG7LWqlXsdD9ij1KQFOPDklgVYPSCzg0gJiSycMivlGY4k7Dn1N76F2ETbBWg1SokYr36xjUE7i7ndmgt5NdjfPQZnlJupcoo0W7EJU1N/sqMfdoim6wzJDxmV7TWG1HMRcKMINTNX/6Bswj1u4LPBON7lV2deH5ovBxzUMY0vjC7GzMiTT4LNZxnyZjsUW52kH8kIQK0gOYrJgIx0wbHnU8AqANl2CF6epAD+yRMqdiy99zstwBLXIkZNa0+/2Mj9wYPxqGR8mO36FSMryupI++4bR+OehjWzvKy0SuGh46xCFAYwwRY+HShxLrnlYVVcAsnGxjWD//ERKRkaMQqeO9/wPAVi7B3KpN8/BKW5vTAL+xEeuEnhaGtzv3MO8CtwJRGLFMutVMsqbfBeIxa1+Ij8fc1gdw6QEo4uh4dkP3qmevrzlm9xEPRJcM4UysMqQddUzqXf0= X-OriginatorOrg: oracle.com X-MS-Exchange-CrossTenant-Network-Message-Id: a3c3bba1-405d-483f-ee42-08dc11a2c831 X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2158.namprd10.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jan 2024 06:10:02.2195 (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: EIXY6scNAAs6KGCD8RbFPMEUmU7D8iQXUPrYBS1+dLSszbggheNoOVhFHiqXI2Yt2m+PDCtZodP3YdguFUMl7w== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR10MB6019 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2024-01-10_02,2024-01-09_02,2023-05-22_02 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 phishscore=0 bulkscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 malwarescore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000 definitions=main-2401100048 X-Proofpoint-GUID: nGMrnN93WFPumqQcV2XOdHtVTSdRAO2r X-Proofpoint-ORIG-GUID: nGMrnN93WFPumqQcV2XOdHtVTSdRAO2r X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,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: On 1/9/24 01:30, Jan Beulich wrote: > On 08.01.2024 20:33, Indu Bhagat wrote: >> On 1/5/24 05:58, Jan Beulich wrote: >>> On 03.01.2024 08:15, Indu Bhagat wrote: >>>> @@ -2311,6 +2312,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 (); >>>> } >>>> >>>> @@ -2499,6 +2507,14 @@ obj_elf_type (int ignore ATTRIBUTE_UNUSED) >>>> elfsym->symbol.flags &= ~mask; >>>> } >>>> >>>> + if (S_IS_FUNCTION (sym) && flag_synth_cfi) >>>> + { >>>> + /* Wrap up processing the previous block of ginsns first. */ >>>> + if (frchain_now->frch_ginsn_data) >>>> + ginsn_data_end (symbol_temp_new_now ()); >>>> + ginsn_data_begin (sym); >>>> + } >>>> + >>>> demand_empty_rest_of_line (); >>>> } >>> >>> Documentation about .type and .size use could be more precise. Generally >>> it is entirely benign where exactly these directives live relative to >>> the code they annotate. >> >> Added a comment for V5. >> >> As stated in as.texi, usage of .type and .size will be bread and butter >> for SCFI: "The input asm must begin with the @code{.type} directive, and >> should ideally be closed off using a @code{.size} directive." > > Except that to me "must begin" talks about a source file, not individual > functions. Hence that wording (which I saw) is at bets ambiguous, which > led me to ask for it to be "more precise". > I have updated both code comments and gas/doc/as.texi. Code comments: "When using SCFI, .type directive indicates start of a new FDE for SCFI processing. So, we must first demarcate the previous block of ginsns, if any, to mark the end of a previous FDE." gas/doc/as.texi: " Each input function in assembly must begin with the @code{.type} directive, and should ideally be closed off using a @code{.size} directive. When using SCFI, each @code{.type} directive prompts GAS to start a new FDE (a.k.a., Function Descriptor Entry). This implies that with each @code{.type} directive, a previous block of instructions, if any, is finalised as a distinct FDE." >>>> +/* Check whether a '66H' prefix accompanies the instruction. >>> >>> With APX 16-bit operand size isn't necessarily represented by a 66h >>> prefix, but perhaps with an "embedded prefix" inside the EVEX one. >>> Therefore both the comment and even more so ... >>> >>>> + The current users of this API are in the handlers for PUSH, POP >>>> + instructions. These instructions affect the stack pointer implicitly: the >>>> + operand size (16, 32, or 64 bits) determines the amount by which the stack >>>> + pointer is decremented (2, 4 or 8). When '66H' prefix is present, the >>>> + instruction has a 16-bit operand. */ >>>> + >>>> +static bool >>>> +ginsn_prefix_66H_p (i386_insn insn) >>> >>> ... the function name would better not allude to just the legacy >>> encoding. Maybe ginsn_opsize_prefix_p()? >>> >> >> Isnt 66H_p more readable and easier to follow because that's what the >> function is currently checking ? If more scenarios were being handled, >> ginsn_opsize_prefix_p () would fit better. > > Well, as said - with APX you can't get away with just 0x66 prefix checking. > That prefix is simply illegal to use with EVEX-encoded insns. > I am using the following in ginsn_opsize_prefix_p (): !(i.prefix[REX_PREFIX] & REX_W) && i.prefix[DATA_PREFIX] == 0x66 >>>> +/* Get the DWARF register number for the given register entry. >>>> + For specific byte/word register accesses like al, cl, ah, ch, r8dyte etc., >>> >>> What's "r8dyte"? I expect it's a typo, but I can't derive what was >>> intended to be written. >> >> Typo it is. I meant to write r8d. I have updated this to " like al, cl, >> ah, ch, r8d, r20w etc." > > Then perhaps further s;byte/word;byte/word/dword; ? > Yes. Done. >>>> +static unsigned int >>>> +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 = ireg; >>>> + >>>> + /* ginsn creation is available for AMD64 abi only ATM. Other flag_code >>>> + are not expected. */ >>>> + gas_assert (flag_code == CODE_64BIT); >>> >>> With this assertion it is kind of odd to see a further use of flag_code >>> below. >> >> I think you are referring to the checks like: >> >> /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. */ >> if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) >> stack_opnd_size = 2; >> >> Although the check on flag_code is redundant now, I chose to have them >> here to keep it aligned to how the prefix is meant to be used. > > Yet the same is true in 32-bit mode (minus, of course, the REX.W aspect > mentioned elsewhere, but that could be ignored here as 32-bit code has > no way of setting that flag). IOW - either you drop the use of flag_code, > or you make it actually correct (as in: not misleading). > Dropped the use of flag_code in these instances for V5. >>>> + /* op %reg, symbol. */ >>>> + if (i.mem_operands == 1 && !i.base_reg && !i.index_reg) >>>> + return ginsn; >>> >>> Why does this need special treatment, and why is returning NULL here >>> okay? >> >> An instruction like "addq %rax, symbol" etc are uninteresting for >> SCFI. One of feedback in a previous iteration was to "consider not >> generating ginsns for cases that are known to be uninteresting". > > But then why not simply > > if (i.mem_operands) > return ginsn; > > ? What memory is added to doesn't matter for SFCI, does it? Aiui you > only really want to notice adds to registers. > Correct. Updated the code with additional comments. >>>> +static ginsnS * >>>> +x86_ginsn_addsub_mem_reg (const symbolS *insn_end_sym) >>>> +{ >>>> + unsigned int dw2_regnum; >>>> + unsigned int src2_dw2_regnum; >>>> + const reg_entry *mem_reg; >>>> + int32_t gdisp = 0; >>>> + ginsnS *ginsn = NULL; >>>> + ginsnS * (*ginsn_func) (const symbolS *, bool, >>>> + enum ginsn_src_type, unsigned int, offsetT, >>>> + enum ginsn_src_type, unsigned int, offsetT, >>>> + enum ginsn_dst_type, unsigned int, offsetT); >>>> + uint16_t opcode = i.tm.base_opcode; >>>> + >>>> + gas_assert (opcode == 0x3 || opcode == 0x2b); >>>> + ginsn_func = (opcode == 0x3) ? ginsn_new_add : ginsn_new_sub; >>>> + >>>> + /* op symbol, %reg. */ >>>> + if (i.mem_operands && !i.base_reg && !i.index_reg) >>>> + return ginsn; >>>> + /* op mem, %reg. */ >>> >>> /* op reg/mem, reg. */ you mean? Which then raises the question ... >>> >> >> Yes (updated the comment for V5). >> >>>> + dw2_regnum = ginsn_dw2_regnum (i.op[1].regs); >>>> + >>>> + if (i.mem_operands) >>>> + { >>>> + mem_reg = (i.base_reg) ? i.base_reg : i.index_reg; >>>> + src2_dw2_regnum = ginsn_dw2_regnum (mem_reg); >>>> + if (i.disp_operands == 1) >>>> + gdisp = i.op[0].disps->X_add_number; >>>> + ginsn = ginsn_func (insn_end_sym, true, >>>> + GINSN_SRC_INDIRECT, src2_dw2_regnum, gdisp, >>>> + GINSN_SRC_REG, dw2_regnum, 0, >>>> + GINSN_DST_REG, dw2_regnum, 0); >>>> + ginsn_set_where (ginsn); >>>> + } >>>> + >>>> + return ginsn; >>>> +} >>> >>> ... why a register source isn't dealt with here. >> >> I saw that the opcode used when the source is reg is either 0x1 or 0x29, >> hence concluded that the handling in x86_ginsn_addsub_reg_mem should >> suffice. Perhaps this is a GAS implementation artifact, and I should >> have handling for register source here in x86_ginsn_addsub_mem_reg (for >> opcodes 0x3 and 0x2b) ? > > I think you should, yes. Try using the {load} / {store} pseudo-prefixes, > and I think you'll see these opcodes used. > I see them now with the suggested pseudo prefixes. Added handling in the appropriate function and an additional instruction in ginsn-add-1 testcase. >>>> +static ginsnS * >>>> +x86_ginsn_move (const symbolS *insn_end_sym) >>>> +{ >>>> + ginsnS *ginsn; >>>> + unsigned int dst_reg; >>>> + unsigned int src_reg; >>>> + offsetT src_disp = 0; >>>> + offsetT dst_disp = 0; >>>> + const reg_entry *dst = NULL; >>>> + const reg_entry *src = NULL; >>>> + uint16_t opcode = i.tm.base_opcode; >>>> + enum ginsn_src_type src_type = GINSN_SRC_REG; >>>> + enum ginsn_dst_type dst_type = GINSN_DST_REG; >>>> + >>>> + if (opcode == 0x8b || opcode == 0x8a) >>> >>> Above when handling ALU insns you didn't care about byte ops - why do >>> you do so here (by checking for 0x8a, and 0x88 below)? >> >> Because there may be weird code that saves and restores 8-byte registers >> on stack. For ALU insns, if the destination is REG_SP/REG_FP, we will >> detect them in the unhandled track. > > You talk about 8-byte registers when I asked about byte reg moves. If > there's a MOV targeting %spl or %bpl, is that really any different from, > say, an ADD doing so? > ATM, Yes. SCFI has heuristics implemented, _some_ of which (can be evolved) are opcode specific. E.g., - MOV %rsp, %rbp will make SCFI machinery check if this is making the CFA switch to %rbp. If the target was %bpl, since we track 8-byte registers, it will still trigger the same code path. A bug as I ack below. - ADD rsp + 0 = rbp will not trigger CFA switch to RBP. Should it ? Perhaps yes? Its on my TODO list (with low priority) to evolve this bit. >>>> + } >>>> + >>>> + ginsn_set_where (ginsn); >>>> + >>>> + return ginsn; >>>> +} >>> >>> Throughout this function (and perhaps others as well), how come you >>> don't consider operand size at all? It matters whether results are >>> 64-bit, 32-bit, or 16-bit, after all. >> >> Operation size matters: No, not for all instructions in context of SCFI. >> For instructions using stack (save / restore), the size is important. >> But for other instructions, operation size will not affect SCFI correctness. > > But aren't you wrongly treating an update of %rbp and an update of, > say, %bp the same then? The latter should end up as untracable, aiui. > For ALU ops: - ADD/SUB reg1, reg2 If reg2 is the same as REG_CFA, then even in 64-bit mode, this causes untraceability. So there is untraceability irrespective of operation size. On the other hand, if uninteresting, it will remain unintersting irrespective of operation size. - ADD/SUB imm, reg will never trigger untraceability, irrespective of size. There is the element of ignoring the carry bit, but I think the current diagnostics around "asymmetrical save/restore" and the planned "unbalanced stack at return" should provide user with some safety net. - Other ALU ops should all trigger untracebility alike, irrespective of operation size. Hence, my statement that ignoring operation size is fine here for SCFI. For MOV ops: - 8-bit/16-bit MOV should trigger untracebility. I ack this as bug to be fixed. Thanks to your careful review. ATM, I plan to deal with this after the series goes in, unless you have strong opinion about this. >>>> +static int >>>> +x86_ginsn_unhandled (void) >>>> +{ >>>> + int err = X86_GINSN_UNHANDLED_NONE; >>>> + const reg_entry *reg_op; >>>> + unsigned int dw2_regnum; >>>> + >>>> + /* Keep an eye out for instructions affecting control flow. */ >>>> + if (i.tm.opcode_modifier.jump) >>>> + err = X86_GINSN_UNHANDLED_CFG; >>>> + /* Also, for any instructions involving an implicit update to the stack >>>> + pointer. */ >>>> + else if (i.tm.opcode_modifier.implicitstackop) >>>> + err = X86_GINSN_UNHANDLED_STACKOP; >>>> + /* Finally, also check if the missed instructions are affecting REG_SP or >>>> + REG_FP. The destination operand is the last at all stages of assembly >>>> + (due to following AT&T syntax layout in the internal representation). In >>>> + case of Intel syntax input, this still remains true as swap_operands () >>>> + is done by now. >>>> + PS: These checks do not involve index / base reg, as indirect memory >>>> + accesses via REG_SP or REG_FP do not affect SCFI correctness. >>>> + (Also note these instructions are candidates for other ginsn generation >>>> + modes in future. TBD_GINSN_GEN_NOT_SCFI.) */ >>>> + else if (i.operands && i.reg_operands >>>> + && !(i.flags[i.operands - 1] & Operand_Mem)) >>>> + { >>>> + reg_op = i.op[i.operands - 1].regs; >>>> + if (reg_op) >>>> + { >>>> + dw2_regnum = ginsn_dw2_regnum (reg_op); >>>> + if (dw2_regnum == REG_SP || dw2_regnum == REG_FP) >>>> + err = X86_GINSN_UNHANDLED_DEST_REG; >>>> + } >>> >>> else >>> err = X86_GINSN_UNHANDLED_CONFUSED; >>> >>> ? You can't let this case go silently. An alternative would be to >>> assert instead of using if(). >> >> No, the other cases are not important for SCFI correctness. The case of >> potential register save/restore operation cannot be detected >> automatically. Keeping an eye on ISA additions will be the only way for >> that category. > > That wasn't my point. reg_op turning out to be NULL is bogus considering > the earlier checks. Hence why an assertion may make sense, and hence why > otherwise I suggested an error indicator towards "internal error". > Sorry, I misunderstood. I added a new X86_GINSN_UNHANDLED_UNEXPECTED now for V5. >>>> +/* Generate one or more generic GAS instructions, a.k.a, ginsns for the current >>>> + machine instruction. >>>> + >>>> + Returns the head of linked list of ginsn(s) added, if success; Returns NULL >>>> + if failure. >>>> + >>>> + The input ginsn_gen_mode GMODE determines the set of minimal necessary >>>> + ginsns necessary for correctness of any passes applicable for that mode. >>>> + For supporting the GINSN_GEN_SCFI generation mode, following is the list of >>>> + machine instructions that must be translated into the corresponding ginsns >>>> + to ensure correctness of SCFI: >>>> + - All instructions affecting the two registers that could potentially >>>> + be used as the base register for CFA tracking. For SCFI, the base >>>> + register for CFA tracking is limited to REG_SP and REG_FP only for >>>> + now. >>>> + - All change of flow instructions: conditional and unconditional branches, >>>> + call and return from functions. >>>> + - All instructions that can potentially be a register save / restore >>>> + operation. >>> >>> This could do with being more fine grained, as "potentially" is pretty vague, >>> and (as per earlier version review comments) my take on this is a much wider >>> set than yours. >> >> I would like to understand more on this comment, especially the "my take >> on this is a much wider set than yours". I see its being hinted at in >> different flavors in the current review. >> >> I see some issues pointed out in this review (addressing modes of mov >> etc, safe to skip opcodes for TEST, CMP) etc., but it seems that your >> concerns are wider than this. > > I earlier version review I mentioned that even vector or mask registers > could in principle be use to hold preserved GPR values. I seem to recall > that you said you wouldn't want to deal with such. Hence my use of > "wider set": Just to give an example, "kmovq %rbp, %k0" plus later > "kmovq %k0, %rbp" is a pair of "instructions that can potentially be a > register save / restore operation". > Hmm. I will need to understand them on a case to case basis. For the case of "kmovq %rbp, %k0" / "kmovq %k0, %rbp" how can this be used as save/restore to/from stack ? >>>> + case 0xa0: >>>> + case 0xa8: >>>> + /* push fs / push gs have opcode_space == SPACE_0F. */ >>>> + if (i.tm.opcode_space != SPACE_0F) >>>> + break; >>>> + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); >>>> + /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. */ >>>> + if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) >>>> + stack_opnd_size = 2; >>> >>> But only if there's not also REX.W / REX2.W. >> >> This needs to be done for both push and pop then. I am not sure about >> the REX2.W check though. > > It's not just PUSH/POP, but all stack operations. > Sorry I was imprecise. Now checking REX.W in ginsn_opsize_prefix_p; for all stack related instructions. >> Something like >> !is_apx_rex2_encoding () && !(i.prefix[REX_PREFIX] & REX_W) >> covers it ? > > I don't see why you'd have is_apx_rex2_encoding() here. I don't recall > where/when exactly you invoke your code. It may therefore be either, as > you have it, i.prefix[REX_PREFIX], or (before establish_rex()) it could > (additionally) be i.rex. > This is done after output_insn () (hence after establish_rex ()). I am now using the following in ginsn_opsize_prefix_p (): !(i.prefix[REX_PREFIX] & REX_W) && i.prefix[DATA_PREFIX] == 0x66 >>>> + /* push fs / push gs. */ >>>> + ginsn = ginsn_new_sub (insn_end_sym, false, >>>> + GINSN_SRC_REG, REG_SP, 0, >>>> + GINSN_SRC_IMM, 0, stack_opnd_size, >>>> + 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: >>>> + /* pop fs / pop gs have opcode_space == SPACE_0F. */ >>>> + if (i.tm.opcode_space != SPACE_0F) >>>> + break; >>>> + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); >>>> + /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. */ >>>> + if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) >>>> + stack_opnd_size = 2; >>>> + /* 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, stack_opnd_size, >>>> + GINSN_DST_REG, REG_SP, 0); >>>> + ginsn_set_where (ginsn_next); >>>> + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); >>>> + break; >>>> + >>>> + case 0x50 ... 0x57: >>>> + if (i.tm.opcode_space != SPACE_BASE) >>>> + break; >>>> + /* push reg. */ >>>> + dw2_regnum = ginsn_dw2_regnum (i.op[0].regs); >>>> + /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. */ >>>> + if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) >>>> + stack_opnd_size = 2; >>>> + ginsn = ginsn_new_sub (insn_end_sym, false, >>>> + GINSN_SRC_REG, REG_SP, 0, >>>> + GINSN_SRC_IMM, 0, stack_opnd_size, >>>> + 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); >>>> + /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. */ >>>> + if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) >>>> + stack_opnd_size = 2; >>>> + ginsn_next = ginsn_new_add (insn_end_sym, false, >>>> + GINSN_SRC_REG, REG_SP, 0, >>>> + GINSN_SRC_IMM, 0, stack_opnd_size, >>>> + GINSN_DST_REG, REG_SP, 0); >>>> + ginsn_set_where (ginsn_next); >>>> + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); >>>> + break; >>>> + >>>> + case 0x6a: >>>> + case 0x68: >>>> + if (i.tm.opcode_space != SPACE_BASE) >>>> + break; >>>> + /* push imm8/imm16/imm32. */ >>>> + if (opcode == 0x6a) >>>> + stack_opnd_size = 1; >>>> + /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. >>>> + However, this prefix may only be present when opcode is 0x68. */ >>> >>> Why would this be limited to opcode 0x68? >> >> My understanding from the manual is that 0x6a will always be push imm8. >> Hence, 66H is expected only with 0x68. Isnt this incorrect ? > > The opcode byte determines merely the size of the immediate field. > Operand size determines to which size the signed 8-bit field would be > extended and then pushed onto the stack. There's never a single byte > that would be pushed. > This is now fixed for V5. >>>> + 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, stack_opnd_size, >>>> + GINSN_DST_REG, REG_SP, 0); >>>> + ginsn_set_where (ginsn_next); >>>> + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); >>>> + break; >>>> + >>>> + case 0xff: >>>> + if (i.tm.opcode_space != SPACE_BASE) >>>> + break; >>>> + /* push from mem. */ >>>> + if (i.tm.extension_opcode == 6) >>>> + { >>>> + /* In 64-bit mode, presence of 66H prefix indicates 16-bit op. */ >>>> + if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) >>>> + stack_opnd_size = 2; >>>> + ginsn = ginsn_new_sub (insn_end_sym, false, >>>> + GINSN_SRC_REG, REG_SP, 0, >>>> + GINSN_SRC_IMM, 0, stack_opnd_size, >>>> + 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); >>> >>> What if both base and index are in use? Like for PUSH/POP, all addressing >>> forms are permitted here and ... >>> >>>> + } >>>> + 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 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); >>>> + } >>> >>> ... here. >> >> For the indirect jump and call instructions, the target destination is >> not necessary to be known. Indirect jumps will cause SCFI to error out >> as control flow cannot be constructed. >> >> Call instructions are assumed to transfer control out of function. >> >> In other words, more information in these cases will not be of use to SCFI. > > But then aren't you already doing too much work here? With what you say, > you don't care about the kind of operand at all, merely the fact it's a > CALL might be interesting. Albeit even there I'd then wonder why - if > the function called isn't of interest, how is CALL different from just > NOP? The only CALL you'd need to be concerned about would be the direct > form with a displacement of 0, as indicated elsewhere. But of course > tricky code might also use variants of that; see e.g. the retpoline code > that was introduced into kernels a couple of years back. Recognizing > and bailing on such tricky code may be desirable, if not necessary. > Tracking operands for CALL instructions does not provide value ATM. We do not even split a BB if there is a CALL instruction (the assumption is that CALL insn transfers control out of function). You're right that we need to treat some CALLs differently (because of its affect of control flow and SCFI correctness). RE: doing more work. Sure, but the vision for ginsn was to allow a representation where other analyses may be added. The representation of ginsn will need revisions to get there, but keeping an explicit GINSN_TYPE_CALL seems good. >>>> + } >>>> + break; >>>> + >>>> + case 0xc2: >>>> + case 0xc3: >>>> + if (i.tm.opcode_space != SPACE_BASE) >>>> + break; >>>> + /* Near ret. */ >>>> + ginsn = ginsn_new_return (insn_end_sym, true); >>>> + ginsn_set_where (ginsn); >>>> + break; >>> >>> No tracking of the stack pointer adjustment? >> >> No stack unwind information for a function is relevant after the >> function has returned. So, tracking of stack pointer adjustment by >> return is not necessary. > > What information does the "return" insn then carry, beyond it being > an unconditional branch (which you have a different insn for)? > "return" does not carry any more information than just the GINSN_TYPE_RETURN as ginsn->type. So then why support both "return" and an unconditional branch: The intention is to carry the semantic difference between ret and unconditional jump. Unconditional jumps may be to a label within function, and in those cases, we use it for some validation and BB linking when creating CFG. Return, OTOH, always indicates exit from function. For SCFI purposes, above is the one use. Future analyses may find other use-cases for an explicit return ginsn. But IMO, keeping GINSN_TYPE_RETURN as an explicit insn makes the overall offering cleaner. >>>> @@ -5830,6 +6804,14 @@ md_assemble (char *line) >>>> /* We are ready to output the insn. */ >>>> output_insn (last_insn); >>>> >>>> + /* PS: SCFI is enabled only for AMD64 ABI. The ABI check has been >>>> + performed in i386_target_format. */ >>> >>> See my earlier comment - it's yet more restrictive (as in not covering >>> e.g. the Windows ABI, which importantly is also used in EFI). >> >> Not clear, can you please elaborate ? > > Hmm, it's not clear to me what's not clear in my earlier comment. The > set of preserved registers, for example, differs between the SysV and > the Windows ABIs (see x86_scfi_callee_saved_p()). Then again, thinking > of it, talking of anything ABI-ish in assembly code is questionable. > An assembly function calling another assembly function may use an > entirely custom "ABI". You just cannot guess upon preserved registers. > I think the confusion is stemming from my usage of AMD64 colloquially to refer to System V ABI for x86_64. I think "System V AMD64 ABI" is the correct term. I will use that. And yes, GAS can only work out SCFI if there is ABI adherence. If input asm does not adhere to the supported ABIs, and the user invokes SCFI, then the user is on their own. GAS will not (rather can not) warn about ABI incompliance. >>>> @@ -12104,6 +13087,14 @@ s_insn (int dummy ATTRIBUTE_UNUSED) >>>> last_insn->name = ".insn directive"; >>>> last_insn->file = as_where (&last_insn->line); >>>> >>>> + /* PS: SCFI is enabled only for AMD64 ABI. The ABI check has been >>>> + performed in i386_target_format. */ >>>> + if (flag_synth_cfi) >>>> + { >>>> + ginsn = x86_ginsn_new (symbol_temp_new_now (), frch_ginsn_gen_mode ()); >>> >>> If you really want to use this function here, more cases will need >>> handling (perhaps even beyond what I've commented on above). However, >>> I'd strongly suggest splitting off the "unhandled" part of that >>> function, and using only that here. After all you hardly know what >>> exactly the programmer's intentions are. Because of that, you may >>> also want to consider simply forbidding use of .insn when SCFI is to >>> be generated. >> >> Sorry, not clear to me. I am not sure how moving simply the "unhandled" >> part of x86_ginsn_new will work better. If we dont handle the >> instructions, more instructions will potentially trigger the "unhandled" >> case. > > Correct. You simply shouldn't try to interpret what the user encoded > via .insn. Much like gcc doesn't try to interpret what the string > literal of an asm() contains. > Removed the call to x86_ginsn_new () in s_insn () and replaced by an error if SCFI is in effect. >>>> @@ -15748,6 +16739,11 @@ i386_target_format (void) >>>> else >>>> as_fatal (_("unknown architecture")); >>>> >>>> +#if defined (OBJ_ELF) || defined (OBJ_MAYBE_ELF) >>>> + if (flag_synth_cfi && x86_elf_abi != X86_64_ABI) >>>> + as_fatal (_("Synthesizing CFI is not supported for this ABI")); >>>> +#endif >>> >>> Elsewhere I've raised the question of whether we should really check >>> OBJ_MAYBE_ELF anywhere in this file. However, as long as we do, you'll >>> need to accompany that with an IS_ELF check in the if(). If, to >>> address my unreachable code comment near the top, you elect to add >>> further OBJ_ELF / OBJ_MAYBE_ELF checks, there you then wouldn't need >>> that further check (as flag_synth_cfi set then implies ELF). >> >> I took the suggestion to not compile unnecessarily for non-ELF targets. >> >> Use IS_ELF in if (): But flag_synth_cfi does not imply IS_ELF, it >> implies OBJ_ELF || OBJ_MAYBE_ELF. Looks to me then, that I should >> include IS_ELF check in each of the 3 blocks. > > Why? If you use IS_ELF here, you won't make it there when IS_ELF > returned "false", simply because you use as_fatal() here. It is > once you pass the check here that flag_synth_cfi implies IS_ELF > (as much as it then also implies x86_elf_abi == X86_64_ABI). > Got it now.