From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D0322C00144 for ; Fri, 29 Jul 2022 07:23:17 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.377376.610567 (Exim 4.92) (envelope-from ) id 1oHKKe-0006W4-FY; Fri, 29 Jul 2022 07:22:48 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 377376.610567; Fri, 29 Jul 2022 07:22:48 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1oHKKe-0006Vx-Cs; Fri, 29 Jul 2022 07:22:48 +0000 Received: by outflank-mailman (input) for mailman id 377376; Fri, 29 Jul 2022 07:22:47 +0000 Received: from se1-gles-flk1-in.inumbo.com ([94.247.172.50] helo=se1-gles-flk1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1oHKKd-0006Vr-AR for xen-devel@lists.xenproject.org; Fri, 29 Jul 2022 07:22:47 +0000 Received: from EUR05-VI1-obe.outbound.protection.outlook.com (mail-vi1eur05on2065.outbound.protection.outlook.com [40.107.21.65]) by se1-gles-flk1.inumbo.com (Halon) with ESMTPS id 3de6eab6-0f0f-11ed-924f-1f966e50362f; Fri, 29 Jul 2022 09:22:46 +0200 (CEST) Received: from VE1PR04MB6560.eurprd04.prod.outlook.com (2603:10a6:803:122::25) by DB6PR0401MB2373.eurprd04.prod.outlook.com (2603:10a6:4:4b::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5482.10; Fri, 29 Jul 2022 07:22:41 +0000 Received: from VE1PR04MB6560.eurprd04.prod.outlook.com ([fe80::9da9:fa60:f04a:2a0e]) by VE1PR04MB6560.eurprd04.prod.outlook.com ([fe80::9da9:fa60:f04a:2a0e%7]) with mapi id 15.20.5482.011; Fri, 29 Jul 2022 07:22:41 +0000 X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: 3de6eab6-0f0f-11ed-924f-1f966e50362f ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Wqg+gETwyaeDg7ara1MCghmL+XIWsoxgW1pKXa9mSpLO/2T2Y07RNRio9XQnlm3IsgHlS+rxVb0rjYYrAMeF7u5aC4R+gixVuHtp8sVfJT1kyIi8s89vejhqjWOy2b6qZCoE2VMQrhMvPYTyKsC66H5VE8AJppHDybzdyp2kStoKEDRPW1G/4oDs6mdzFg7E85qZNXukao0RbxxXKifThYi0A+6CAZV7qv8gu37yGFEVKIav9TlfcVdneQqQ4fAtfwVT/WeWt6gtPC29QRJW6RMpRXd+qafhE9072vW88xDGpVfGPYpzx2oK6v/cHmsSUHD+SO40x4BWO2KVKE1CFQ== 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=BDMirb63e4mQrLrukOg1fWLmKm00xM0tUHidAUnE5X0=; b=HbN6q9hqF/tLLgBJnOFIgnHhdzOzGuL3dVGy4oLJu0o19Aw6wtdxpRuayko7C7uJmPxmRcWbtRPdBZR+XfVybz1SbIMNMywtAgIirGgk1cHv3Nocjb6Z9TTtYJD1yiMgglMqurIEkeuU88mYVqaf7GN5KrjQOh1d6bJBieXMdHr2ewd5ShqwHmgbYIWI5uy27uAHvPm9SnjUAAYKNReX8GfeaV77hPCmU5Q9iSWjJtRsI9DFtEy/QRFgAmN6U6jKqUzTCo7/l+616GI5pz8eqetmsx6cHPlr+GrGidxfxl88iUSz0p05j83yyVWCNC11njWgaEX9fvSW+9kNN28jMg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=BDMirb63e4mQrLrukOg1fWLmKm00xM0tUHidAUnE5X0=; b=sztcMS92vgOY1hjgl0luVjE9W7M3VNSdcPc3hDREIICeRGtSuLaoC1DHUtEAVfPVduj6i1fNKfogsEG8G1IKNKALd5M57XEs/5QiQjQHnSUDVxPFRaxr/c5DN8l7hHke+7IX00jM3AQhmESeXkWvd6Y/CVV5DsOd/CKt3qo9Jys3ZpBKal9w1IqXX7VTzVgYAJIPrwS1WjKsnBJ0Cqc5D/mzbgtrOYFzgNiwYyYcgOQlvdKMzeI656yUpnJrm8eSdxfoqZWfGSkEPOzqxsJJH8eRa93fz90x82kh1xNb9yLdM7K1Kc8bjjI0gjvFF2X8TXSIZOM6wwdKKxQdqORUGQ== Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com; Message-ID: Date: Fri, 29 Jul 2022 09:22:39 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH] xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation Content-Language: en-US To: Xenia Ragiadakou , Julien Grall Cc: Bertrand Marquis , Volodymyr Babchuk , xen-devel@lists.xenproject.org, Stefano Stabellini References: <20220728134943.1185621-1-burzalodowa@gmail.com> <0a8ff178-280d-717f-dacb-4eb9f57a24eb@xen.org> <83c17bf6-b9b2-a297-6f7f-dd08231d0f90@suse.com> <69942917-f2e9-718e-094d-9b01aea16a4a@gmail.com> From: Jan Beulich In-Reply-To: <69942917-f2e9-718e-094d-9b01aea16a4a@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-ClientProxiedBy: FR3P281CA0008.DEUP281.PROD.OUTLOOK.COM (2603:10a6:d10:1d::14) To VE1PR04MB6560.eurprd04.prod.outlook.com (2603:10a6:803:122::25) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: c9a908a1-2395-4d7c-33a4-08da71331f82 X-MS-TrafficTypeDiagnostic: DB6PR0401MB2373:EE_ X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 8aQ9Lp9qfz5aYVJFlAOvhA5Q36KEaXViJNTONanztnJ994sPYn+0XM2bluFClm/dt+QAi+MQ1WcX4iVbQeaFpr2mLeB5rAGBXNsL4cjXj1C5ZJuspFg1M12Y2qeOTVvUH2fDvxl2EBtYuAGhmsjc8GDAzqBDmR+2A3wflyOaCXWEGG50LDDpDgif5919AWYZrCZFg8TD/E26yGBRi7Uiio9fyGQBf59m/Y+a6lMkDCNvDv+t3lABlYZp6982NYn0cwo0eei7LGlptSWayMYRZ95+N3V25pHxRf/LptU7hhZHeZx0l5IhVudX2qzAvnvJfSQW4FLNrN75vJcSBT249lfVis38E3MhDZKy3xd8frAuxooNGxqUHZeoUHx2q/lKmyAIewg+nX1afdBrdyxjbyTcWiG7+M6EdrjsICrmIRvZmAYrUePuE1D1hkU33lxvIvc5qvXW+aGZlcohlkpun5/aD5vLzMTpnl69w7Mb+a7649noeeeGgHWy/JBQx3Ec/W72cshndGbgwjWylMz17LJ210vbEVcyioo9Bi4s8yLziAfQS+yjXVdAvLk90ztytuDMHCcFqEx8sRWC27NZMM63maqR7e2qfR1W8WOLe8ZPsm+0DYxME+paXahQbcfcvp8u0tQ5kFKxRMJGKpM2N79HdGcY+vdCIpQxEVKxmCFphLbZK4YD9VdB2WG7mQ9PKj7pnzvwF85U1aMlPcWbf29q1cGYMjfZwpm4Vgfg/5i5LgnogwY9r+v0pkQ+vDajwhRf6gCvXikSM4AT7AMbLE3hB18N2BiaVONc1eBONVLL+b2S4wDxG1E5cFRyPKwOiOVWvE4fO2PbZgW5e6A3Xg== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:VE1PR04MB6560.eurprd04.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230016)(366004)(38100700002)(8936002)(2906002)(31686004)(5660300002)(110136005)(54906003)(31696002)(66946007)(4326008)(86362001)(66476007)(36756003)(8676002)(66556008)(53546011)(6506007)(2616005)(6486002)(508600001)(26005)(6512007)(83380400001)(186003)(43740500002)(45980500001);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QzQ0RllwdFR6VVZSMVZ1cGpFcythTERoNk5GZUJvS1BVRWFmb0Z4Z0hzU25Z?= =?utf-8?B?eW9rcEVsS3ZYT2U3MEhvVWJjMDdUVk1ObGlIUDF1ZHRWeStlQlJ1ZGQrUkpH?= =?utf-8?B?VGh6cHk0T3krNkpPa3RsaFFzTHZpcktON2VxOGttQk1zdmNnZ0lVZmZRUVhH?= =?utf-8?B?UVg0UWNKRU56M1dOQzYrUWc4aURsTWNLTHJSUjNLUERoMHpUM1FaWG1qYkQz?= =?utf-8?B?RG1LM0d0U0xkb3FmbzZBK0pHVjVQMm1vQytiZkU2TXZpbVhucFFQdTBFL0xk?= =?utf-8?B?eUQ3UlZTb1M2MkNKUy9zRU40YVVVenpMbkloOTUvemZpNEpHRjhBWjdua0Mv?= =?utf-8?B?eVhXcUsvWnNyckdxV3J6dnVLYlpGVlg3YVlZQlBvV2tHZjlkbWRIdThCSnY0?= =?utf-8?B?R3F4UXNCYVVQTHptMjVrQjhJbW82NkhRQzlDazlrU0tYYkNNVmNiY0x2L1Jj?= =?utf-8?B?L2ZkSWUzSmtMSEZwUmxocENEVU5SOWptRWo4TVZYN2ZKbVp6RXowRXRlTi9V?= =?utf-8?B?RldjVGRheW12bGJPMlFCSDY4N3ZDdTNqY3owemNPZmM2MmtzQlY5TWFwbG54?= =?utf-8?B?QlVUaFJscDVqK1lPaDJtL2srR0FjZklsaWgyWVZLcDdwUmgveXY5QUNhYjZk?= =?utf-8?B?Y2xsYjZmLzhHRkU2eHlubXRpcTlMYmFSMHluOTIxZ0hDMW9KVVROY1JXbE8y?= =?utf-8?B?UWVKZUlDUXBLTHlmNk4vRUpsRWpqRUxDSjc2b1hTb0dia2g2SWFJMEVKSmlq?= =?utf-8?B?ZjlBVDA5TEN1OVlDUzJ5K2pXYWtXMm10ZHhBazZHNkF3REhTSlMyeTEzZEZW?= =?utf-8?B?bzMzSEdIWW5COUNUdi9SakJ4YmsrWjBIQjAwWjVVV1dLRlpYZllDOVplZzhk?= =?utf-8?B?eDlkQTFjYm9YUlkrM2FYTmRCaFY5RllKVUFrbmhCaUFqL0M0OUkveGpRbXpQ?= =?utf-8?B?clliZzZDNjFxaHZHZnZpM0xCV1FoNTBrN1JteDI3T3UyNEl2MnBLa1cwQlM4?= =?utf-8?B?UXBSV00yOFlyV2RrVHRNU2xIeWQ0TmFqVW5ueHo4SklLVTEyVjhLSE4xVUV5?= =?utf-8?B?V29iOFJhaWpWVnQ5V2RHakhOV2NaUXNrK0NuK3QxeUsyVk1hSlBKaTc4SU5z?= =?utf-8?B?TmtBdW8yWnBibEFrakF5UkU5T2djc0RqVTRDSWQ1NVdqZmNIL2xiUzZiNGdS?= =?utf-8?B?cWxlYVZFcXFwd3RSRTJPT0xxM0RkQmpLdWsydDdIMTFpZzlLM0dibUREVlRs?= =?utf-8?B?NXFsMThIajhjOFVjSG03OWNzZDIwR1VJaVFYRUNoRHN1dEpIeUZUTEp0T3F6?= =?utf-8?B?NmZwK1NTenV5eURMbnlldFZpeVM5R1NESkhyVkJDRnhXQ0lLYU1ucGRudEtO?= =?utf-8?B?bi80OTBIczRNelJrWTQ1S3lIdDlxVWFtcGNwMTlzazFyZlB2bFlIU1JPTHln?= =?utf-8?B?ZmJDYlBqZUNrcUpJemhVM3pEOUg1SDFnUDcxMFdIWW1WSkF6WWxUVzFKY1pG?= =?utf-8?B?cEg2K1VpcFdsUFBZQjg0N2VaaUxvVmE0WTdESUxxRWdNYWMvOW15ZHoyK0h2?= =?utf-8?B?VlVnRHhwZE8vOXFhRzBDaDh4NUpTYlJlbS9OUG0yMzFFSExERW1SY3NyMC9V?= =?utf-8?B?VUV1a3FhZmR2blVaVWloeGdTVE4xelJMeHNESEc4VC9FeE83d3hBZEtUb25Y?= =?utf-8?B?cHNZczF0dGJ0RllwL2pmS01mT2t2YkhPazBzVzBKam1lbytheFEyR2Z5S2U2?= =?utf-8?B?cDk0K1NzTjFnNDZDR0VsOXkxajNuRzR1R3Fjd3Y0T3ZzbXNUTEdoOVdUOERS?= =?utf-8?B?R0g1TkYwL2V2NENESVpJa1VZdXFBVDNDaFpnbXF4d1ZBdTJRa3FJZThPMnZ4?= =?utf-8?B?VWN0Vzh5VVdnSzVhZzcxOFVxcG5Pc2lyTWtvaXJTUjlNOEFuV084ZlVHdjhJ?= =?utf-8?B?WXJEWmJ4STdmcVd3UDRiQWRFTURmQmZOU3VTZHdSQXpCZS9HQ2N6YjB0Skx1?= =?utf-8?B?NUVuUWJWTXR4ckhUUEgxUFNhQ1FqSkR1d2dvdTJLV2NLblRNRklnaVRuNVN0?= =?utf-8?B?R1NZUTZGZVQ0dzRLeFJiekxVNXE5N0RRUUphVGNkcHR4dHVWQlc1YjVIUzNY?= =?utf-8?Q?6DeVyAXTtnA8UplmAYC2KCa/w?= X-OriginatorOrg: suse.com X-MS-Exchange-CrossTenant-Network-Message-Id: c9a908a1-2395-4d7c-33a4-08da71331f82 X-MS-Exchange-CrossTenant-AuthSource: VE1PR04MB6560.eurprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jul 2022 07:22:41.3650 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: veKSU4jFAngE788ZK6ThH9cRU7Q/bpCcTv7XGeHP00Q10vzunonibSPk3Dy5K8GCf8yzntDhyXVR/pUfi1uXWw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0401MB2373 On 29.07.2022 09:01, Xenia Ragiadakou wrote: > On 7/29/22 09:16, Jan Beulich wrote: >> On 29.07.2022 07:23, Xenia Ragiadakou wrote: >>> On 7/29/22 01:56, Stefano Stabellini wrote: >>>> On Thu, 28 Jul 2022, Julien Grall wrote: >>>>> On 28/07/2022 15:20, Jan Beulich wrote: >>>>>> On 28.07.2022 15:56, Julien Grall wrote: >>>>>>> On 28/07/2022 14:49, Xenia Ragiadakou wrote: >>>>>>>> --- a/xen/arch/arm/include/asm/arm64/sysregs.h >>>>>>>> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h >>>>>>>> @@ -461,7 +461,7 @@ >>>>>>>> /* Access to system registers */ >>>>>>>> #define WRITE_SYSREG64(v, name) do { \ >>>>>>>> - uint64_t _r = v; \ >>>>>>>> + uint64_t _r = (v); \ >>>>>>> >>>>>>> I am failing to see why the parentheses are necessary here. Could you >>>>>>> give an example where the lack of them would end up to different code? >>>>>> >>>>>> I think it is merely good practice to parenthesize the right sides of =. >>>>>> Indeed with assignment operators having second to lowest precedence, and >>>>>> with comma (the lowest precedence one) requiring parenthesization at the >>>>>> macro invocation site, there should be no real need for parentheses here. >>>>> >>>>> I am not really happy with adding those parentheses because they are >>>>> pointless. But if there are a consensus to use it, then the commit message >>>>> should be updated to clarify this is just here to please MISRA (to me "need" >>>>> implies it would be bug). >>>> >>>> Let me premise that I don't know if this counts as a MISRA violation or >>>> not. (Also I haven't checked if cppcheck/eclair report it as violation.) >>>> >>>> But I think the reason for making the change would be to follow our >>>> coding style / coding practices. It makes the code simpler to figure out >>>> that it is correct, to review and maintain if we always add the >>>> parenthesis even in cases like this one where they are not strictly >>>> necessary. We are going to save our future selves some mental cycles. >>>> >>>> So the explanation on the commit message could be along those lines. >>> >>> First, the rule 20.7 states "Expressions resulting from the expansion of >>> macro parameters shall >>> be enclosed in parentheses". So, here it is a clear violation of the >>> rule because the right side of the assignment operator is an expression. >>> >>> Second, as I stated in a previous email, if v is not enclosed in >>> parentheses, I could write the story of my life in there and compile it >>> :) So, it would be a bug. >>> >>> So, I recommend the title and the explanation i.e >>> "xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation >>> >>> The macro parameter 'v' is used as an expression and needs to be enclosed in >>> parentheses." >>> to remain as is because they are accurate. >> >> I'm afraid you're following the MISRA wording too much to the latter. >> Earlier on you agreed that when macro parameters are used as function >> arguments, the parentheses can be omitted. Yet by what you say above >> those are also expressions. > > Yes, those are also expressions (that's why I added parentheses > initially) and I agreed with you that the parentheses there may not be > necessary because I could not think of an example that will produce > different behaviors with and without the parentheses. This will need a > formal deviation I imagine or maybe a MISRA C expert could provide a > justification regarding why parentheses are needed around function > arguments that we may have not think of. > >> As indicated before - I think parentheses >> are wanted here, but it's strictly "wanted", and hence the title >> better wouldn't say "fix" (but e.g. "improve") and the description >> also should be "softened". >> > > Regarding the latter, are you saying that the parentheses are not needed? > In my opinion they are needed to prevent the bug described in the > previous email i.e passing multiple statements to the macro. Any such use would be rejected during review, I'm sure. However I think there's another case which might indeed make this more than just a "want" (and then responses further down are to be viewed only in the context of earlier discussion): #define wr(v) ({ \ unsigned r_ = v; \ asm("" :: "r" (r_)); \ }) #define M x, y void test(unsigned x) { wr(M); } While this would result in an unused variable warning, it's surely misleading (and less certain to be noticed during review) - which is what Misra wants to avoid. Let's see what Julien thinks. > By whom are they wanted? I 'm afraid I cannot understand. By us as a community: This can be viewed as one of many agreements we have on coding style. (As such it may want to be written down somewhere.) > Also, are you proposing to change the title into "Improve MISRA C 2012 > Rule 20.7 violation" ? Obviously not. I was thinking of "improve to avoid ...". Jan