[XEN PATCH][for-4.19 v3] xen: address violations of Rule 11.9

[XEN PATCH][for-4.19 v3] xen: address violations of Rule 11.9

[Nicola Vetrini]

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(-)

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__ */
-- 
2.34.1

Re: [XEN PATCH][for-4.19 v3] xen: address violations of Rule 11.9

[Jan Beulich]

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>

Re: [XEN PATCH][for-4.19 v3] xen: address violations of Rule 11.9

[Stefano Stabellini]

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>

Re: [XEN PATCH][for-4.19 v3] xen: address violations of Rule 11.9

[andrew.cooper3]

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.

Re: [XEN PATCH][for-4.19 v3] xen: address violations of Rule 11.9

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1517 bytes --]

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.

Re: [XEN PATCH][for-4.19 v3] xen: address violations of Rule 11.9

[Jan Beulich]

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

Re: [XEN PATCH][for-4.19 v3] xen: address violations of Rule 11.9

[Nicola Vetrini]

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

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH][for-4.19 v3] xen: address violations of Rule 11.9

[andrew.cooper3]

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