[XEN PATCH][for-4.19 v2 0/1] address violations of MISRA C:2012 Rule 11.9

[XEN PATCH][for-4.19 v2 0/1] address violations of MISRA C:2012 Rule 11.9

[Nicola Vetrini]

Rule 11.9 forbids the usage of '0' as a null pointer constant, therefore uses of
this pattern have been amended. One exception, recorded in deviations.rst, is in
__ACCESS_ONCE to do a scalar type check.

Changes in v2:
- dropped patch 2, as it has already been committed to [1]
- rebased against [2] to insert the deviation record in the newly created
  deviations.rst

[1] https://gitlab.com/xen-project/people/andyhhp/xen/-/commits/for-next
[2] https://marc.info/?l=xen-devel&m=169686613720371&w=2

Nicola Vetrini (1):
  xen: introduce a deviation for Rule 11.9

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

--
2.34.1

[XEN PATCH][for-4.19 v2 1/1] xen: introduce a deviation for 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 '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(-)

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

Re: [XEN PATCH][for-4.19 v2 1/1] xen: introduce a deviation for Rule 11.9

[andrew.cooper3]

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

Re: [XEN PATCH][for-4.19 v2 1/1] xen: introduce a deviation for Rule 11.9

[Nicola Vetrini]

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.

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

Re: [XEN PATCH][for-4.19 v2 1/1] xen: introduce a deviation for Rule 11.9

[Jan Beulich]

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

Re: [XEN PATCH][for-4.19 v2 1/1] xen: introduce a deviation for Rule 11.9

[Stefano Stabellini]

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
> 

Re: [XEN PATCH][for-4.19 v2 1/1] xen: introduce a deviation for Rule 11.9

[Jan Beulich]

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

Re: [XEN PATCH][for-4.19 v2 1/1] xen: introduce a deviation for Rule 11.9

[Nicola Vetrini]

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.

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

Re: [XEN PATCH][for-4.19 v2 1/1] xen: introduce a deviation for Rule 11.9

[Jan Beulich]

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

Re: [XEN PATCH][for-4.19 v2 1/1] xen: introduce a deviation for Rule 11.9

[Nicola Vetrini]

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.

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