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

[XEN PATCH][for-4.19 0/2] 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 rules.rst, is in
__ACCESS_ONCE to do a scalar type check.

The series only touches common headers, therefore it should be safe to include
in the for-4.19 branch.

Nicola Vetrini (2):
  xen: introduce a deviation for Rule 11.9
  xen/spinlock: fix use of 0 as a null pointer constant

 .../eclair_analysis/ECLAIR/deviations.ecl     | 23 +++++++------------
 docs/misra/rules.rst                          |  3 ++-
 xen/include/xen/compiler.h                    |  5 +++-
 xen/include/xen/kernel.h                      |  2 +-
 xen/include/xen/spinlock.h                    |  2 +-
 5 files changed, 16 insertions(+), 19 deletions(-)

--
2.34.1

[XEN PATCH][for-4.19 1/2] 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 documented in rules.rst as an exception.

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.

No functional change intended.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
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

[XEN PATCH][for-4.19 2/2] xen/spinlock: fix use of 0 as a null pointer constant

[Nicola Vetrini]

The constant 0 is used as a null pointer constant, in
violation of MISRA C:2012 Rule 11.9, in builds with
CONFIG_DEBUG_LOCK_PROFILE defined.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Release builds should not be impacted by this
---
 xen/include/xen/spinlock.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index e7a1c1aa8988..16d933ae7ebe 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -94,7 +94,7 @@ struct lock_profile_qhead {
     int32_t                   idx;     /* index for printout */
 };
 
-#define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 }
+#define _LOCK_PROFILE(name) { NULL, #name, &name, 0, 0, 0, 0, 0 }
 #define _LOCK_PROFILE_PTR(name)                                               \
     static struct lock_profile * const __lock_profile_##name                  \
     __used_section(".lockprofile.data") =                                     \
-- 
2.34.1

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

[Stefano Stabellini]

On Thu, 5 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 documented in rules.rst as an exception.
> 
> 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.
> 
> No functional change intended.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> If NULL is not suitable for usage inside access_field, then 0 could
> be put there and the macro deviated.

I think that's OK but let's wait to see if anyone else has a different
feedback


> ---
>  .../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
> 

Re: [XEN PATCH][for-4.19 2/2] xen/spinlock: fix use of 0 as a null pointer constant

[Stefano Stabellini]

On Thu, 5 Oct 2023, Nicola Vetrini wrote:
> The constant 0 is used as a null pointer constant, in
> violation of MISRA C:2012 Rule 11.9, in builds with
> CONFIG_DEBUG_LOCK_PROFILE defined.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Release builds should not be impacted by this
> ---
>  xen/include/xen/spinlock.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
> index e7a1c1aa8988..16d933ae7ebe 100644
> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -94,7 +94,7 @@ struct lock_profile_qhead {
>      int32_t                   idx;     /* index for printout */
>  };
>  
> -#define _LOCK_PROFILE(name) { 0, #name, &name, 0, 0, 0, 0, 0 }
> +#define _LOCK_PROFILE(name) { NULL, #name, &name, 0, 0, 0, 0, 0 }
>  #define _LOCK_PROFILE_PTR(name)                                               \
>      static struct lock_profile * const __lock_profile_##name                  \
>      __used_section(".lockprofile.data") =                                     \
> -- 
> 2.34.1
> 

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

[Julien Grall]

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@bugseng.com>
> ---
> 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

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

[Nicola Vetrini]

On 06/10/2023 11:27, Julien Grall wrote:
> 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().
> 

I figured a single accepted use would benefit from an explicit 
exclusion.
I can rework it to use an in-code comment to deviate, in whatever form 
that comment may be
(still with some bits of ECLAIR-specific configuration anyway, as 
discussed for R2.1).

>> 
>> 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.
> 

Understood. There will be a separate MISRA deviations cleanup/update 
patch anyway,
so these can be included there.


> [1] 
> https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#What_is_in_a_patch_series.3F

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

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

[Julien Grall]

On 06/10/2023 10:58, Nicola Vetrini wrote:
> On 06/10/2023 11:27, Julien Grall wrote:
>> 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().
>>
> 
> I figured a single accepted use would benefit from an explicit exclusion.
> I can rework it to use an in-code comment to deviate, in whatever form 
> that comment may be
> (still with some bits of ECLAIR-specific configuration anyway, as 
> discussed for R2.1).

I think it would be preferrable to have a deviation in the code. This 
would be helpful for other analysis tools.

> 
>>>
>>> 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.
>>
> 
> Understood. There will be a separate MISRA deviations cleanup/update 
> patch anyway,
> so these can be included there.

Sounds good to me. Thanks!

Cheers,

-- 
Julien Grall

Re: [XEN PATCH][for-4.19 2/2] xen/spinlock: fix use of 0 as a null pointer constant

[andrew.cooper3]

On 06/10/2023 2:07 am, Stefano Stabellini wrote:
> On Thu, 5 Oct 2023, Nicola Vetrini wrote:
>> The constant 0 is used as a null pointer constant, in
>> violation of MISRA C:2012 Rule 11.9, in builds with
>> CONFIG_DEBUG_LOCK_PROFILE defined.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

I've picked this up for 4.19.  It's trivial and not liable to conflict
with other work.

~Andrew

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

[Stefano Stabellini]

On Fri, 6 Oct 2023, Julien Grall wrote:
> On 06/10/2023 10:58, Nicola Vetrini wrote:
> > On 06/10/2023 11:27, Julien Grall wrote:
> > > 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().
> > > 
> > 
> > I figured a single accepted use would benefit from an explicit exclusion.
> > I can rework it to use an in-code comment to deviate, in whatever form that
> > comment may be
> > (still with some bits of ECLAIR-specific configuration anyway, as discussed
> > for R2.1).
> 
> I think it would be preferrable to have a deviation in the code. This would be
> helpful for other analysis tools.

Yes exactly, see my reply:
https://marc.info/?l=xen-devel&m=169663696228889

I know I acked the patch but I agree with Julien. A deviation as an
in-code comment (SAF-x-safe) is always the best option. If that doesn't
work, we cannot keep adding stuff in the notes section of rules.rst. It
doesn't scale. We should create a new document, like deviations.rst, or
add a new section at the bottom of documenting-violations.rst or
possibly safe.json.

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

[Nicola Vetrini]

On 07/10/2023 02:33, Stefano Stabellini wrote:
> On Fri, 6 Oct 2023, Julien Grall wrote:
>> On 06/10/2023 10:58, Nicola Vetrini wrote:
>> > On 06/10/2023 11:27, Julien Grall wrote:
>> > > 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().
>> > >
>> >
>> > I figured a single accepted use would benefit from an explicit exclusion.
>> > I can rework it to use an in-code comment to deviate, in whatever form that
>> > comment may be
>> > (still with some bits of ECLAIR-specific configuration anyway, as discussed
>> > for R2.1).
>> 
>> I think it would be preferrable to have a deviation in the code. This 
>> would be
>> helpful for other analysis tools.
> 
> Yes exactly, see my reply:
> https://marc.info/?l=xen-devel&m=169663696228889
> 
> I know I acked the patch but I agree with Julien. A deviation as an
> in-code comment (SAF-x-safe) is always the best option. If that doesn't
> work, we cannot keep adding stuff in the notes section of rules.rst. It
> doesn't scale. We should create a new document, like deviations.rst, or
> add a new section at the bottom of documenting-violations.rst or
> possibly safe.json.

I'll rebase this patch with an entry in deviations.rst, so that this 
applies to staging
cleanly.

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