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 04F95385840F for ; Mon, 8 Jan 2024 19:33:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 04F95385840F 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 04F95385840F 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=1704742415; cv=pass; b=oyI5FEBvxBVQOzmzCQNsp/3H4yxRvecr3ij2JePCd5mFzWneczsO35IsFQ7HvouXq1e65G9vIaJvLcBItU8IJlL5FrXfpU1LxdKMZlVUn2LsVOncGXkOrBFLL3psG1WSl9D9bkJ4gMTsggm3hWR4u46QZboUjynZM7263EvpqIk= ARC-Message-Signature: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1704742415; c=relaxed/simple; bh=TJTPj9Peq3FBDjtfJu7f8rfl0yXfrEZ2moc4BzpLlY4=; h=DKIM-Signature:DKIM-Signature:Message-ID:Date:From:Subject:To: MIME-Version; b=vVMuCJGdOCyKhI69iqDUlzZNvWJXr4/onBtmjtDd8oOKLDEz4EkiWo+/6k63GBZJQ3tdapLtd5EIOj1P0aS2NqW+TCV55lPdKRsgmePFB9aYlKLdlZhDs9RgSi8Z+IsQ7tGD946WhzFszKnuRgDnqu8osXgAUXfUzReWdfW3P3Q= ARC-Authentication-Results: i=2; server2.sourceware.org Received: from pps.filterd (m0246617.ppops.net [127.0.0.1]) by mx0b-00069f02.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 408JU07W000393; Mon, 8 Jan 2024 19:33:28 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-11-20; bh=vLENeCpsNEHz1apGcW7LxBao+Te7oliGjHRyncyr1OU=; b=CXDydWw9by83ozIGkv7vy4nFkMHAFkbVmP9BfS/Wb8lVl7k5qqypjpwZx1+UOOkZPlBD xnlfzna0udaPGGon9PjSkZqcyjjPdFr2dxTSYgF+bmP5uaHVkS8nC/K7HwNeyVs9juyD 8Vs/eSugJAiZSmH77vqmzEqYhf9G4lVAmpNvgfarigpdBbYNp8W4eXufvoppiujwGtox 4Zve5nCnEBcjPuwIIpsm9NLAnoAnqUzpPF2zZ1/5cknzgssdodk4MmsDo/n/2pJ6AigS ehM92YCVhPy8FC+0hAMQCRYOGOJVAapDUo0Jk1JXuS2BKw6obJcVW7Vlwp23YHBhSHBS 4w== Received: from iadpaimrmta01.imrmtpd1.prodappiadaev1.oraclevcn.com (iadpaimrmta01.appoci.oracle.com [130.35.100.223]) by mx0b-00069f02.pphosted.com (PPS) with ESMTPS id 3vgq8ng0a1-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 08 Jan 2024 19:33:27 +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 408JMtg6035025; Mon, 8 Jan 2024 19:33:25 GMT Received: from nam04-mw2-obe.outbound.protection.outlook.com (mail-mw2nam04lp2169.outbound.protection.outlook.com [104.47.73.169]) by iadpaimrmta01.imrmtpd1.prodappiadaev1.oraclevcn.com (PPS) with ESMTPS id 3vfuu3byp5-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 08 Jan 2024 19:33:24 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=mmENZK6dfwKEqeC0eYONy3KDzRktEzsFldpETRdVYy525QJ4cGx2uDKyPtYHThjLRhUv5CYI5qJ4r+CdAT3zK3VYRYu+s647czYym7WD7Hern5MbpDbkjWX2GYOSsGxWeZgfXAu4vGLCXpzoPBrxvPOcmLeXVWp78W+QJJ8XVfbPhlzEiNWC5Tkg2M2zxFvrxvZ8hw2aGSFiwNnIUCjWkLaivvDTTOGVsVVLF4WDoZIP+IkqTs0MCxBlYc84CnzrX4pr9i+vYCH0+pxsrC1k0dpgIAaX/uailNc3VyTxZc8xIOe/bZrlolkJls144CxwmVq3MVAukv7X6ORHpS96LA== 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=vLENeCpsNEHz1apGcW7LxBao+Te7oliGjHRyncyr1OU=; b=gs/LLn55QDT0yt46W2fHMCNO1xlIuKq0Rk1cPOmClDsb26bKKW8VJjP3RfuOL+l22vQM3JthpbZpc0oi9KEclhgXjZi1RoTOk9AFEtJx08Wwjw/wz8TUXCdvaBZOTqUnFfRORY6Vbnp4+HC7D9KCyO0BmswfpbLhvYK32Peh48o2rWmyAsNv+AlD2G8C9G2PKRX9+yN1X+juyjgeyeE4Hx078IdjnRrkDhyTbvL7CPYvNwxO1u5D4BFpO/+edMlPmjzhszSnvt2X2aF1/5KmPjvW0poek7ypDPsR4RqYiiDhMxbdBqwfzG1DF0fpKFwTTsyDuZViG5kObo9gTdLf7Q== 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=vLENeCpsNEHz1apGcW7LxBao+Te7oliGjHRyncyr1OU=; b=DL/1LQ/O/v+hEoWT8QMP6MnSyteQUiENWJVT7CPpvHENW9sz1Qa8R0GgkxM11wBV8kaXdm6r4IgDX530Slw3XcoNFLkEAIVNfABR7vQFqF4KbkLbljiHhB+E+lKXYfxuUPcUkD17vKuIHHH+4FUOdiSu5tJ165KodSK8wZJG30Y= Received: from MWHPR1001MB2158.namprd10.prod.outlook.com (2603:10b6:301:2d::17) by MW4PR10MB6677.namprd10.prod.outlook.com (2603:10b6:303:22f::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7159.21; Mon, 8 Jan 2024 19:33:20 +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; Mon, 8 Jan 2024 19:33:20 +0000 Message-ID: Date: Mon, 8 Jan 2024 11:33:19 -0800 User-Agent: Mozilla Thunderbird From: Indu Bhagat Subject: Re: [PATCH,V4 10/14] gas: synthesize CFI for hand-written asm 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> Content-Language: en-US In-Reply-To: <0ecd9240-0700-4072-91d4-ccf9bdb56071@suse.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: MW4P222CA0022.NAMP222.PROD.OUTLOOK.COM (2603:10b6:303:114::27) To MWHPR1001MB2158.namprd10.prod.outlook.com (2603:10b6:301:2d::17) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: MWHPR1001MB2158:EE_|MW4PR10MB6677:EE_ X-MS-Office365-Filtering-Correlation-Id: 8fd4841f-cece-4665-b1c7-08dc1080abc2 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: GSFb3ILzJghGOPGX/Jk6GUNunL4AT0CV3eHOUzaZ1CTVPYPACXegyjVcgXiW+kxOacQOjWua1biLIAjWwEV8guRveNp0Isbyk6DHc9UAvm3faTgGCIpUAkTihVZ/ihaCePCKwg3BP95zNlwDLsOijkwL30CicO1sA2uo8GpKcNk2R57Ao3goAJtTtI8tSTUKniWkaYuxojeACU/H9PUPkGaAbzEftDYzGQo5uG9z0lhtAJYrtBTIwXVuB/gMtv3GNNVTHCpzPpqfSeqMr177+Ou4BEvNV5xElq13/1LunPlbYoWB209ZMGQ3OlC90XsaxUdZg1QEnq24KTVRUyJUr+a9E7qXzP2nEbGW4oHc4OasRP7gzyJoAA1wR/Ic3BVowDUjnQ/x5pm8vSqa3bln3lqoML4746GdnnmTv50OTBLZuU6UvCp5pG4wV9D8rFpPfjWp3+jl2xcLbzyzlhfbWKC1gx1mYLmdomVG1KlIHPdU4X9obp4znqm8J3PzPBMZaANy84rd8X/af9WlBq9rJOaEI74HQ2lWA/DP06iKXgEj/IZ8q9BpZLuN4b8ke5Bqth6jBS3hQwSh+JklkRZbFRzuj4NehNYDP2Hi/5Xy+yC2V46cuSfJTDnM10utcVh4Ez16ra75SnQlQy3j0EFiHw== 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)(366004)(346002)(136003)(396003)(376002)(39860400002)(230922051799003)(64100799003)(186009)(451199024)(1800799012)(478600001)(66899024)(8936002)(5660300002)(8676002)(4326008)(36756003)(30864003)(44832011)(2906002)(316002)(66556008)(66476007)(6916009)(66946007)(6486002)(86362001)(31696002)(38100700002)(6512007)(2616005)(53546011)(83380400001)(41300700001)(6506007)(31686004)(43740500002)(45980500001)(559001)(579004);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WlVyemNyMTBkd0xiMWpJVmpzQzJRQkMwbzMxMEw5UlVBSnhZUmoydU5DRWdV?= =?utf-8?B?akUxZTlKVENxSytaV3VnNFg2VkRjVSsrVHFJSjRvYkFoeEFCNlFmaGZPUHFa?= =?utf-8?B?MGI0R2JPTFdKU2tYVGVZM1lWYitsZndyVENLY0VPMGpUZ3RHYkMzUW5zUHhs?= =?utf-8?B?cVh3clpiQkVMc3FkcnhMS0NjRDBuQkViYTBiREFObitYbGpwRHRFUCtRbFps?= =?utf-8?B?RWtRMUl2andjckI1RHlPQVJtNXJBUmFNQS9yQlFkWUpldmpvUnhhd3lPZDZB?= =?utf-8?B?c0hVd1FkTEw3QjFzU1RmMkhBV0NZbS9oMkw1S0xRTDlPWXJuMkdUcVhIWi9Q?= =?utf-8?B?WkJEWDR2TWdzUEhBR05rVFFxN2FVNDE5bmpvZ2kvMnNsdmJDcS8vTUVtdjFH?= =?utf-8?B?RHlMMmdXZUhzZGw2NUFmaWtJVXNuY1FOQWV0aUlTL3lJWnU4czZpQWh6OGxl?= =?utf-8?B?M09RaFF2aFdyNEdYQ3kxUXlJY2RkRFA0TkRiaWE0aFhRcFMzaDByMDkvKzNa?= =?utf-8?B?aHdHcW53RGMwY2JPU2J5NEZabDgzYTdLV3dxQ0xkNUFTRGdCcWZPWGpWeFFv?= =?utf-8?B?d3RLbkkzYUI0blpLSk9mZlFXcHZKVTdUQ1dhK0tiV2V3WCtXUGoyeCt5cG5I?= =?utf-8?B?WCtmSXJRTWE3TWNDVVhOajZHTXZINlArRytvaGo2UzB5MjVTTkRUVTF2dU1U?= =?utf-8?B?ZTNoaHUwbFJEemZzbXpINDh3QzNiYUxkUkF0TFNwMEJ0Q2JKY0ZDTVB5dlNx?= =?utf-8?B?WFUxRjRpVVFYek9aRUhXbEdUcXVqdGJzY3RzNnd6NXRUYngwRHdDZUFiemlS?= =?utf-8?B?Z1I3SndqRjVQZlpSVlRVOWN1VmdyUS93cjdTSTR0UU9wZ1NXZnVocWtxL0dq?= =?utf-8?B?RDQ4V0cxaS9CVHJkYmw4NnkxOCtMK0tVeE9OU2ZxVEtIV1VsanAyeWZDTWpL?= =?utf-8?B?UkVMbjRURngza2lYbFRFZ1M5STREbGc0K3huTm5sWEhoRXRjUDZzM2tnS3B4?= =?utf-8?B?QkNranFYcFA3Mko3cGMzNkoxWWVZTE5UaVF2c0hlaXVDcUJSL2tNUE9lSDFv?= =?utf-8?B?NFVITFBwQmV2SFl0YmJJS0RXcHJOSDhabGF4L2hCRGZ3Nmp5OGNON0lTeVZI?= =?utf-8?B?a3RWZmpxRGhDRUpTcjZkeFhuVUtLUU93Vng5WTdnSmZSTjdhUFJqUmRldlBi?= =?utf-8?B?V3NqOVczTFF3TDNxWVZGN3lLVjhab28zdWRwN29namZTRHBuWXJ2a0FNcjV5?= =?utf-8?B?ZC9vTEY4aDhiRHdGTGVibVNlWkgxMkttSEl2eXdvSmxMM3Q4Q3NRaG9XNE9H?= =?utf-8?B?TmF6OG1COC95NnVoQWd0QmpMOTB2dmFCRG5CdmhlR3AyMldSL0cydDZHR3Er?= =?utf-8?B?MUZOd0ljclJNaEx5a2x5R1crOE9rL2t5WWZCUHZaY1orbnExTFpxaEFWUXRK?= =?utf-8?B?Wjh5UE9Kb1lPQ1lRWCt0SnM1bFJFbjgvSXFGeVNyUDFaSWM5bmEwYi9uUzhV?= =?utf-8?B?Nzd3cGJNdEFBOXVaWkhjeHBGMFRCYlU5NmdzVVdRTkdkVzl3MmhLaHFBb2Nt?= =?utf-8?B?Nis5dmZFM2Fwa0lIQW90NUhyUkZGblduYXpGSXpCWGFSZFluNkhITy96L3p6?= =?utf-8?B?NmUwdHgwUVNFUU0yLzBQNE1NSXNYdzdScU1Iem43citNN25hYWxUc2plY3RX?= =?utf-8?B?eDJtRzVzZXUxUi85VVQvdWVlRzUrZUREK3RKcXpMdWp5R1Z1UlBuZS84am9a?= =?utf-8?B?RnZYKzN1a0NwYmtJemxnSmxGMWFrY0NMaEJVdmNMLzBSczhwaFdCVm5mOFpG?= =?utf-8?B?b0dLUVhQL3A2OXphUnJzejBSeTVwSmE2MW01Y1J2dS9yK29EYkljVG5ra3Vo?= =?utf-8?B?K2w0eWhIRFZSd2dwNDRmSjFsdENCamUrMEJrdU5XeVNVcTFuWlEyTW82RnA5?= =?utf-8?B?blQyejBUYzA5M2dONjlPYmhFNnVscXgrL0tSQkI4T2t4Vy9oN2UxN2tSMGw5?= =?utf-8?B?bUlqWkhEalFzeE9MZzRKNHgvQmVTZVc5QzRncTJyMWkyalVURy9UalR1U1lD?= =?utf-8?B?V2c1bUhURUhRRXBOOGtseWxORVhYUHordDFzYS9adWtGbTJXUFhQQjZWQWRu?= =?utf-8?B?MnI5ei9heFhQK2VGVlR6TUNIR1NzdVB0aktvQ0wreElLejA3VlU5YktjRGVJ?= =?utf-8?Q?nIAjTKFgGjj+u4qZZ1TiUro=3D?= X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: m4+yjhJL8rMHjF5aphGoWvKhSTC1Jhl4YfP5SI5z2Fv5aSyKtlbA/5vSY7nxzOsSPH1alXPB1yX7TCR3SATzcl1hiQuMCiTQury3+dTMNv+qDBo57nWyx5vwaqRIi8H2K6z8yk2LC/ioPOThQEGNVWCYSHmzGTG+nYxT2Cl3gHbDfg7Fzmb1yZOEypUIaBtBhEbdq2NtqojRe6b3sZdRdvHNCn0XRU8MYXghyrWmOZB/CoC+FJ+gCrqkRT4JpiToe2HvP9HNBcqizrtbDovKcqJOgMKF+Pa6fGYRhJVvq++RB+X0IeoGYAGDohMozrJkxCaxLdZnaBs1QHwZTa0SuYk6zi3w2j+esCmnSqPc1E3bxM9ezB1pzIabR3z8R+Mg1JSIIBkRUyycMbw7H6+iPwEs9edgAftw797nZIi5NOazQkcDx+ReiaVQ/FO/g+XBjaOWsE/8uTN+8WJ8p5PAn8iYmMAVQZUH3AqAINmu/vvp6ykTQzCYHpa+XYdSYxD1s2z+1KOwe9PsGTn++ZgWdmnc54LyWnKo+CKAFwqyC0Zagn5hBIQ7ULJtJ//SPYgz9Igc/Ze+Z6KDoB62VmSsGtnjbFP1wt/H+jfLYAl0LJo= X-OriginatorOrg: oracle.com X-MS-Exchange-CrossTenant-Network-Message-Id: 8fd4841f-cece-4665-b1c7-08dc1080abc2 X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2158.namprd10.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jan 2024 19:33:20.4641 (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: g2NOJCQnLgFTmp5iKZLw3rqIfrkFoXlA5q+hEoJEFp6Iz65+7PsAhah80RISMyQMHdaRxaCmYyT7+D1aN5p/eg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR10MB6677 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-08_08,2024-01-08_01,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-2401080162 X-Proofpoint-GUID: FgwggZ6McJG7rI26aWyJriWfCkwXsKMk X-Proofpoint-ORIG-GUID: FgwggZ6McJG7rI26aWyJriWfCkwXsKMk X-Spam-Status: No, score=-6.3 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,SCC_5_SHORT_WORD_LINES,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no 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, On 1/5/24 05:58, Jan Beulich wrote: > On 03.01.2024 08:15, Indu Bhagat wrote: >> --- 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 >> @@ -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." >> --- 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" >> @@ -5287,6 +5288,978 @@ static INLINE bool may_need_pass2 (const insn_template *t) >> && t->base_opcode == 0x63); >> } >> >> +bool >> +x86_scfi_callee_saved_p (unsigned int dw2reg_num) > > Iirc SCFI is ELF-only. We're not in ELF-only code here, though. Non-ELF > gas, as indicated before, would better not carry any unreachable code. > Guarded these APIs with OBJ_ELF || MAYBE_OBJ_ELF for V5. >> +{ >> + 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; >> + >> + return false; >> +} > > This entire function is SysV-ABI-specific without this being made clear > by a comment. > Added a comment. >> +/* 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. >> +{ >> + return (insn.prefix[DATA_PREFIX] == 0x66); >> +} > > I take it that all ginsn / scfi interaction is late enough in the > process for this array element already having been reliably set? > Yes, I think so. All of ginsn creation is after output_insn (). >> +/* 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." >> + there is no valid DWARF register number. This function is a hack - it >> + relies on relative ordering of reg entries in the i386_regtab. FIXME - it >> + will be good to allow a more direct way to get this information. */ > > Saying it's a hack is a helpful sign, but it still would be useful to > also briefly describe what the intentions here are. It's hard to spot > whether the code is correct without knowing what's intended (i.e. how > 8- and 16-bit registers, or non-GPRs, are meant to be treated). > I have updated the function-level description to include: "For specific byte/word register accesses like al, cl, ah, ch, r8d, r20w etc., we need to identify the DWARF register number for the corresponding 8-byte GPR." Also included a test for new GPR r31w in ginsn-dw2-regnum-1 in V5. I realized that the data type for dw2_regnum in reg_entry needs to be (bumped up): signed short dw2_regnum[2]; instead of the current 'signed char' as the new GPRs have DWARF reg numbers > 130. Will include a patch for this. >> +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. >> + if (ireg <= &i386_regtab[3]) >> + /* For al, cl, dl, bl, bump over to axl, cxl, dxl, bxl respectively by >> + adding 8. */ >> + temp = ireg + 8; >> + else if (ireg <= &i386_regtab[7]) >> + /* For ah, ch, dh, bh, bump over to axl, cxl, dxl, bxl respectively by >> + adding 4. */ >> + temp = ireg + 4; > > Assuming this is a frequently executed path, why do these not live ... > >> + dwarf_reg = temp->dw2_regnum[flag_code >> 1]; >> + if (dwarf_reg == Dw2Inval) >> + { > > ... here, thus at least not affecting the code path taken for 64-bit GPRs. > Thanks. I have moved it inside the if () block. >> + /* The code relies on the relative ordering of the reg entries in >> + i386_regtab. The assertion here ensures the code does not recurse >> + indefinitely. */ >> + gas_assert (temp + 16 < &i386_regtab[i386_regtab_size - 1]); > > Afaict this is (still) undefined behavior. You may not add to a pointer > without knowing whether the result still points into or exactly past > the underlying array. > Ah. I have now updated this to: unsigned int idx; gas_assert ((temp - &i386_regtab[0]) >= 0); idx = temp - &i386_regtab[0]; gas_assert (idx + 32 < i386_regtab_size - 1); >> + temp = temp + 16; > > Also - where's the 16 coming from? Was this not updated when rebasing over > APX? > Interesting that the testcase ginsn-dw2-regnum-1 was meant to catch such cases when the function goes out of sync with i386-opc.tbl, but this one was not detected after the addition of GPRs with the APX patch. The correct magic number after the new GPRs were added should be 32. The V4 implementation, however, still reached the correct register entry in two iterations. Anyway, fixed now for V5. >> + dwarf_reg = ginsn_dw2_regnum (temp); >> + } >> + >> + gas_assert (dwarf_reg != Dw2Inval); /* Needs to be addressed. */ > > Without actually addressing this (and possible similar cases elsewhere), I > don't think this can go in as other than experimental code (which the > NEWS entry then should state, and where there then should be a plan for an > easy approach of probing gas for no-longer-experimental SCFI support). > The /* Needs to be addressed. */ comment is imprecise and may have lead the impression that this is an unhandled case. The intention was to catch most bad cases during dev/testing so that in production this serves merely as sanity check here - a failure triggered in rare case of state corruption or bad ginsn etc. I have updated the code comment. I have also updated this assert to now check that (dwarf_reg >= 0). As I comment below, I have included handling for RegIP, RegIZ. >> + return (unsigned int) dwarf_reg; >> +} >> + >> +static ginsnS * >> +x86_ginsn_addsub_reg_mem (const symbolS *insn_end_sym) >> +{ >> + unsigned int dw2_regnum; >> + unsigned int src2_dw2_regnum; >> + 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 == 0x1 || opcode == 0x29); >> + ginsn_func = (opcode == 0x1) ? ginsn_new_add : ginsn_new_sub; > > As mentioned before - checking opcode without also checking opcode > space is pretty meaningless. > The caller of this API screens has checks for i.tm.opcode_space, so this is OK in theory. But I have now also added to the assert above. >> + /* 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". >> + /* op reg, reg/mem. */ >> + 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_func (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); >> + } >> + /* Other cases where destination involves indirect access are unnecessary >> + for SCFI correctness. TBD_GINSN_GEN_NOT_SCFI. */ >> + >> + return ginsn; >> +} >> + >> +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) ? >> +static ginsnS * >> +x86_ginsn_alu_imm (const symbolS *insn_end_sym) >> +{ >> + offsetT src_imm; >> + unsigned int dw2_regnum; >> + ginsnS *ginsn = NULL; >> + enum ginsn_src_type src_type = GINSN_SRC_REG; >> + enum ginsn_dst_type dst_type = GINSN_DST_REG; >> + >> + 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); >> + >> + /* FIXME - create ginsn where dest is REG_SP / REG_FP 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/and imm, %reg only at this time for SCFI. >> + Although all three (and, or , xor) make the destination reg untraceable, > > Why would this also be done for CMP? And neither ADC nor SBB are > mentioned at all in ... > >> + and op is handled but not or/xor because we will look into supporting >> + the DRAP pattern at some point. */ > > ... this entire comment, justifying the choice made. > I will add comment about CMP, ADC, SBB. The choice made here is based on assumption that ADC, SBB with REG_SP / REG_FP are not commonly used for stack manipulation purposes. If at all they do occur, they will be handled in the x86_ginsn_unhandled () track, and correctness is not sacrificed. Note that ADC, SBB with other registers as destination are not interesting for SCFI. I have added an adc op in ginsn-add-1 to keep this tested. >> + if (i.tm.extension_opcode == 5) >> + ginsn_func = ginsn_new_sub; >> + else if (i.tm.extension_opcode == 4) >> + ginsn_func = ginsn_new_and; >> + else if (i.tm.extension_opcode == 0) >> + ginsn_func = ginsn_new_add; >> + else >> + return ginsn; >> + >> + /* 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 (i.imm_operands == 1 >> + && i.op[0].imms->X_op != O_constant) >> + return ginsn; >> + >> + /* addq $1, symbol >> + addq $1, -16(%rbp) >> + Such instructions are not of interest for SCFI. */ >> + if (i.mem_operands == 1) >> + return ginsn; > > Perhaps not just here: TBD_GINSN_GEN_NOT_SCFI? > OK. Added a comment TBD_GINSN_GEN_NOT_SCFI. >> + gas_assert (i.imm_operands == 1); >> + src_imm = i.op[0].imms->X_add_number; >> + /* The second operand may be a register or indirect access. For SCFI, only >> + the case when the second opnd is a register is interesting. Revisit this >> + if generating ginsns for a different gen mode TBD_GINSN_GEN_NOT_SCFI. */ >> + if (i.reg_operands == 1) >> + { >> + dw2_regnum = ginsn_dw2_regnum (i.op[1].regs); >> + /* For ginsn, keep the imm as second src operand. */ >> + ginsn = ginsn_func (insn_end_sym, true, >> + src_type, dw2_regnum, 0, >> + GINSN_SRC_IMM, 0, src_imm, >> + dst_type, dw2_regnum, 0); >> + >> + ginsn_set_where (ginsn); >> + } >> + >> + return ginsn; >> +} >> + >> +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. >> + { >> + /* mov disp(%reg), %reg. */ >> + if (i.mem_operands && i.base_reg) >> + { >> + src = i.base_reg; >> + if (i.disp_operands == 1) >> + src_disp = i.op[0].disps->X_add_number; >> + src_type = GINSN_SRC_INDIRECT; >> + } >> + else >> + src = i.op[0].regs; > > Even when there's no base, the source isn't necessarily a register. > And in such a case using i.op[0].regs isn't valid. > >> + dst = i.op[1].regs; >> + } >> + else if (opcode == 0x89 || opcode == 0x88) >> + { >> + /* mov %reg, disp(%reg). */ >> + src = i.op[0].regs; >> + if (i.mem_operands && i.base_reg) >> + { >> + dst = i.base_reg; >> + if (i.disp_operands == 1) >> + dst_disp = i.op[1].disps->X_add_number; >> + dst_type = GINSN_DST_INDIRECT; >> + } >> + else >> + dst = i.op[1].regs; > > Similarly here then. > Right. Fixed this and above. >> + } >> + >> + 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; >> +} >> + >> +/* Generate appropriate ginsn for lea. >> + Sub-cases marked with TBD_GINSN_INFO_LOSS indicate some loss of information >> + in the ginsn. But these are fine for now for GINSN_GEN_SCFI generation >> + mode. */ >> + >> +static ginsnS * >> +x86_ginsn_lea (const symbolS *insn_end_sym) >> +{ >> + offsetT src_disp = 0; >> + ginsnS *ginsn = NULL; >> + unsigned int base_reg; >> + unsigned int index_reg; >> + offsetT index_scale; >> + unsigned int dst_reg; >> + >> + if (!i.index_reg && !i.base_reg) >> + { >> + /* lea symbol, %rN. */ >> + dst_reg = ginsn_dw2_regnum (i.op[1].regs); >> + /* TBD_GINSN_INFO_LOSS - Skip encoding information about the symbol. */ >> + ginsn = ginsn_new_mov (insn_end_sym, false, >> + GINSN_SRC_IMM, 0xf /* arbitrary const. */, 0, >> + GINSN_DST_REG, dst_reg, 0); >> + } >> + else if (i.base_reg && !i.index_reg) >> + { >> + /* lea -0x2(%base),%dst. */ >> + base_reg = ginsn_dw2_regnum (i.base_reg); > > What if base is %eip? Aiui ginsn_dw2_regnum() will hit an assertion > then. > Yes it will. Handled RegIP, RegIZ now for V5. Thanks. > And what about e.g. "lea symbol(%rbx), %rbp"? The ... > >> + dst_reg = ginsn_dw2_regnum (i.op[1].regs); >> + >> + if (i.disp_operands) >> + src_disp = i.op[0].disps->X_add_number; > > ... constant retrieved here won't properly represent the displacement > then. > >> + 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 (!i.base_reg && i.index_reg) >> + { >> + /* lea (,%index,imm), %dst. */ >> + /* TBD_GINSN_INFO_LOSS - There is no explicit ginsn multiply operation, >> + instead use GINSN_TYPE_OTHER. */ > > You're also losing the displacement here. > Added a comment. >> + index_scale = i.log2_scale_factor; >> + index_reg = ginsn_dw2_regnum (i.index_reg); >> + dst_reg = ginsn_dw2_regnum (i.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); > > Wouldn't it make sense to represent a scale factor of 1 correctly > here (i.e. not as "other", but rather similar to the base-without- > index case above)? > Hmm... your suggestion does make sense. For now, I have added this to my TODO items. >> + } >> + else >> + { >> + /* lea disp(%base,%index,imm) %dst. */ >> + /* TBD_GINSN_INFO_LOSS - Skip adding information about the disp and imm >> + for index reg. */ >> + base_reg = ginsn_dw2_regnum (i.base_reg); >> + index_reg = ginsn_dw2_regnum (i.index_reg); >> + dst_reg = ginsn_dw2_regnum (i.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); > > Seeing the use of "other" above, why is this (wrongly) represented > as "add"? > You have a point. No good reason why this shouldnt be GINSN_TYPE_OTHER. Usage of GINSN_TYPE_ADD is however not wrong though for SCFI purposes: because we know that such an add if later deemed as "interesting for SCFI" purposes, will cause the SCFI generation to bail out with a "unsupported stack manipulation pattern". So will a GINSN_TYPE_OTHER. I have now changed this to use ginsn_new_other though. GINSN_TYPE_OTHER is a better fit. >> + } >> + >> + 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. >> +static ginsnS * >> +x86_ginsn_jump (const symbolS *insn_end_sym) >> +{ >> + ginsnS *ginsn = NULL; >> + symbolS *src_symbol; > > Here and elsewhere - please use const whenever possible. (I think I said > so already on an earlier version.) > Yes. Somehow this was missed. I also noted now that some more symbolS * usages in ginsn_new_jump () / ginsn_new_jump_cond () could use const. I have updated those too. >> + gas_assert (i.disp_operands == 1); >> + >> + /* A non-zero addend in jump target makes control-flow tracking difficult. >> + Skip SCFI for now. */ >> + if (i.op[0].disps->X_op == O_symbol && i.op[0].disps->X_add_number) >> + { >> + as_bad ("SCFI: jmp insn with non-zero addend to sym not supported"); >> + return ginsn; >> + } >> + >> + if (i.op[0].disps->X_op == O_symbol) > > Why the redundant evaluation of ->X_op? In fact, if you moved the > earlier if() ... > >> + { > > ... here, this ... > >> + gas_assert (!i.op[0].disps->X_add_number); > > ... assertion would become entirely redundant. > OK. Rearranged the checks. >> + src_symbol = i.op[0].disps->X_add_symbol; >> + ginsn = ginsn_new_jump (insn_end_sym, true, >> + GINSN_SRC_SYMBOL, 0, src_symbol); >> + >> + ginsn_set_where (ginsn); >> + } >> + >> + return ginsn; >> +} >> + >> +static ginsnS * >> +x86_ginsn_jump_cond (const symbolS *insn_end_sym) >> +{ >> + ginsnS *ginsn = NULL; >> + symbolS *src_symbol; >> + >> + gas_assert (i.disp_operands == 1); >> + >> + /* A non-zero addend in JCC target makes control-flow tracking difficult. >> + Skip SCFI for now. */ >> + if (i.op[0].disps->X_op == O_symbol && i.op[0].disps->X_add_number) >> + { >> + as_bad ("SCFI: jcc insn with non-zero addend to sym not supported"); >> + return ginsn; >> + } >> + >> + if (i.op[0].disps->X_op == O_symbol) >> + { >> + gas_assert (i.op[0].disps->X_add_number == 0); >> + src_symbol = i.op[0].disps->X_add_symbol; >> + ginsn = ginsn_new_jump_cond (insn_end_sym, true, >> + GINSN_SRC_SYMBOL, 0, src_symbol); >> + ginsn_set_where (ginsn); >> + } >> + >> + return ginsn; >> +} > > This looks almost identical to x86_ginsn_jump() - can't the two be > folded? > This was in my TODO items. I have merged the two functions now for V5. >> +static ginsnS * >> +x86_ginsn_enter (const symbolS *insn_end_sym) >> +{ >> + ginsnS *ginsn = NULL; >> + ginsnS *ginsn_next = NULL; >> + ginsnS *ginsn_last = NULL; >> + >> + gas_assert (i.imm_operands == 2); >> + >> + /* For non-zero size operands, bail out as untraceable for SCFI. */ >> + if ((i.op[0].imms->X_op != O_constant || i.op[0].imms->X_add_symbol != 0) >> + || (i.op[1].imms->X_op != O_constant || i.op[1].imms->X_add_symbol != 0)) > > While the comment makes sufficiently clear what's meant, the use of (inner) > parentheses here is still confusing as to whether indeed the || are meant. > >> + { >> + as_bad ("SCFI: enter insn with non-zero operand not supported"); >> + return ginsn; >> + } >> + >> + /* If the nesting level is 0, the processor pushes the frame pointer from >> + the BP/EBP/RBP register onto the stack, copies the current stack >> + pointer from the SP/ESP/RSP register into the BP/EBP/RBP register, and >> + loads the SP/ESP/RSP register with the current stack-pointer value >> + minus the value in the size operand. */ >> + ginsn = ginsn_new_sub (insn_end_sym, false, >> + GINSN_SRC_REG, REG_SP, 0, >> + GINSN_SRC_IMM, 0, 8, > > I guess 8 is the operand size and you simply hope no-one's going to use > a 16-bit ENTER? > Fixed for V5. >> + GINSN_DST_REG, REG_SP, 0); >> + ginsn_set_where (ginsn); >> + ginsn_next = ginsn_new_store (insn_end_sym, false, >> + GINSN_SRC_REG, REG_FP, >> + GINSN_DST_INDIRECT, REG_SP, 0); >> + ginsn_set_where (ginsn_next); >> + gas_assert (!ginsn_link_next (ginsn, ginsn_next)); >> + ginsn_last = ginsn_new_mov (insn_end_sym, false, >> + GINSN_SRC_REG, REG_SP, 0, >> + GINSN_DST_REG, REG_FP, 0); >> + ginsn_set_where (ginsn_last); >> + gas_assert (!ginsn_link_next (ginsn_next, ginsn_last)); >> + >> + return ginsn; >> +} >> + >> +static bool >> +x86_ginsn_safe_to_skip (void) >> +{ >> + bool skip_p = false; >> + uint16_t opcode = i.tm.base_opcode; >> + >> + switch (opcode) >> + { >> + case 0x39: > > This isn't the only CMP encoding, and ... > >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + /* cmp reg, reg. */ >> + skip_p = true; >> + break; >> + case 0x85: > > ... this isn't the only TEST one. > These were added as I ran into them. Will add other opcodes and a FIXME around the outstanding problem: There may also be yet more opcodes which are safe to skip... >> + /* test reg, reg/mem. */ >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + skip_p = true; >> + break; >> + default: >> + break; >> + } >> + >> + return skip_p; >> +} >> + >> +#define X86_GINSN_UNHANDLED_NONE 0 >> +#define X86_GINSN_UNHANDLED_DEST_REG 1 >> +#define X86_GINSN_UNHANDLED_CFG 2 >> +#define X86_GINSN_UNHANDLED_STACKOP 3 >> + >> +/* Check the input insn for its impact on the correctness of the synthesized >> + CFI. Returns an error code to the caller. */ >> + >> +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. >> + } >> + >> + return err; >> +} >> + >> +/* 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. >> + - All instructions that perform stack manipulation implicitly: the CALL, >> + RET, PUSH, POP, ENTER, and LEAVE instructions. >> + >> + The function currently supports GINSN_GEN_SCFI ginsn generation mode only. >> + To support other generation modes 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 >> + possible starting points. > > Oh, I see you're not meaning to have this annotation consistently. That's a > little sad ... > >> + - Also note that ginsn representation may need enhancements. Specifically, >> + note some TBD_GINSN_INFO_LOSS and TBD_GINSN_REPRESENTATION_LIMIT markers. >> + */ >> + >> +static ginsnS * >> +x86_ginsn_new (const symbolS *insn_end_sym, enum ginsn_gen_mode gmode) >> +{ >> + int err = 0; >> + uint16_t opcode; >> + unsigned int dw2_regnum; >> + ginsnS *ginsn = NULL; >> + ginsnS *ginsn_next = NULL; >> + ginsnS *ginsn_last = NULL; >> + /* In 64-bit mode, the default stack update size is 8 bytes. */ >> + int stack_opnd_size = 8; >> + >> + /* Currently supports generation of selected ginsns, sufficient for >> + the use-case of SCFI only. */ >> + if (gmode != GINSN_GEN_SCFI) >> + return ginsn; >> + >> + opcode = i.tm.base_opcode; >> + >> + switch (opcode) >> + { >> + case 0x1: >> + /* add reg, reg/mem. */ >> + case 0x29: >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; > > What about the new APX NDD encodings in EVEX map 4? > I need to disallow them until I have proper handling for them. I dont know why I didnt include the is_apx_rex2_encoding () and is_apx_evex_encoding () check to bar them in V4. I will add that check in x86_ginsn_new to disallow those with SCFI for now. >> + /* sub reg, reg/mem. */ > > Please be careful with placing such comments when there are multiple > case labels (or fall-through). I think these would better go on the > same lines as the case labels themselves. > >> + ginsn = x86_ginsn_addsub_reg_mem (insn_end_sym); >> + break; >> + >> + case 0x3: >> + /* add reg/mem, reg. */ >> + case 0x2b: >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + /* sub reg/mem, reg. */ >> + ginsn = x86_ginsn_addsub_mem_reg (insn_end_sym); >> + break; >> + >> + 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. Something like !is_apx_rex2_encoding () && !(i.prefix[REX_PREFIX] & REX_W) covers it ? >> + /* 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 ? >> + else if (flag_code == CODE_64BIT && ginsn_prefix_66H_p (i)) >> + stack_opnd_size = 2; >> + else >> + stack_opnd_size = 4; > > In 64-bit mode stack operations are never 32-bit. > Ah right, so I need to use 8 in the fallback case, i.e., stack_opnd_size is 8 in case of push imm32. >> + /* Skip getting the value of imm from machine instruction >> + because this is not important for SCFI. */ >> + 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_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: >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + ginsn = x86_ginsn_jump_cond (insn_end_sym); >> + break; > > I think this wants a comment briefly explaining why SPACE_0F opcodes > 0x8[0-f] don't need handling explicitly. Same for JMP (0xeb) below. > OK. >> + case 0x80: >> + case 0x81: >> + case 0x83: >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + ginsn = x86_ginsn_alu_imm (insn_end_sym); >> + break; >> + >> + case 0x8a: >> + case 0x8b: >> + /* Move reg/mem, reg (8/16/32/64). */ >> + case 0x88: >> + case 0x89: >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + /* mov reg, reg/mem. (8/16/32/64). */ >> + ginsn = x86_ginsn_move (insn_end_sym); >> + break; >> + >> + case 0x8d: >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + /* lea disp(%src), %dst */ > > disp(%src) doesn't really represent the full set of possibilities. > Why not use "mem" as you do elsewhere? > I can update this to "lea disp(%base,%index,imm) %dst". If I use mem now, it may confuse me later. >> + ginsn = x86_ginsn_lea (insn_end_sym); >> + break; >> + >> + case 0x8f: >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + /* pop to mem. */ > > Or to register. While this won't happen today, allowing a means to > have the programmer request that alternative encoding would surely > miss to update the code here. Hence this code would better be ready > to deal with the case right away. > >> + gas_assert (i.base_reg); > > POP isn't different from other explicit memory accesses: All forms > are allowed, and hence a base register may not be in use. > > Both remarks also apply to PUSH further down. > OK. I will take a look. >> + 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); >> + 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 0x9c: >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + /* pushf / pushfq. */ >> + /* 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); >> + /* Tracking EFLAGS register by number is not necessary. */ > > How does this fit with ... > >> + 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 0x9d: >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + /* popf / popfq. */ >> + /* 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; >> + /* FIXME - hardcode the actual DWARF reg number value. As for SCFI >> + correctness, although this behaves simply a placeholder value; its >> + just clearer if the value is correct. */ >> + dw2_regnum = 49; > > ... this? > Inconsistency on my part. I have fixed the above case to use the actual DWARF reg number like done here. >> + 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. >> + } >> + 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. >> + case 0xc8: >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + /* enter. */ >> + ginsn = x86_ginsn_enter (insn_end_sym); >> + break; >> + >> + case 0xc9: >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + /* 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, > > Same comment as for ENTER wrt operand size. > I have added handling 16-bit leave for V5. >> + GINSN_DST_REG, REG_SP, 0); >> + ginsn_set_where (ginsn_next); >> + gas_assert (!ginsn_link_next (ginsn_next, ginsn_last)); >> + break; >> + >> + case 0xe0 ... 0xe2: >> + /* loop / loope / loopne. */ >> + case 0xe3: >> + /* jecxz / jrcxz. */ >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + ginsn = x86_ginsn_jump_cond (insn_end_sym); >> + ginsn_set_where (ginsn); >> + break; >> + >> + case 0xe8: >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + /* 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; > > Again - what about the stack pointer adjustment? Or wait - you only > care about the local function's view. Just that this will get you > in trouble for something like > > call 1f > 1: > pop %rax > > CALL with zero displacement really acts as if "push %rip". > Trouble for SCFI, yes. Some of these cases may get help once the feature to issue diagnostic at unbalanced stack at the end of function is implemented. I'm afraid there is no other respite for the user here otherwise. I'd expect this to be a corner case though. I will keep this is my notes until I have a resolution to this. >> + case 0xeb: >> + /* If opcode_space != SPACE_BASE, this is not a jmp insn. Skip it >> + for GINSN_GEN_SCFI. */ >> + if (i.tm.opcode_space != SPACE_BASE) >> + break; >> + /* Unconditional jmp. */ >> + ginsn = x86_ginsn_jump (insn_end_sym); >> + ginsn_set_where (ginsn); >> + break; >> + >> + default: >> + /* TBD_GINSN_GEN_NOT_SCFI: Skip all other opcodes uninteresting for >> + GINSN_GEN_SCFI mode. */ >> + break; >> + } >> + >> + if (!ginsn && !x86_ginsn_safe_to_skip ()) >> + { >> + /* For all unhandled insns that are not whitelisted, check that they do >> + not impact SCFI correctness. */ >> + err = x86_ginsn_unhandled (); >> + switch (err) >> + { >> + case X86_GINSN_UNHANDLED_NONE: >> + break; >> + case X86_GINSN_UNHANDLED_DEST_REG: >> + /* Not all writes to REG_FP are harmful in context of SCFI. Simply >> + generate a GINSN_TYPE_OTHER with destination set to the >> + appropriate register. The SCFI machinery will bail out if this >> + ginsn affects SCFI correctness. */ >> + dw2_regnum = ginsn_dw2_regnum (i.op[i.operands - 1].regs); >> + ginsn = ginsn_new_other (insn_end_sym, true, >> + GINSN_SRC_IMM, 0, >> + GINSN_SRC_IMM, 0, >> + GINSN_DST_REG, dw2_regnum); >> + ginsn_set_where (ginsn); >> + break; >> + case X86_GINSN_UNHANDLED_CFG: >> + /* Fall through. */ >> + case X86_GINSN_UNHANDLED_STACKOP: > > No fall-through comment please between immediately successive case labels. > >> + as_bad (_("SCFI: unhandled op 0x%x may cause incorrect CFI"), >> + i.tm.base_opcode); > > As a remark: %#x is a one byte shorter representation with largely the > same effect (plus, nicely imo, omitting the 0x when the value is zero). > OK to both of the above comments. >> + break; >> + default: >> + abort (); >> + break; >> + } >> + } >> + >> + return ginsn; >> +} >> + >> /* This is the guts of the machine-dependent assembler. LINE points to a >> machine dependent instruction. This function is supposed to emit >> the frags/bytes it assembles to. */ >> @@ -5299,6 +6272,7 @@ md_assemble (char *line) >> const char *end, *pass1_mnem = NULL; >> enum i386_error pass1_err = 0; >> const insn_template *t; >> + ginsnS *ginsn; >> struct last_insn *last_insn >> = &seg_info(now_seg)->tc_segment_info_data.last_insn; >> >> @@ -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 ? >> + if (flag_synth_cfi) >> + { >> + ginsn = x86_ginsn_new (symbol_temp_new_now (), frch_ginsn_gen_mode ()); >> + frch_ginsn_data_append (ginsn); >> + } >> + >> insert_lfence_after (); >> >> if (i.tm.opcode_modifier.isprefix) >> @@ -11333,6 +12315,7 @@ s_insn (int dummy ATTRIBUTE_UNUSED) >> const char *end; >> unsigned int j; >> valueT val; >> + ginsnS *ginsn; >> bool vex = false, xop = false, evex = false; >> struct last_insn *last_insn; >> >> @@ -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. I could consider forbidding use of .insn as you suggest, for the first cut. >> + frch_ginsn_data_append (ginsn); >> + } >> + >> done: >> *saved_ilp = saved_char; >> input_line_pointer = line; >> @@ -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. Thanks again