| Message ID | 20220728134943.1185621-1-burzalodowa@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm64: sysreg.h: Fix MISRA C 2012 Rule 20.7 violation | expand |
Hi, On 28/07/2022 14:49, Xenia Ragiadakou wrote: > The macro parameter 'v' is used as an expression and needs to be enclosed in > parentheses. > > Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> > --- > xen/arch/arm/include/asm/arm64/sysregs.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h > index 54670084c3..f5a7269a27 100644 > --- 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? > asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \ > } while (0) > #define READ_SYSREG64(name) ({ \ Cheers,
On 28.07.2022 15: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); \ > asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \ Out of curiosity - why is the intermediate variable necessary? Can't v be used directly in the asm(), possibly with a suitable modifier added to %0 such that it'll always be x<N> (and not w<N>) which is used as the operand to "msr"? Jan
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. Jan
Hi, 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). Cheers,
Hi Jan, On 28/07/2022 15:17, Jan Beulich wrote: > On 28.07.2022 15: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); \ >> asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \ > > Out of curiosity - why is the intermediate variable necessary? > Can't v be used directly in the asm(), possibly with a suitable > modifier added to %0 such that it'll always be x<N> (and not > w<N>) which is used as the operand to "msr"? It should be possible to use %x0. However, We may need to use (uint64_t)(v). Linux seems to be use it, but IIRC they are not supported GCC versions as old as ours. So we would want to check when %x0 was introduced. Cheers,
Hi Julien, On 7/28/22 16:56, Julien Grall wrote: > Hi, > > On 28/07/2022 14:49, Xenia Ragiadakou wrote: >> The macro parameter 'v' is used as an expression and needs to be >> enclosed in >> parentheses. >> >> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> >> --- >> xen/arch/arm/include/asm/arm64/sysregs.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h >> b/xen/arch/arm/include/asm/arm64/sysregs.h >> index 54670084c3..f5a7269a27 100644 >> --- 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? Here v is supposed to be used as an expression. So maybe the rule wants to enforce that in the macro we will pass an expression and not multiple statements (?) ... not sure. > >> asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \ >> } while (0) >> #define READ_SYSREG64(name) ({ \ > > Cheers, >
On 7/28/22 17:34, Julien Grall wrote: > Hi, > > 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). The parentheses are not pointless if the intention is v to be used as an expression to the assignment. If this is not the intended usage for v, then the rule does not apply anyway.
On 28/07/2022 16:15, Xenia Ragiadakou wrote: > On 7/28/22 16:56, Julien Grall wrote: >> Hi, >> >> On 28/07/2022 14:49, Xenia Ragiadakou wrote: >>> The macro parameter 'v' is used as an expression and needs to be >>> enclosed in >>> parentheses. >>> >>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> >>> --- >>> xen/arch/arm/include/asm/arm64/sysregs.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h >>> b/xen/arch/arm/include/asm/arm64/sysregs.h >>> index 54670084c3..f5a7269a27 100644 >>> --- 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? > > Here v is supposed to be used as an expression. So maybe the rule wants > to enforce that in the macro we will pass an expression and not multiple > statements (?) ... not sure. Do you mean something like below? #include <stdio.h> #include <inttypes.h> #define foo(v) do { \ uint64_t _r = v; \ \ printf("_r %lu\n", _r); \ } while (0) int main(void) { foo(1; printf("Bar\n")); } If yes, then I agree that the compiler will still be happy. Or if you pass "1; 2", v would be set to 1 rather than 2. I am not sure why one would want to write such code. But I guess we want to prevent that. Cheers,
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.
Hi Stefano, 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. Xenia
On 29.07.2022 07:23, Xenia Ragiadakou wrote: > Hi Stefano, > > 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. 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". Jan
Hi Jan, On 7/29/22 09:16, Jan Beulich wrote: > On 29.07.2022 07:23, Xenia Ragiadakou wrote: >> Hi Stefano, >> >> 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. By whom are they wanted? I 'm afraid I cannot understand. Also, are you proposing to change the title into "Improve MISRA C 2012 Rule 20.7 violation" ?
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
On 7/29/22 10:22, Jan Beulich wrote: > 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. I would argue that any such use should be rejected by the compiler. > > 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.) > In that case, first it should be mentioned in the coding style document and then make reference to it in commit messages. >> 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 ...". >
On 7/29/22 10:22, Jan Beulich wrote: > 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. >> With the example that Jan provided I just realized, if function arguments are not parenthesized, somebody could alter the rest of the arguments with which a function is called via an intermediate macro ... a rather far fetched example but still ... >>> 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
Hi Jan, On 29/07/2022 08:22, Jan Beulich wrote: > 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); > } Interesting. I would have expected the pre-processor to first expand M and then consider wr() is called with 2 parameters. > > While this would result in an unused variable warning, FWIW, in our case, the compiler is going to throw an error. > it's surely > misleading (and less certain to be noticed during review) - which My expectation is we would notice that M is missing the parentheses. If it is really wanted, the name of the macro should be obvious. > is what Misra wants to avoid. Let's see what Julien thinks. I am struggling to see how this is different from: #define wr(v) printf("%u\n", v) If I am not mistaken, you have been arguing against adding the parentheses here. So, AFAIU, this means we will need to rely on the compiler to notice the extra parameters. Anyway, I am not against adding the parentheses in your example. However, I think we should be consistent how we use them. Cheers,
On 02.08.2022 19:32, Julien Grall wrote: > Hi Jan, > > On 29/07/2022 08:22, Jan Beulich wrote: >> 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); >> } > > Interesting. I would have expected the pre-processor to first expand M > and then consider wr() is called with 2 parameters. > >> >> While this would result in an unused variable warning, > > FWIW, in our case, the compiler is going to throw an error. > >> it's surely >> misleading (and less certain to be noticed during review) - which > My expectation is we would notice that M is missing the parentheses. If > it is really wanted, the name of the macro should be obvious. > >> is what Misra wants to avoid. Let's see what Julien thinks. > I am struggling to see how this is different from: > > #define wr(v) printf("%u\n", v) > > If I am not mistaken, you have been arguing against adding the > parentheses here. Yes - not the least because we actually use such in our code (at the very least in hvmloader, see PRIllx_arg()). > So, AFAIU, this means we will need to rely on the > compiler to notice the extra parameters. > > Anyway, I am not against adding the parentheses in your example. > However, I think we should be consistent how we use them. Indeed I, too, am all for consistency. Jan
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h index 54670084c3..f5a7269a27 100644 --- 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); \ asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \ } while (0) #define READ_SYSREG64(name) ({ \
The macro parameter 'v' is used as an expression and needs to be enclosed in parentheses. Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> --- xen/arch/arm/include/asm/arm64/sysregs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)