| Message ID | bb0ba44f8a3944c22a1c7cf19196c7060e8adb4b.1697123806.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C:2012 Rule 10.1 | expand |
On Thu, 12 Oct 2023, Nicola Vetrini wrote: > The purpose of this macro is to encapsulate the well-known expression > 'x & -x', that in 2's complement architectures on unsigned integers will > give 2^ffs(x), where ffs(x) is the position of the lowest set bit in x. > > A deviation for ECLAIR is also introduced. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > Changes in v2: > - rename to LOWEST_BIT > --- > automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ++++++ > xen/include/xen/macros.h | 6 ++++-- > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl > index d8170106b449..b8e1155ee49d 100644 > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl > @@ -274,6 +274,12 @@ still non-negative." > -config=MC3R1.R10.1,etypes+={safe, "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))", "dst_type(ebool||boolean)"} > -doc_end > > +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to obtain the value > +2^ffs(x) for unsigned integers on two's complement architectures > +(all the architectures supported by Xen satisfy this requirement)." > +-config=MC3R1.R10.1,reports+={safe, "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"} > +-doc_end Please also add the same deviation and explanation to docs/misra/deviations.rst. Other than that, this looks fine. > ### Set 3 ### > > # > diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h > index d0caae7db298..af47179d1056 100644 > --- a/xen/include/xen/macros.h > +++ b/xen/include/xen/macros.h > @@ -8,8 +8,10 @@ > #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) > #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) > > -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) > -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) > +#define LOWEST_BIT(x) ((x) & -(x)) > + > +#define MASK_EXTR(v, m) (((v) & (m)) / LOWEST_BIT(m)) > +#define MASK_INSR(v, m) (((v) * LOWEST_BIT(m)) & (m)) > > #define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x > #define count_args(args...) \ > -- > 2.34.1 >
Hi Nicola, On 12/10/2023 16:28, Nicola Vetrini wrote: > The purpose of this macro is to encapsulate the well-known expression > 'x & -x', that in 2's complement architectures on unsigned integers will > give 2^ffs(x), where ffs(x) is the position of the lowest set bit in x. In the commit message it is clear that the macro will return the lowest set bit. But... > > A deviation for ECLAIR is also introduced. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > Changes in v2: > - rename to LOWEST_BIT > --- > automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ++++++ > xen/include/xen/macros.h | 6 ++++-- > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl > index d8170106b449..b8e1155ee49d 100644 > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl > @@ -274,6 +274,12 @@ still non-negative." > -config=MC3R1.R10.1,etypes+={safe, "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))", "dst_type(ebool||boolean)"} > -doc_end > > +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to obtain the value > +2^ffs(x) for unsigned integers on two's complement architectures > +(all the architectures supported by Xen satisfy this requirement)." > +-config=MC3R1.R10.1,reports+={safe, "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"} > +-doc_end > + > ### Set 3 ### > > # > diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h > index d0caae7db298..af47179d1056 100644 > --- a/xen/include/xen/macros.h > +++ b/xen/include/xen/macros.h > @@ -8,8 +8,10 @@ > #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) > #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) > > -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) > -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) > +#define LOWEST_BIT(x) ((x) & -(x)) ... this is not reflected in the name of the macro. So it is not obvious if it will return the lowest bit set or clear. Can you at least add a comment on top explaining what it returns? Something like: /* Return the lowest bit set */ Cheers,
On 13/10/2023 01:31, Stefano Stabellini wrote: > On Thu, 12 Oct 2023, Nicola Vetrini wrote: >> The purpose of this macro is to encapsulate the well-known expression >> 'x & -x', that in 2's complement architectures on unsigned integers >> will >> give 2^ffs(x), where ffs(x) is the position of the lowest set bit in >> x. >> >> A deviation for ECLAIR is also introduced. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> Changes in v2: >> - rename to LOWEST_BIT >> --- >> automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ++++++ >> xen/include/xen/macros.h | 6 ++++-- >> 2 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl >> b/automation/eclair_analysis/ECLAIR/deviations.ecl >> index d8170106b449..b8e1155ee49d 100644 >> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >> @@ -274,6 +274,12 @@ still non-negative." >> -config=MC3R1.R10.1,etypes+={safe, >> "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))", >> "dst_type(ebool||boolean)"} >> -doc_end >> >> +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to >> obtain the value >> +2^ffs(x) for unsigned integers on two's complement architectures >> +(all the architectures supported by Xen satisfy this requirement)." >> +-config=MC3R1.R10.1,reports+={safe, >> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"} >> +-doc_end > > Please also add the same deviation and explanation to > docs/misra/deviations.rst. Other than that, this looks fine. > > Ok.
On 13/10/2023 10:25, Julien Grall wrote: > Hi Nicola, > > On 12/10/2023 16:28, Nicola Vetrini wrote: >> The purpose of this macro is to encapsulate the well-known expression >> 'x & -x', that in 2's complement architectures on unsigned integers >> will >> give 2^ffs(x), where ffs(x) is the position of the lowest set bit in >> x. > > In the commit message it is clear that the macro will return the > lowest set bit. But... > >> >> A deviation for ECLAIR is also introduced. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> Changes in v2: >> - rename to LOWEST_BIT >> --- >> automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ++++++ >> xen/include/xen/macros.h | 6 ++++-- >> 2 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl >> b/automation/eclair_analysis/ECLAIR/deviations.ecl >> index d8170106b449..b8e1155ee49d 100644 >> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >> @@ -274,6 +274,12 @@ still non-negative." >> -config=MC3R1.R10.1,etypes+={safe, >> "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))", >> "dst_type(ebool||boolean)"} >> -doc_end >> >> +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to >> obtain the value >> +2^ffs(x) for unsigned integers on two's complement architectures >> +(all the architectures supported by Xen satisfy this requirement)." >> +-config=MC3R1.R10.1,reports+={safe, >> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"} >> +-doc_end >> + >> ### Set 3 ### >> >> # >> diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h >> index d0caae7db298..af47179d1056 100644 >> --- a/xen/include/xen/macros.h >> +++ b/xen/include/xen/macros.h >> @@ -8,8 +8,10 @@ >> #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) >> #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) >> >> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) >> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) >> +#define LOWEST_BIT(x) ((x) & -(x)) > > ... this is not reflected in the name of the macro. So it is not > obvious if it will return the lowest bit set or clear. > > Can you at least add a comment on top explaining what it returns? > Something like: > > /* Return the lowest bit set */ > > Cheers, No problem
On 12.10.2023 17:28, Nicola Vetrini wrote: > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl > @@ -274,6 +274,12 @@ still non-negative." > -config=MC3R1.R10.1,etypes+={safe, "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))", "dst_type(ebool||boolean)"} > -doc_end > > +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to obtain the value > +2^ffs(x) for unsigned integers on two's complement architectures > +(all the architectures supported by Xen satisfy this requirement)." > +-config=MC3R1.R10.1,reports+={safe, "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"} > +-doc_end Why is this added here rather than by ... > --- a/xen/include/xen/macros.h > +++ b/xen/include/xen/macros.h > @@ -8,8 +8,10 @@ > #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) > #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) > > -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) > -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) a SAF-<n>-safe comment here? > +#define LOWEST_BIT(x) ((x) & -(x)) Personally I consider the name misleading: I'd expect a macro of this name to return a bit number, not a mask with a single bit set. No good, reasonably short name comes to mind - ISOLATE_LOW_BIT() is too long for my taste. Jan
On 16/10/2023 17:33, Jan Beulich wrote: > On 12.10.2023 17:28, Nicola Vetrini wrote: >> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >> @@ -274,6 +274,12 @@ still non-negative." >> -config=MC3R1.R10.1,etypes+={safe, >> "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))", >> "dst_type(ebool||boolean)"} >> -doc_end >> >> +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to >> obtain the value >> +2^ffs(x) for unsigned integers on two's complement architectures >> +(all the architectures supported by Xen satisfy this requirement)." >> +-config=MC3R1.R10.1,reports+={safe, >> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"} >> +-doc_end > > Why is this added here rather than by ... > >> --- a/xen/include/xen/macros.h >> +++ b/xen/include/xen/macros.h >> @@ -8,8 +8,10 @@ >> #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) >> #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) >> >> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) >> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) > > a SAF-<n>-safe comment here? > One reason is that now that violations only belonging to tool configurations and similar are documented in docs/misra/deviations.rst (committed in Stefano's branch for-4.19 [1]). Also, there were disagreements on the SAF naming scheme, and patches like those would not be accepted at the moment. >> +#define LOWEST_BIT(x) ((x) & -(x)) > > Personally I consider the name misleading: I'd expect a macro of this > name to return a bit number, not a mask with a single bit set. No > good, reasonably short name comes to mind - ISOLATE_LOW_BIT() is too > long for my taste. > > Jan [1] https://gitlab.com/xen-project/people/sstabellini/xen/-/commits/for-4.19
On 16.10.2023 18:17, Nicola Vetrini wrote: > On 16/10/2023 17:33, Jan Beulich wrote: >> On 12.10.2023 17:28, Nicola Vetrini wrote: >>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >>> @@ -274,6 +274,12 @@ still non-negative." >>> -config=MC3R1.R10.1,etypes+={safe, >>> "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))", >>> "dst_type(ebool||boolean)"} >>> -doc_end >>> >>> +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to >>> obtain the value >>> +2^ffs(x) for unsigned integers on two's complement architectures >>> +(all the architectures supported by Xen satisfy this requirement)." >>> +-config=MC3R1.R10.1,reports+={safe, >>> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"} >>> +-doc_end >> >> Why is this added here rather than by ... >> >>> --- a/xen/include/xen/macros.h >>> +++ b/xen/include/xen/macros.h >>> @@ -8,8 +8,10 @@ >>> #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) >>> #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) >>> >>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) >>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) >> >> a SAF-<n>-safe comment here? >> > > One reason is that now that violations only belonging to tool > configurations > and similar are documented in docs/misra/deviations.rst (committed in > Stefano's > branch for-4.19 [1]). But tool configuration means every analysis tool needs configuring separately. That's why the comment tagging scheme was decided to be preferred, iirc. > Also, there were disagreements on the SAF naming > scheme, and > patches like those would not be accepted at the moment. Well, that needs resolving. The naming there shouldn't lead to patches being accepted that later may need redoing. Jan
On Mon, 16 Oct 2023, Jan Beulich wrote: > On 16.10.2023 18:17, Nicola Vetrini wrote: > > On 16/10/2023 17:33, Jan Beulich wrote: > >> On 12.10.2023 17:28, Nicola Vetrini wrote: > >>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl > >>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl > >>> @@ -274,6 +274,12 @@ still non-negative." > >>> -config=MC3R1.R10.1,etypes+={safe, > >>> "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))", > >>> "dst_type(ebool||boolean)"} > >>> -doc_end > >>> > >>> +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to > >>> obtain the value > >>> +2^ffs(x) for unsigned integers on two's complement architectures > >>> +(all the architectures supported by Xen satisfy this requirement)." > >>> +-config=MC3R1.R10.1,reports+={safe, > >>> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"} > >>> +-doc_end > >> > >> Why is this added here rather than by ... > >> > >>> --- a/xen/include/xen/macros.h > >>> +++ b/xen/include/xen/macros.h > >>> @@ -8,8 +8,10 @@ > >>> #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) > >>> #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) > >>> > >>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) > >>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) > >> > >> a SAF-<n>-safe comment here? > >> > > > > One reason is that now that violations only belonging to tool > > configurations > > and similar are documented in docs/misra/deviations.rst (committed in > > Stefano's > > branch for-4.19 [1]). > > But tool configuration means every analysis tool needs configuring > separately. That's why the comment tagging scheme was decided to be > preferred, iirc. > > > Also, there were disagreements on the SAF naming > > scheme, and > > patches like those would not be accepted at the moment. > > Well, that needs resolving. The naming there shouldn't lead to patches > being accepted that later may need redoing. I agree
On 16/10/2023 18:30, Jan Beulich wrote: > On 16.10.2023 18:17, Nicola Vetrini wrote: >> On 16/10/2023 17:33, Jan Beulich wrote: >>> On 12.10.2023 17:28, Nicola Vetrini wrote: >>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >>>> @@ -274,6 +274,12 @@ still non-negative." >>>> -config=MC3R1.R10.1,etypes+={safe, >>>> "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))", >>>> "dst_type(ebool||boolean)"} >>>> -doc_end >>>> >>>> +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern >>>> to >>>> obtain the value >>>> +2^ffs(x) for unsigned integers on two's complement architectures >>>> +(all the architectures supported by Xen satisfy this requirement)." >>>> +-config=MC3R1.R10.1,reports+={safe, >>>> "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"} >>>> +-doc_end >>> >>> Why is this added here rather than by ... >>> >>>> --- a/xen/include/xen/macros.h >>>> +++ b/xen/include/xen/macros.h >>>> @@ -8,8 +8,10 @@ >>>> #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) >>>> #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) >>>> >>>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) >>>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) >>> >>> a SAF-<n>-safe comment here? >>> >> >> One reason is that now that violations only belonging to tool >> configurations >> and similar are documented in docs/misra/deviations.rst (committed in >> Stefano's >> branch for-4.19 [1]). > > But tool configuration means every analysis tool needs configuring > separately. That's why the comment tagging scheme was decided to be > preferred, iirc. > >> Also, there were disagreements on the SAF naming >> scheme, and >> patches like those would not be accepted at the moment. > > Well, that needs resolving. The naming there shouldn't lead to patches > being accepted that later may need redoing. > > Jan While this is true, in this case I'm not willing to deviate with a SAF, given that some ECLAIR-specific configuration would be needed anyways, given that I'm deviating a macro definition, rather than the line where it's actually used (and maybe other tools would need that as well).
+Luca > > > > > --- a/xen/include/xen/macros.h > > > > > +++ b/xen/include/xen/macros.h > > > > > @@ -8,8 +8,10 @@ > > > > > #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) > > > > > #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) > > > > > > > > > > -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) > > > > > -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) > > > > > > > > a SAF-<n>-safe comment here? > > > > > > > > > > One reason is that now that violations only belonging to tool > > > configurations > > > and similar are documented in docs/misra/deviations.rst (committed in > > > Stefano's > > > branch for-4.19 [1]). > > > > But tool configuration means every analysis tool needs configuring > > separately. That's why the comment tagging scheme was decided to be > > preferred, iirc. > > > > > Also, there were disagreements on the SAF naming > > > scheme, and > > > patches like those would not be accepted at the moment. > > > > Well, that needs resolving. The naming there shouldn't lead to patches > > being accepted that later may need redoing. > > > > Jan > > While this is true, in this case I'm not willing to deviate with a SAF, given > that > some ECLAIR-specific configuration would be needed anyways, given that I'm > deviating a macro definition, rather than the line where it's actually used > (and maybe other tools would need > that as well). Did I get it right that the problem with using SAF in this case is that it wouldn't be sufficient to add a SAF comment on top of the MACRO definition, but we would need a SAF comment on top of every MACRO invocation? If so, then not just for this MACRO but in general basically we have to use deviations.rst. Luca, do you know what would be the behavior for cppcheck and/or Coverity? I imagine it will be the same and they would also need a deviation at every MACRO invocation, not just the definition?
On 19.10.2023 21:58, Stefano Stabellini wrote: >>>>>> --- a/xen/include/xen/macros.h >>>>>> +++ b/xen/include/xen/macros.h >>>>>> @@ -8,8 +8,10 @@ >>>>>> #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) >>>>>> #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) >>>>>> >>>>>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) >>>>>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) >>>>> >>>>> a SAF-<n>-safe comment here? >>>>> >>>> >>>> One reason is that now that violations only belonging to tool >>>> configurations >>>> and similar are documented in docs/misra/deviations.rst (committed in >>>> Stefano's >>>> branch for-4.19 [1]). >>> >>> But tool configuration means every analysis tool needs configuring >>> separately. That's why the comment tagging scheme was decided to be >>> preferred, iirc. >>> >>>> Also, there were disagreements on the SAF naming >>>> scheme, and >>>> patches like those would not be accepted at the moment. >>> >>> Well, that needs resolving. The naming there shouldn't lead to patches >>> being accepted that later may need redoing. >>> >>> Jan >> >> While this is true, in this case I'm not willing to deviate with a SAF, given >> that >> some ECLAIR-specific configuration would be needed anyways, given that I'm >> deviating a macro definition, rather than the line where it's actually used >> (and maybe other tools would need >> that as well). > > Did I get it right that the problem with using SAF in this case is that > it wouldn't be sufficient to add a SAF comment on top of the MACRO > definition, but we would need a SAF comment on top of every MACRO > invocation? > > If so, then not just for this MACRO but in general basically we have to > use deviations.rst. That would be pretty sad. Jan > Luca, do you know what would be the behavior for cppcheck and/or > Coverity? I imagine it will be the same and they would also need a > deviation at every MACRO invocation, not just the definition?
On 20/10/2023 08:00, Jan Beulich wrote: > On 19.10.2023 21:58, Stefano Stabellini wrote: >>>>>>> --- a/xen/include/xen/macros.h >>>>>>> +++ b/xen/include/xen/macros.h >>>>>>> @@ -8,8 +8,10 @@ >>>>>>> #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) >>>>>>> #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) >>>>>>> >>>>>>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) >>>>>>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) >>>>>> >>>>>> a SAF-<n>-safe comment here? >>>>>> >>>>> >>>>> One reason is that now that violations only belonging to tool >>>>> configurations >>>>> and similar are documented in docs/misra/deviations.rst (committed >>>>> in >>>>> Stefano's >>>>> branch for-4.19 [1]). >>>> >>>> But tool configuration means every analysis tool needs configuring >>>> separately. That's why the comment tagging scheme was decided to be >>>> preferred, iirc. >>>> >>>>> Also, there were disagreements on the SAF naming >>>>> scheme, and >>>>> patches like those would not be accepted at the moment. >>>> >>>> Well, that needs resolving. The naming there shouldn't lead to >>>> patches >>>> being accepted that later may need redoing. >>>> >>>> Jan >>> >>> While this is true, in this case I'm not willing to deviate with a >>> SAF, given >>> that >>> some ECLAIR-specific configuration would be needed anyways, given >>> that I'm >>> deviating a macro definition, rather than the line where it's >>> actually used >>> (and maybe other tools would need >>> that as well). >> >> Did I get it right that the problem with using SAF in this case is >> that >> it wouldn't be sufficient to add a SAF comment on top of the MACRO >> definition, but we would need a SAF comment on top of every MACRO >> invocation? >> >> If so, then not just for this MACRO but in general basically we have >> to >> use deviations.rst. > > That would be pretty sad. > > Jan > Local deviation comments are for local deviations; deviating patterns is a tool configuration. >> Luca, do you know what would be the behavior for cppcheck and/or >> Coverity? I imagine it will be the same and they would also need a >> deviation at every MACRO invocation, not just the definition?
> On 19 Oct 2023, at 20:58, Stefano Stabellini <sstabellini@kernel.org> wrote: > > +Luca > >>>>>> --- a/xen/include/xen/macros.h >>>>>> +++ b/xen/include/xen/macros.h >>>>>> @@ -8,8 +8,10 @@ >>>>>> #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) >>>>>> #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) >>>>>> >>>>>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) >>>>>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) >>>>> >>>>> a SAF-<n>-safe comment here? >>>>> >>>> >>>> One reason is that now that violations only belonging to tool >>>> configurations >>>> and similar are documented in docs/misra/deviations.rst (committed in >>>> Stefano's >>>> branch for-4.19 [1]). >>> >>> But tool configuration means every analysis tool needs configuring >>> separately. That's why the comment tagging scheme was decided to be >>> preferred, iirc. >>> >>>> Also, there were disagreements on the SAF naming >>>> scheme, and >>>> patches like those would not be accepted at the moment. >>> >>> Well, that needs resolving. The naming there shouldn't lead to patches >>> being accepted that later may need redoing. >>> >>> Jan >> >> While this is true, in this case I'm not willing to deviate with a SAF, given >> that >> some ECLAIR-specific configuration would be needed anyways, given that I'm >> deviating a macro definition, rather than the line where it's actually used >> (and maybe other tools would need >> that as well). > > Did I get it right that the problem with using SAF in this case is that > it wouldn't be sufficient to add a SAF comment on top of the MACRO > definition, but we would need a SAF comment on top of every MACRO > invocation? > > If so, then not just for this MACRO but in general basically we have to > use deviations.rst. > > Luca, do you know what would be the behavior for cppcheck and/or > Coverity? I imagine it will be the same and they would also need a > deviation at every MACRO invocation, not just the definition? Seems that cppcheck reports at the macro definition, instead Coverity reports at the macro invocation.
On 20.10.2023 12:40, Nicola Vetrini wrote: > On 20/10/2023 08:00, Jan Beulich wrote: >> On 19.10.2023 21:58, Stefano Stabellini wrote: >>>>>>>> --- a/xen/include/xen/macros.h >>>>>>>> +++ b/xen/include/xen/macros.h >>>>>>>> @@ -8,8 +8,10 @@ >>>>>>>> #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) >>>>>>>> #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) >>>>>>>> >>>>>>>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) >>>>>>>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) >>>>>>> >>>>>>> a SAF-<n>-safe comment here? >>>>>>> >>>>>> >>>>>> One reason is that now that violations only belonging to tool >>>>>> configurations >>>>>> and similar are documented in docs/misra/deviations.rst (committed >>>>>> in >>>>>> Stefano's >>>>>> branch for-4.19 [1]). >>>>> >>>>> But tool configuration means every analysis tool needs configuring >>>>> separately. That's why the comment tagging scheme was decided to be >>>>> preferred, iirc. >>>>> >>>>>> Also, there were disagreements on the SAF naming >>>>>> scheme, and >>>>>> patches like those would not be accepted at the moment. >>>>> >>>>> Well, that needs resolving. The naming there shouldn't lead to >>>>> patches >>>>> being accepted that later may need redoing. >>>>> >>>>> Jan >>>> >>>> While this is true, in this case I'm not willing to deviate with a >>>> SAF, given >>>> that >>>> some ECLAIR-specific configuration would be needed anyways, given >>>> that I'm >>>> deviating a macro definition, rather than the line where it's >>>> actually used >>>> (and maybe other tools would need >>>> that as well). >>> >>> Did I get it right that the problem with using SAF in this case is >>> that >>> it wouldn't be sufficient to add a SAF comment on top of the MACRO >>> definition, but we would need a SAF comment on top of every MACRO >>> invocation? >>> >>> If so, then not just for this MACRO but in general basically we have >>> to >>> use deviations.rst. >> >> That would be pretty sad. > > Local deviation comments are for local deviations; deviating patterns is > a tool configuration. That's orthogonal. A deviating comment on a macro definition, when it is about an aspect that's meaningful only after the macro is expanded (i.e. not violating some rule concerning macro definitions only), would be quite helpful to limit the number of such comments that need sprinkling across the code base. Jan
On Fri, 20 Oct 2023, Luca Fancellu wrote: > > On 19 Oct 2023, at 20:58, Stefano Stabellini <sstabellini@kernel.org> wrote: > > +Luca > > > >>>>>> --- a/xen/include/xen/macros.h > >>>>>> +++ b/xen/include/xen/macros.h > >>>>>> @@ -8,8 +8,10 @@ > >>>>>> #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) > >>>>>> #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) > >>>>>> > >>>>>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) > >>>>>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) > >>>>> > >>>>> a SAF-<n>-safe comment here? > >>>>> > >>>> > >>>> One reason is that now that violations only belonging to tool > >>>> configurations > >>>> and similar are documented in docs/misra/deviations.rst (committed in > >>>> Stefano's > >>>> branch for-4.19 [1]). > >>> > >>> But tool configuration means every analysis tool needs configuring > >>> separately. That's why the comment tagging scheme was decided to be > >>> preferred, iirc. > >>> > >>>> Also, there were disagreements on the SAF naming > >>>> scheme, and > >>>> patches like those would not be accepted at the moment. > >>> > >>> Well, that needs resolving. The naming there shouldn't lead to patches > >>> being accepted that later may need redoing. > >>> > >>> Jan > >> > >> While this is true, in this case I'm not willing to deviate with a SAF, given > >> that > >> some ECLAIR-specific configuration would be needed anyways, given that I'm > >> deviating a macro definition, rather than the line where it's actually used > >> (and maybe other tools would need > >> that as well). > > > > Did I get it right that the problem with using SAF in this case is that > > it wouldn't be sufficient to add a SAF comment on top of the MACRO > > definition, but we would need a SAF comment on top of every MACRO > > invocation? > > > > If so, then not just for this MACRO but in general basically we have to > > use deviations.rst. > > > > Luca, do you know what would be the behavior for cppcheck and/or > > Coverity? I imagine it will be the same and they would also need a > > deviation at every MACRO invocation, not just the definition? > > Seems that cppcheck reports at the macro definition, instead Coverity reports > at the macro invocation. Does that mean that a /* SAF-xx-safe */ comment would work at the MACRO definition for cppcheck but not for Coverity? If so, then I wonder if cppcheck's behavior is what we would like to have, but from a code compliance point of view it is the least reliable, so that's the reason why both Coverity and ECLAIR don't implement it. In terms of correctness of the implementation we know cppcheck is lagging a bit behind...
> On 20 Oct 2023, at 19:07, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Fri, 20 Oct 2023, Luca Fancellu wrote: >>> On 19 Oct 2023, at 20:58, Stefano Stabellini <sstabellini@kernel.org> wrote: >>> +Luca >>> >>>>>>>> --- a/xen/include/xen/macros.h >>>>>>>> +++ b/xen/include/xen/macros.h >>>>>>>> @@ -8,8 +8,10 @@ >>>>>>>> #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) >>>>>>>> #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) >>>>>>>> >>>>>>>> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) >>>>>>>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) >>>>>>> >>>>>>> a SAF-<n>-safe comment here? >>>>>>> >>>>>> >>>>>> One reason is that now that violations only belonging to tool >>>>>> configurations >>>>>> and similar are documented in docs/misra/deviations.rst (committed in >>>>>> Stefano's >>>>>> branch for-4.19 [1]). >>>>> >>>>> But tool configuration means every analysis tool needs configuring >>>>> separately. That's why the comment tagging scheme was decided to be >>>>> preferred, iirc. >>>>> >>>>>> Also, there were disagreements on the SAF naming >>>>>> scheme, and >>>>>> patches like those would not be accepted at the moment. >>>>> >>>>> Well, that needs resolving. The naming there shouldn't lead to patches >>>>> being accepted that later may need redoing. >>>>> >>>>> Jan >>>> >>>> While this is true, in this case I'm not willing to deviate with a SAF, given >>>> that >>>> some ECLAIR-specific configuration would be needed anyways, given that I'm >>>> deviating a macro definition, rather than the line where it's actually used >>>> (and maybe other tools would need >>>> that as well). >>> >>> Did I get it right that the problem with using SAF in this case is that >>> it wouldn't be sufficient to add a SAF comment on top of the MACRO >>> definition, but we would need a SAF comment on top of every MACRO >>> invocation? >>> >>> If so, then not just for this MACRO but in general basically we have to >>> use deviations.rst. >>> >>> Luca, do you know what would be the behavior for cppcheck and/or >>> Coverity? I imagine it will be the same and they would also need a >>> deviation at every MACRO invocation, not just the definition? >> >> Seems that cppcheck reports at the macro definition, instead Coverity reports >> at the macro invocation. > > Does that mean that a /* SAF-xx-safe */ comment would work at the MACRO > definition for cppcheck but not for Coverity? > > If so, then I wonder if cppcheck's behavior is what we would like to > have, but from a code compliance point of view it is the least reliable, > so that's the reason why both Coverity and ECLAIR don't implement it. In > terms of correctness of the implementation we know cppcheck is lagging a > bit behind... Yes, I’m starting to think that if we want to deviate a large number of macro usage, We should come up with something like declaring the macro somewhere and adding the in-code comment automatically by the script before the analysis... But we need to see how feasible it is.
diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl index d8170106b449..b8e1155ee49d 100644 --- a/automation/eclair_analysis/ECLAIR/deviations.ecl +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl @@ -274,6 +274,12 @@ still non-negative." -config=MC3R1.R10.1,etypes+={safe, "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))", "dst_type(ebool||boolean)"} -doc_end +-doc_begin="The macro LOWEST_BIT encapsulates a well-known pattern to obtain the value +2^ffs(x) for unsigned integers on two's complement architectures +(all the architectures supported by Xen satisfy this requirement)." +-config=MC3R1.R10.1,reports+={safe, "any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"} +-doc_end + ### Set 3 ### # diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h index d0caae7db298..af47179d1056 100644 --- a/xen/include/xen/macros.h +++ b/xen/include/xen/macros.h @@ -8,8 +8,10 @@ #define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) #define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) +#define LOWEST_BIT(x) ((x) & -(x)) + +#define MASK_EXTR(v, m) (((v) & (m)) / LOWEST_BIT(m)) +#define MASK_INSR(v, m) (((v) * LOWEST_BIT(m)) & (m)) #define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x #define count_args(args...) \
The purpose of this macro is to encapsulate the well-known expression 'x & -x', that in 2's complement architectures on unsigned integers will give 2^ffs(x), where ffs(x) is the position of the lowest set bit in x. A deviation for ECLAIR is also introduced. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- Changes in v2: - rename to LOWEST_BIT --- automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ++++++ xen/include/xen/macros.h | 6 ++++-- 2 files changed, 10 insertions(+), 2 deletions(-) -- 2.34.1