| Message ID | d760ef0c088dc15ecc411b83640735123444f887.1697636446.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN,for-4.19,v3] xen: address violations of Rule 11.9 | expand |
On 18.10.2023 15:42, 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 'typeof_field' macro is introduced as a general way > to access the type of a struct member without declaring a variable > of struct type. Both this macro and 'sizeof_field' are moved to > 'xen/macros.h'. > > No functional change intended. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On Wed, 18 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 'typeof_field' macro is introduced as a general way > to access the type of a struct member without declaring a variable > of struct type. Both this macro and 'sizeof_field' are moved to > 'xen/macros.h'. > > No functional change intended. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
On 18/10/2023 2:42 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. This is still deeply misleading. There is an integer, which happens to be 0 but could be anything, used for a compile time typecheck[1]. In some cases this may be interpreted as a pointer constant, and is permitted for this purpose. ~Andrew [1] I know I wrote scalar typecheck in the comment, but I suspect that what I actually meant was non-compound-type typecheck.
On Thu, 19 Oct 2023, andrew.cooper3@citrix.com wrote: > On 18/10/2023 2:42 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. > > This is still deeply misleading. > > There is an integer, which happens to be 0 but could be anything, used > for a compile time typecheck[1]. In some cases this may be interpreted > as a pointer constant, and is permitted for this purpose. > > ~Andrew > > [1] I know I wrote scalar typecheck in the comment, but I suspect that > what I actually meant was non-compound-type typecheck. To help Nicola find the right wording do you have a concrete suggestion for the text to use? Reading your reply, I am guessing it would be: * - R11.9 - __ACCESS_ONCE uses an integer, which happens to be zero, as a non-compound-type typecheck. The typecheck uses a cast. The usage of zero or other integers for this purpose is allowed. Andrew, please confirm? Nicola, please also confirm that this version of the text would be suitable for an assessor.
On 19.10.2023 02:54, Stefano Stabellini wrote: > On Thu, 19 Oct 2023, andrew.cooper3@citrix.com wrote: >> On 18/10/2023 2:42 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. >> >> This is still deeply misleading. >> >> There is an integer, which happens to be 0 but could be anything, used >> for a compile time typecheck[1]. In some cases this may be interpreted >> as a pointer constant, and is permitted for this purpose. >> >> ~Andrew >> >> [1] I know I wrote scalar typecheck in the comment, but I suspect that >> what I actually meant was non-compound-type typecheck. > > To help Nicola find the right wording do you have a concrete suggestion > for the text to use? > > Reading your reply, I am guessing it would be: > > * - R11.9 > - __ACCESS_ONCE uses an integer, which happens to be zero, as a > non-compound-type typecheck. The typecheck uses a cast. The usage of > zero or other integers for this purpose is allowed. "non-compound" isn't correct either: __int128_t, for example, isn't a compound type but may not be used with ACCESS_ONCE(). Furthermore certain compound types are, as indicated earlier, in principle okay to use with ACCESS_ONCE(). Both are shortcomings of the present implementation, which imo shouldn't propagate into this document. I'd say just "as a compile time check". Jan
On 19/10/2023 09:03, Jan Beulich wrote: > On 19.10.2023 02:54, Stefano Stabellini wrote: >> On Thu, 19 Oct 2023, andrew.cooper3@citrix.com wrote: >>> On 18/10/2023 2:42 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. >>> >>> This is still deeply misleading. >>> >>> There is an integer, which happens to be 0 but could be anything, >>> used >>> for a compile time typecheck[1]. In some cases this may be >>> interpreted >>> as a pointer constant, and is permitted for this purpose. >>> >>> ~Andrew >>> >>> [1] I know I wrote scalar typecheck in the comment, but I suspect >>> that >>> what I actually meant was non-compound-type typecheck. >> >> To help Nicola find the right wording do you have a concrete >> suggestion >> for the text to use? >> >> Reading your reply, I am guessing it would be: >> >> * - R11.9 >> - __ACCESS_ONCE uses an integer, which happens to be zero, as a >> non-compound-type typecheck. The typecheck uses a cast. The usage >> of >> zero or other integers for this purpose is allowed. > > "non-compound" isn't correct either: __int128_t, for example, isn't a > compound type but may not be used with ACCESS_ONCE(). Furthermore > certain compound types are, as indicated earlier, in principle okay > to use with ACCESS_ONCE(). Both are shortcomings of the present > implementation, which imo shouldn't propagate into this document. I'd > say just "as a compile time check". > > Jan Ok, I'll amend it
On 19/10/2023 1:54 am, Stefano Stabellini wrote: > On Thu, 19 Oct 2023, andrew.cooper3@citrix.com wrote: >> On 18/10/2023 2:42 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. >> This is still deeply misleading. >> >> There is an integer, which happens to be 0 but could be anything, used >> for a compile time typecheck[1]. In some cases this may be interpreted >> as a pointer constant, and is permitted for this purpose. >> >> ~Andrew >> >> [1] I know I wrote scalar typecheck in the comment, but I suspect that >> what I actually meant was non-compound-type typecheck. > To help Nicola find the right wording do you have a concrete suggestion > for the text to use? > > Reading your reply, I am guessing it would be: > > * - R11.9 > - __ACCESS_ONCE uses an integer, which happens to be zero, as a > non-compound-type typecheck. The typecheck uses a cast. The usage of > zero or other integers for this purpose is allowed. > > Andrew, please confirm? Nicola, please also confirm that this version of > the text would be suitable for an assessor. Yes, that paragraph was intended as a specific suggestion, but I see I did miss __ACCESS_ONCE() off the start. Sorry. And yes - we should probably just leave it as "a compile time typecheck". Anything more specific isn't relevant here, and isn't trivial to describe either. ~Andrew
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..a8be1f19cfc2 100644 --- a/xen/include/xen/compiler.h +++ b/xen/include/xen/compiler.h @@ -109,14 +109,6 @@ #define offsetof(a,b) __builtin_offsetof(a,b) -/** - * 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)) - #if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L #define alignof __alignof__ #endif 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) );}) /* diff --git a/xen/include/xen/macros.h b/xen/include/xen/macros.h index d0caae7db298..457c84b9d1a0 100644 --- a/xen/include/xen/macros.h +++ b/xen/include/xen/macros.h @@ -54,6 +54,22 @@ #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]) + __must_be_array(x)) +/** + * typeof_field(type, member) + * + * @type: The structure containing the field of interest + * @member: The field whose type is returned + */ +#define typeof_field(type, member) typeof(((type *)NULL)->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 *)NULL)->member) + #endif /* __ASSEMBLY__ */ #endif /* __MACROS_H__ */
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 'typeof_field' macro is introduced as a general way to access the type of a struct member without declaring a variable of struct type. Both this macro and 'sizeof_field' are moved to 'xen/macros.h'. No functional change intended. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- Changes in v2: - added entry in deviations.rst Changes in v3: - dropped access_field - moved macro to macros.h --- automation/eclair_analysis/ECLAIR/deviations.ecl | 9 +++++++++ docs/misra/deviations.rst | 5 +++++ xen/include/xen/compiler.h | 8 -------- xen/include/xen/kernel.h | 2 +- xen/include/xen/macros.h | 16 ++++++++++++++++ 5 files changed, 31 insertions(+), 9 deletions(-)