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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 05/10/2023 09:45, 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 documented in rules.rst as an exception.
Documenting ACCESS_ONCE() in rules.rst seems a bit odd. I am guessing 
that other analysis tool may point out the same error and therefore it 
would seem more appropriate to use a deviation.
This would also avoid having a specific rule in the Eclair configuration 
for __ACCESS_ONCE().
> 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.
>
> Cleanup of spurious MISRA deviations.
Please don't do that. This is making the review of the patches a lot 
more complicated because there are unrelated changes (see [1]).
We often allow simple clean-up if they are in the context. But this is 
not the case here.
> No functional change intended.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> ---
> If NULL is not suitable for usage inside access_field, then 0 could
> be put there and the macro deviated.
> ---
>   .../eclair_analysis/ECLAIR/deviations.ecl     | 23 +++++++------------
>   docs/misra/rules.rst                          |  3 ++-
>   xen/include/xen/compiler.h                    |  5 +++-
>   xen/include/xen/kernel.h                      |  2 +-
>   4 files changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index d8170106b449..acd42386e0a9 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -110,10 +110,6 @@ neither functions nor pointers to functions."
>   
> -config=MC3R1.R5.5,reports={safe,"all_area(decl(node(enum_decl||record_decl||field_decl||param_decl||var_decl)&&!type(canonical(address((node(function||function_no_proto))))))||macro(function_like()))"}
>   -doc_end
>
> --doc_begin="The use of these identifiers for both macro names and other 
> entities
> -is deliberate and does not generate developer confusion."
> --config=MC3R1.R5.5,reports+={safe, 
> "any_area(text(^\\s*/\\*\\s+SAF-[0-9]+-safe\\s+MC3R1\\.R5\\.5.*$, begin-1))"}
> --doc_end
>
>   -doc_begin="The definition of macros and functions ending in '_bit' that use 
> the
>   same identifier in 'bitops.h' is deliberate and safe."
> @@ -156,11 +152,6 @@ particular use of it done in xen_mk_ulong."
>   
> -config=MC3R1.R7.2,reports+={deliberate,"any_area(any_loc(macro(name(BUILD_BUG_ON))))"}
>   -doc_end
>
> --doc_begin="The following string literals are assigned to pointers to non
> -const-qualified char."
> --config=MC3R1.R7.4,reports+={safe, 
> "any_area(text(^\\s*/\\*\\s+SAF-[0-9]+-safe\\s+MC3R1\\.R7\\.4.*$, begin-1))"}
> --doc_end
> - >   -doc_begin="Allow pointers of non-character type as long as the 
pointee is
>   const-qualified."
>   -config=MC3R1.R7.4,same_pointee=false
> @@ -222,12 +213,6 @@ definition is compiled-out or optimized-out by the 
> compiler)"
>   # Series 9.
>   #
>
> --doc_begin="The following variables are written before being set, therefore no
> -access to uninitialized memory locations happens, as explained in the deviation
> -comment."
> --config=MC3R1.R9.1,reports+={safe, 
> "any_area(text(^\\s*/\\*\\s+SAF-[0-9]+-safe\\s+MC3R1\\.R9\\.1.*$, begin-1))"}
> --doc_end
> -
>   -doc_begin="Violations in files that maintainers have asked to not modify in 
> the
>   context of R9.1."
>   -file_tag+={adopted_r9_1,"^xen/arch/arm/arm64/lib/find_next_bit\\.c$"}
> @@ -274,6 +259,14 @@ still non-negative."
>   -config=MC3R1.R10.1,etypes+={safe, 
> "stmt(operator(logical)||node(conditional_operator||binary_conditional_operator))", 
> "dst_type(ebool||boolean)"}
>   -doc_end
>
> +#
> +# Series 11
> +#
> +
> +-doc_begin="This macro 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
> +
>   ### Set 3 ###
>
>   #
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index 3139ca7ae6dd..d5569696b3a8 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -393,7 +393,8 @@ maintainers if you want to suggest a change.
>      * - `Rule 11.9 
> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_11_09.c>`_
>        - Required
>        - The macro NULL shall be the only permitted form of null pointer 
> constant
> -     -
> +     - Using 0 as a null pointer constant to check if a type is scalar is
> +       allowed and always happens through the macro __ACCESS_ONCE.
>
>      * - `Rule 12.5 
> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_12_05.c>`_
>        - Mandatory
> 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
Cheers,

[1] 
https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#What_is_in_a_patch_series.3F
--
Julien Grall