Re: [XEN PATCH 08/11] x86/altcall: address violations of MISRA C Rule 20.7

From: Jan Beulich <jbeulich@suse.com>
To: Nicola Vetrini <nicola.vetrini@bugseng.com>
Cc: sstabellini@kernel.org, michal.orzel@amd.com,
	xenia.ragiadakou@amd.com, ayan.kumar.halder@amd.com,
	consulting@bugseng.com, andrew.cooper3@citrix.com,
	roger.pau@citrix.com, bertrand.marquis@arm.com, julien@xen.org,
	xen-devel@lists.xenproject.org
Subject: Re: [XEN PATCH 08/11] x86/altcall: address violations of MISRA C Rule 20.7
Date: Mon, 25 Mar 2024 15:58:06 +0100	[thread overview]
Message-ID: <b1777a3a-d468-40ea-aa5b-79d506a24abf@suse.com> (raw)
In-Reply-To: <bacda1cba33d40092209d30f48cb9c3b@bugseng.com>

On 25.03.2024 15:47, Nicola Vetrini wrote:
> On 2024-03-25 10:38, Jan Beulich wrote:
>> On 22.03.2024 17:01, Nicola Vetrini wrote:
>>> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
>>> of macro parameters shall be enclosed in parentheses". Therefore, some
>>> macro definitions should gain additional parentheses to ensure that 
>>> all
>>> current and future users will be safe with respect to expansions that
>>> can possibly alter the semantics of the passed-in macro parameter.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>
>> Hmm. These macros are, at least in part, hard to read already. The 
>> added
>> parentheses, while necessary when following the rule to the letter, are
>> making things worse, even if just slightly. I therefore have a proposal 
>> /
>> question:
>>
>>> --- a/xen/arch/x86/include/asm/alternative.h
>>> +++ b/xen/arch/x86/include/asm/alternative.h
>>> @@ -243,28 +243,28 @@ extern void alternative_branches(void);
>>>
>>>  #define alternative_vcall0(func) ({             \
>>>      ALT_CALL_NO_ARG1;                           \
>>> -    (void)sizeof(func());                       \
>>> +    (void)sizeof((func)());                     \
>>
>> Like this, all that's touched here are (syntactical, but not real) 
>> function
>> invocations. Function calls, like all postfix operators, are highest
>> precedence. Hence by omitting parentheses in that case no breakage can
>> happen as a result: If the passed expression is another postfix one, 
>> that'll
>> be evaluated first anyway. If any other expression is passed (aside 
>> primary
>> ones, of course), that'll be evaluated afterwards only due to being 
>> lower
>> precedence, irrespective of the presence/absence of parentheses.
>>
>> Therefore, where harmful to readability, can we perhaps leave postfix
>> expressions alone?
> 
> While I can understand the benefits of this, and the reasoning on 
> postfix expressions, what about, for instance (modulo the actual 
> invocation, this is just an example)
> 
> alternative_vcall0(2 + f) or similar scenarios?

Any kind of such expression will break with alternative_callN()'s
using of [addr] "i" (&(func)) as an asm() operand. Which clarifies
that even of the postfix expressions only some (in particular not
increment, decrement, and function call) could potentially be used
with the altcall machinery.

That said, you have a point in (indirectly) expressing that my
earlier description wasn't quite right (as in: too generic, when
it really needs tying to the specific case here).

Jan