Mailing List Archive

[XEN PATCH 08/11] x86/altcall: address violations of MISRA C Rule 20.7
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>
---
xen/arch/x86/include/asm/alternative.h | 76 +++++++++++++-------------
1 file changed, 38 insertions(+), 38 deletions(-)

diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
index 0d3697f1de49..a3b7cbab8740 100644
--- 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)()); \
(void)alternative_callN(0, int, func); \
})

-#define alternative_call0(func) ({ \
- ALT_CALL_NO_ARG1; \
- alternative_callN(0, typeof(func()), func); \
+#define alternative_call0(func) ({ \
+ ALT_CALL_NO_ARG1; \
+ alternative_callN(0, typeof((func)()), func); \
})

#define alternative_vcall1(func, arg) ({ \
typeof(arg) v1_ = (arg); \
ALT_CALL_ARG(v1_, 1); \
ALT_CALL_NO_ARG2; \
- (void)sizeof(func(arg)); \
+ (void)sizeof((func)(arg)); \
(void)alternative_callN(1, int, func); \
})

-#define alternative_call1(func, arg) ({ \
- typeof(arg) v1_ = (arg); \
- ALT_CALL_ARG(v1_, 1); \
- ALT_CALL_NO_ARG2; \
- alternative_callN(1, typeof(func(arg)), func); \
+#define alternative_call1(func, arg) ({ \
+ typeof(arg) v1_ = (arg); \
+ ALT_CALL_ARG(v1_, 1); \
+ ALT_CALL_NO_ARG2; \
+ alternative_callN(1, typeof((func)(arg)), func); \
})

#define alternative_vcall2(func, arg1, arg2) ({ \
@@ -273,17 +273,17 @@ extern void alternative_branches(void);
ALT_CALL_ARG(v1_, 1); \
ALT_CALL_ARG(v2_, 2); \
ALT_CALL_NO_ARG3; \
- (void)sizeof(func(arg1, arg2)); \
+ (void)sizeof((func)(arg1, arg2)); \
(void)alternative_callN(2, int, func); \
})

-#define alternative_call2(func, arg1, arg2) ({ \
- typeof(arg1) v1_ = (arg1); \
- typeof(arg2) v2_ = (arg2); \
- ALT_CALL_ARG(v1_, 1); \
- ALT_CALL_ARG(v2_, 2); \
- ALT_CALL_NO_ARG3; \
- alternative_callN(2, typeof(func(arg1, arg2)), func); \
+#define alternative_call2(func, arg1, arg2) ({ \
+ typeof(arg1) v1_ = (arg1); \
+ typeof(arg2) v2_ = (arg2); \
+ ALT_CALL_ARG(v1_, 1); \
+ ALT_CALL_ARG(v2_, 2); \
+ ALT_CALL_NO_ARG3; \
+ alternative_callN(2, typeof((func)(arg1, arg2)), func); \
})

#define alternative_vcall3(func, arg1, arg2, arg3) ({ \
@@ -294,20 +294,20 @@ extern void alternative_branches(void);
ALT_CALL_ARG(v2_, 2); \
ALT_CALL_ARG(v3_, 3); \
ALT_CALL_NO_ARG4; \
- (void)sizeof(func(arg1, arg2, arg3)); \
+ (void)sizeof((func)(arg1, arg2, arg3)); \
(void)alternative_callN(3, int, func); \
})

-#define alternative_call3(func, arg1, arg2, arg3) ({ \
- typeof(arg1) v1_ = (arg1); \
- typeof(arg2) v2_ = (arg2); \
- typeof(arg3) v3_ = (arg3); \
- ALT_CALL_ARG(v1_, 1); \
- ALT_CALL_ARG(v2_, 2); \
- ALT_CALL_ARG(v3_, 3); \
- ALT_CALL_NO_ARG4; \
- alternative_callN(3, typeof(func(arg1, arg2, arg3)), \
- func); \
+#define alternative_call3(func, arg1, arg2, arg3) ({ \
+ typeof(arg1) v1_ = (arg1); \
+ typeof(arg2) v2_ = (arg2); \
+ typeof(arg3) v3_ = (arg3); \
+ ALT_CALL_ARG(v1_, 1); \
+ ALT_CALL_ARG(v2_, 2); \
+ ALT_CALL_ARG(v3_, 3); \
+ ALT_CALL_NO_ARG4; \
+ alternative_callN(3, typeof((func)(arg1, arg2, arg3)), \
+ func); \
})

#define alternative_vcall4(func, arg1, arg2, arg3, arg4) ({ \
@@ -320,7 +320,7 @@ extern void alternative_branches(void);
ALT_CALL_ARG(v3_, 3); \
ALT_CALL_ARG(v4_, 4); \
ALT_CALL_NO_ARG5; \
- (void)sizeof(func(arg1, arg2, arg3, arg4)); \
+ (void)sizeof((func)(arg1, arg2, arg3, arg4)); \
(void)alternative_callN(4, int, func); \
})

@@ -334,8 +334,8 @@ extern void alternative_branches(void);
ALT_CALL_ARG(v3_, 3); \
ALT_CALL_ARG(v4_, 4); \
ALT_CALL_NO_ARG5; \
- alternative_callN(4, typeof(func(arg1, arg2, \
- arg3, arg4)), \
+ alternative_callN(4, typeof((func)(arg1, arg2, \
+ arg3, arg4)), \
func); \
})

@@ -351,7 +351,7 @@ extern void alternative_branches(void);
ALT_CALL_ARG(v4_, 4); \
ALT_CALL_ARG(v5_, 5); \
ALT_CALL_NO_ARG6; \
- (void)sizeof(func(arg1, arg2, arg3, arg4, arg5)); \
+ (void)sizeof((func)(arg1, arg2, arg3, arg4, arg5)); \
(void)alternative_callN(5, int, func); \
})

@@ -367,8 +367,8 @@ extern void alternative_branches(void);
ALT_CALL_ARG(v4_, 4); \
ALT_CALL_ARG(v5_, 5); \
ALT_CALL_NO_ARG6; \
- alternative_callN(5, typeof(func(arg1, arg2, arg3, \
- arg4, arg5)), \
+ alternative_callN(5, typeof((func)(arg1, arg2, arg3, \
+ arg4, arg5)), \
func); \
})

@@ -385,7 +385,7 @@ extern void alternative_branches(void);
ALT_CALL_ARG(v4_, 4); \
ALT_CALL_ARG(v5_, 5); \
ALT_CALL_ARG(v6_, 6); \
- (void)sizeof(func(arg1, arg2, arg3, arg4, arg5, arg6)); \
+ (void)sizeof((func)(arg1, arg2, arg3, arg4, arg5, arg6)); \
(void)alternative_callN(6, int, func); \
})

@@ -402,8 +402,8 @@ extern void alternative_branches(void);
ALT_CALL_ARG(v4_, 4); \
ALT_CALL_ARG(v5_, 5); \
ALT_CALL_ARG(v6_, 6); \
- alternative_callN(6, typeof(func(arg1, arg2, arg3, \
- arg4, arg5, arg6)), \
+ alternative_callN(6, typeof((func)(arg1, arg2, arg3, \
+ arg4, arg5, arg6)), \
func); \
})

--
2.34.1
Re: [XEN PATCH 08/11] x86/altcall: address violations of MISRA C Rule 20.7 [ In reply to ]
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?

Jan
Re: [XEN PATCH 08/11] x86/altcall: address violations of MISRA C Rule 20.7 [ In reply to ]
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?
>
> Jan

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?

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH 08/11] x86/altcall: address violations of MISRA C Rule 20.7 [ In reply to ]
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
Re: [XEN PATCH 08/11] x86/altcall: address violations of MISRA C Rule 20.7 [ In reply to ]
On 2024-03-25 15:58, Jan Beulich wrote:
> 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

Ok, I see what you meant now. I'll deviate these two macros.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)