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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxxx>
>>
>> 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