| Message ID | 653ead65966226f50b0e4ae0268912c9710f9dba.1711118582.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | address some violations of MISRA C Rule 20.7 | expand |
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
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?
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
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.
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); \ })
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(-)