| Message ID | c684c36402e6740472fa91d73436ca5790e5e109.1696948320.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | address violations of MISRA C:2012 Rule 11.9 | expand |
On 11/10/2023 8:46 pm, Nicola Vetrini wrote: > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst > index ee7aed0609d2..1b00e4e3e9b7 100644 > --- a/docs/misra/deviations.rst > +++ b/docs/misra/deviations.rst > @@ -199,6 +199,11 @@ Deviations related to MISRA C:2012 Rules: > See automation/eclair_analysis/deviations.ecl for the full explanation. > - Tagged as `safe` for ECLAIR. > > + * - R11.9 > + - __ACCESS_ONCE uses a 0 as a null pointer constant to check if a type is > + scalar, therefore its usage for this purpose is allowed. > + - Tagged as `deliberate` for ECLAIR. Really? #define __ACCESS_ONCE(x) (void)(typeof(x))0; /* Scalar typecheck. */ That's not a pointer. If someone were to pass in an x who's type was pointer-to-object, then yes it is technically a NULL pointer constant for long enough for the build to error out. What justification is Eclair using to suggest that it is a pointer? If you really really want to shut things up, it doesn't need to be 0 - it could be 1 or any other integer, but this honestly feels like a bug in Eclair to me. ~Andrew
On 11/10/2023 18:56, andrew.cooper3@citrix.com wrote: > On 11/10/2023 8:46 pm, Nicola Vetrini wrote: >> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst >> index ee7aed0609d2..1b00e4e3e9b7 100644 >> --- a/docs/misra/deviations.rst >> +++ b/docs/misra/deviations.rst >> @@ -199,6 +199,11 @@ Deviations related to MISRA C:2012 Rules: >> See automation/eclair_analysis/deviations.ecl for the full >> explanation. >> - Tagged as `safe` for ECLAIR. >> >> + * - R11.9 >> + - __ACCESS_ONCE uses a 0 as a null pointer constant to check if >> a type is >> + scalar, therefore its usage for this purpose is allowed. >> + - Tagged as `deliberate` for ECLAIR. > > Really? > > #define __ACCESS_ONCE(x) > (void)(typeof(x))0; /* Scalar typecheck. */ > > That's not a pointer. > > If someone were to pass in an x who's type was pointer-to-object, then > yes it is technically a NULL pointer constant for long enough for the > build to error out. > > What justification is Eclair using to suggest that it is a pointer? > > If you really really want to shut things up, it doesn't need to be 0 - > it could be 1 or any other integer, but this honestly feels like a bug > in Eclair to me. > > ~Andrew The non-compliant uses found by the checker were due to function pointers e.g. void (*fp)(int i); violation for rule MC3R1.R11.9: (required) The macro `NULL' shall be the only permitted form of integer null pointer constant. (untagged) p.c:12.3-12.7: (MACRO) Loc #1 [culprit: non-compliant use of null pointer constant] A(fp) = NULL; <~~~> p.c:6.8-6.19: for #1 [culprit: expanded from macro `_A'] (void)(typeof(X))0; \ <~~~~~~~~~~> p.c:9.16-9.20: for #1 [culprit: expanded from macro `A'] #define A(X) (*_A(X)) <~~~> These uses do not cause a build fail, and we deemed this usage of 0 to be correct (a neutral value that would allow __ACCESS_ONCE to support any type of argument). While perhaps some other value does have the same property (e.g., 1), we felt that it was okay to let 0 remain there.
On Wed, 11 Oct 2023, Nicola Vetrini wrote: > The constant 0 is used instead of NULL in '__ACCESS_ONCE' as a > compile-time check to detect non-scalar types; its usage for this > purpose is deviated. > > Furthermore, the 'access_field' and 'typeof_field' macros are > introduced as a general way to deal with accesses to structs > without declaring a struct variable. > > No functional change intended. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > Changes in v2: > - added entry in deviations.rst > --- > automation/eclair_analysis/ECLAIR/deviations.ecl | 9 +++++++++ > docs/misra/deviations.rst | 5 +++++ > xen/include/xen/compiler.h | 5 ++++- > xen/include/xen/kernel.h | 2 +- > 4 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl > index fa56e5c00a27..28d9c37bedb2 100644 > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl > @@ -246,6 +246,15 @@ constant expressions are required.\"" > "any()"} > -doc_end > > +# > +# Series 11 > +# > + > +-doc_begin="This construct is used to check if the type is scalar, and for this purpose the use of 0 as a null pointer constant is deliberate." > +-config=MC3R1.R11.9,reports+={deliberate, "any_area(any_loc(any_exp(macro(^__ACCESS_ONCE$))))" > +} > +-doc_end > + > # > # Series 13 > # > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst > index ee7aed0609d2..1b00e4e3e9b7 100644 > --- a/docs/misra/deviations.rst > +++ b/docs/misra/deviations.rst > @@ -199,6 +199,11 @@ Deviations related to MISRA C:2012 Rules: > See automation/eclair_analysis/deviations.ecl for the full explanation. > - Tagged as `safe` for ECLAIR. > > + * - R11.9 > + - __ACCESS_ONCE uses a 0 as a null pointer constant to check if a type is > + scalar, therefore its usage for this purpose is allowed. > + - Tagged as `deliberate` for ECLAIR. > + > * - R13.5 > - All developers and reviewers can be safely assumed to be well aware of > the short-circuit evaluation strategy for logical operators. > diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h > index dd99e573083f..15be9a750b23 100644 > --- a/xen/include/xen/compiler.h > +++ b/xen/include/xen/compiler.h > @@ -109,13 +109,16 @@ > > #define offsetof(a,b) __builtin_offsetof(a,b) > > +/* Access the field of structure type, without defining a local variable */ > +#define access_field(type, member) (((type *)NULL)->member) > +#define typeof_field(type, member) typeof(access_field(type, member)) > /** > * sizeof_field(TYPE, MEMBER) > * > * @TYPE: The structure containing the field of interest > * @MEMBER: The field to return the size of > */ > -#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER)) > +#define sizeof_field(TYPE, MEMBER) sizeof(access_field(TYPE, MEMBER)) > > #if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L > #define alignof __alignof__ > diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h > index 46b3c9c02625..2c5ed7736c99 100644 > --- a/xen/include/xen/kernel.h > +++ b/xen/include/xen/kernel.h > @@ -51,7 +51,7 @@ > * > */ > #define container_of(ptr, type, member) ({ \ > - typeof( ((type *)0)->member ) *__mptr = (ptr); \ > + typeof_field(type, member) *__mptr = (ptr); \ > (type *)( (char *)__mptr - offsetof(type,member) );}) > > /* > -- > 2.34.1 >
On 11.10.2023 14:46, Nicola Vetrini wrote: > --- a/xen/include/xen/compiler.h > +++ b/xen/include/xen/compiler.h > @@ -109,13 +109,16 @@ > > #define offsetof(a,b) __builtin_offsetof(a,b) > > +/* Access the field of structure type, without defining a local variable */ > +#define access_field(type, member) (((type *)NULL)->member) This is not a field access, so I consider the macro misnamed. Question is whether such a helper macro is needed in the first place. > +#define typeof_field(type, member) typeof(access_field(type, member)) If this needs adding, it wants to come ... > /** > * sizeof_field(TYPE, MEMBER) > * > * @TYPE: The structure containing the field of interest > * @MEMBER: The field to return the size of > */ > -#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER)) > +#define sizeof_field(TYPE, MEMBER) sizeof(access_field(TYPE, MEMBER)) ... with a commend similar as this one has. (Or the commend could be slightly altered to cover both). Further, if fiddling with these: Wouldn't they better move to macros.h? Jan
On 11.10.2023 18:56, andrew.cooper3@citrix.com wrote: > On 11/10/2023 8:46 pm, Nicola Vetrini wrote: >> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst >> index ee7aed0609d2..1b00e4e3e9b7 100644 >> --- a/docs/misra/deviations.rst >> +++ b/docs/misra/deviations.rst >> @@ -199,6 +199,11 @@ Deviations related to MISRA C:2012 Rules: >> See automation/eclair_analysis/deviations.ecl for the full explanation. >> - Tagged as `safe` for ECLAIR. >> >> + * - R11.9 >> + - __ACCESS_ONCE uses a 0 as a null pointer constant to check if a type is >> + scalar, therefore its usage for this purpose is allowed. >> + - Tagged as `deliberate` for ECLAIR. > > Really? > > #define __ACCESS_ONCE(x) > (void)(typeof(x))0; /* Scalar typecheck. */ > > That's not a pointer. > > If someone were to pass in an x who's type was pointer-to-object, then > yes it is technically a NULL pointer constant for long enough for the > build to error out. Why would it error out? ACCESS_ONCE() on a variable of pointer type is as okay as one on a variable of (some kind of) integer type. (Sadly the check as is would even let floating point types pass. At the same time the check is too strict to allow e.g. single-machine-word struct/union to also pass.) Jan
On 16/10/2023 15:43, Jan Beulich wrote: > On 11.10.2023 14:46, Nicola Vetrini wrote: >> --- a/xen/include/xen/compiler.h >> +++ b/xen/include/xen/compiler.h >> @@ -109,13 +109,16 @@ >> >> #define offsetof(a,b) __builtin_offsetof(a,b) >> >> +/* Access the field of structure type, without defining a local >> variable */ >> +#define access_field(type, member) (((type *)NULL)->member) > > This is not a field access, so I consider the macro misnamed. Question > is > whether such a helper macro is needed in the first place. > >> +#define typeof_field(type, member) typeof(access_field(type, member)) > > If this needs adding, it wants to come ... > >> /** >> * sizeof_field(TYPE, MEMBER) >> * >> * @TYPE: The structure containing the field of interest >> * @MEMBER: The field to return the size of >> */ >> -#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER)) >> +#define sizeof_field(TYPE, MEMBER) sizeof(access_field(TYPE, MEMBER)) > > ... with a commend similar as this one has. (Or the commend could be > slightly altered to cover both). > I added access_field since it's possibly useful on its own in the future, but that may not be the case. Not a real field access, perhaps a fake_access_field? Ok about the missing comment for typeof_field. > Further, if fiddling with these: Wouldn't they better move to macros.h? > > Jan That seems a good suggestion, as they are not compiler-specific.
On 16.10.2023 18:49, Nicola Vetrini wrote: > On 16/10/2023 15:43, Jan Beulich wrote: >> On 11.10.2023 14:46, Nicola Vetrini wrote: >>> --- a/xen/include/xen/compiler.h >>> +++ b/xen/include/xen/compiler.h >>> @@ -109,13 +109,16 @@ >>> >>> #define offsetof(a,b) __builtin_offsetof(a,b) >>> >>> +/* Access the field of structure type, without defining a local >>> variable */ >>> +#define access_field(type, member) (((type *)NULL)->member) >> >> This is not a field access, so I consider the macro misnamed. Question >> is >> whether such a helper macro is needed in the first place. >> >>> +#define typeof_field(type, member) typeof(access_field(type, member)) >> >> If this needs adding, it wants to come ... >> >>> /** >>> * sizeof_field(TYPE, MEMBER) >>> * >>> * @TYPE: The structure containing the field of interest >>> * @MEMBER: The field to return the size of >>> */ >>> -#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER)) >>> +#define sizeof_field(TYPE, MEMBER) sizeof(access_field(TYPE, MEMBER)) >> >> ... with a commend similar as this one has. (Or the commend could be >> slightly altered to cover both). >> > > I added access_field since it's possibly useful on its own in the > future, > but that may not be the case. Not a real field access, perhaps a > fake_access_field? I don't like this either, I'm afraid: This isn't a fake access. Maybe field_of() or field_of_type()? Yet at this point I'd still prefer for this to not be a separate macro in the first place. Jan
On 17/10/2023 08:51, Jan Beulich wrote: > On 16.10.2023 18:49, Nicola Vetrini wrote: >> On 16/10/2023 15:43, Jan Beulich wrote: >>> On 11.10.2023 14:46, Nicola Vetrini wrote: >>>> --- a/xen/include/xen/compiler.h >>>> +++ b/xen/include/xen/compiler.h >>>> @@ -109,13 +109,16 @@ >>>> >>>> #define offsetof(a,b) __builtin_offsetof(a,b) >>>> >>>> +/* Access the field of structure type, without defining a local >>>> variable */ >>>> +#define access_field(type, member) (((type *)NULL)->member) >>> >>> This is not a field access, so I consider the macro misnamed. >>> Question >>> is >>> whether such a helper macro is needed in the first place. >>> >>>> +#define typeof_field(type, member) typeof(access_field(type, >>>> member)) >>> >>> If this needs adding, it wants to come ... >>> >>>> /** >>>> * sizeof_field(TYPE, MEMBER) >>>> * >>>> * @TYPE: The structure containing the field of interest >>>> * @MEMBER: The field to return the size of >>>> */ >>>> -#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER)) >>>> +#define sizeof_field(TYPE, MEMBER) sizeof(access_field(TYPE, >>>> MEMBER)) >>> >>> ... with a commend similar as this one has. (Or the commend could be >>> slightly altered to cover both). >>> >> >> I added access_field since it's possibly useful on its own in the >> future, >> but that may not be the case. Not a real field access, perhaps a >> fake_access_field? > > I don't like this either, I'm afraid: This isn't a fake access. Maybe > field_of() or field_of_type()? Yet at this point I'd still prefer for > this to not be a separate macro in the first place. > > Jan Ok, no problem.
diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl index fa56e5c00a27..28d9c37bedb2 100644 --- a/automation/eclair_analysis/ECLAIR/deviations.ecl +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl @@ -246,6 +246,15 @@ constant expressions are required.\"" "any()"} -doc_end +# +# Series 11 +# + +-doc_begin="This construct is used to check if the type is scalar, and for this purpose the use of 0 as a null pointer constant is deliberate." +-config=MC3R1.R11.9,reports+={deliberate, "any_area(any_loc(any_exp(macro(^__ACCESS_ONCE$))))" +} +-doc_end + # # Series 13 # diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst index ee7aed0609d2..1b00e4e3e9b7 100644 --- a/docs/misra/deviations.rst +++ b/docs/misra/deviations.rst @@ -199,6 +199,11 @@ Deviations related to MISRA C:2012 Rules: See automation/eclair_analysis/deviations.ecl for the full explanation. - Tagged as `safe` for ECLAIR. + * - R11.9 + - __ACCESS_ONCE uses a 0 as a null pointer constant to check if a type is + scalar, therefore its usage for this purpose is allowed. + - Tagged as `deliberate` for ECLAIR. + * - R13.5 - All developers and reviewers can be safely assumed to be well aware of the short-circuit evaluation strategy for logical operators. diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h index dd99e573083f..15be9a750b23 100644 --- a/xen/include/xen/compiler.h +++ b/xen/include/xen/compiler.h @@ -109,13 +109,16 @@ #define offsetof(a,b) __builtin_offsetof(a,b) +/* Access the field of structure type, without defining a local variable */ +#define access_field(type, member) (((type *)NULL)->member) +#define typeof_field(type, member) typeof(access_field(type, member)) /** * sizeof_field(TYPE, MEMBER) * * @TYPE: The structure containing the field of interest * @MEMBER: The field to return the size of */ -#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER)) +#define sizeof_field(TYPE, MEMBER) sizeof(access_field(TYPE, MEMBER)) #if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L #define alignof __alignof__ diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h index 46b3c9c02625..2c5ed7736c99 100644 --- a/xen/include/xen/kernel.h +++ b/xen/include/xen/kernel.h @@ -51,7 +51,7 @@ * */ #define container_of(ptr, type, member) ({ \ - typeof( ((type *)0)->member ) *__mptr = (ptr); \ + typeof_field(type, member) *__mptr = (ptr); \ (type *)( (char *)__mptr - offsetof(type,member) );}) /*
The constant 0 is used instead of NULL in '__ACCESS_ONCE' as a compile-time check to detect non-scalar types; its usage for this purpose is deviated. Furthermore, the 'access_field' and 'typeof_field' macros are introduced as a general way to deal with accesses to structs without declaring a struct variable. No functional change intended. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- Changes in v2: - added entry in deviations.rst --- automation/eclair_analysis/ECLAIR/deviations.ecl | 9 +++++++++ docs/misra/deviations.rst | 5 +++++ xen/include/xen/compiler.h | 5 ++++- xen/include/xen/kernel.h | 2 +- 4 files changed, 19 insertions(+), 2 deletions(-)