[PATCH 0/7] Fix MISRA C 2012 Rule 20.7 violations

[PATCH 0/7] Fix MISRA C 2012 Rule 20.7 violations

[Xenia Ragiadakou]

Xenia Ragiadakou (7):
  xen/arm: gic_v3_its: Fix MISRA C 2012 Rule 20.7 violations
  xsm/flask: sidtab: Fix MISRA C 2012 Rule 20.7 violations
  xen/elf: Fix MISRA C 2012 Rule 20.7 violations
  xen/vgic: Fix MISRA C 2012 Rule 20.7 violation
  xen/rbtree: Fix MISRA C 2012 Rule 20.7 violation
  xen/arm: processor: Fix MISRA C 2012 Rule 20.7 violations
  xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations

 xen/arch/arm/include/asm/gic_v3_its.h | 10 +++++-----
 xen/arch/arm/include/asm/new_vgic.h   |  2 +-
 xen/arch/arm/include/asm/processor.h  |  4 ++--
 xen/include/xen/device_tree.h         |  6 +++---
 xen/include/xen/elfstructs.h          |  4 ++--
 xen/lib/rbtree.c                      |  2 +-
 xen/xsm/flask/ss/sidtab.c             |  8 ++++----
 7 files changed, 18 insertions(+), 18 deletions(-)

-- 
2.34.1

[PATCH 1/7] xen/arm: gic_v3_its: Fix MISRA C 2012 Rule 20.7 violations

[Xenia Ragiadakou]

In macros GITS_TYPER_DEVICE_ID_BITS(), GITS_TYPER_EVENT_ID_BITS() and
GITS_BASER_ENTRY_SIZE(), add parentheses around the macro parameter to
prevent against unintended expansions.
Realign subsequent lines, if any.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/arch/arm/include/asm/gic_v3_its.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/include/asm/gic_v3_its.h b/xen/arch/arm/include/asm/gic_v3_its.h
index 94e5cb99c5..168617097f 100644
--- a/xen/arch/arm/include/asm/gic_v3_its.h
+++ b/xen/arch/arm/include/asm/gic_v3_its.h
@@ -46,13 +46,13 @@
 #define GITS_TYPER_PTA                  BIT(19, UL)
 #define GITS_TYPER_DEVIDS_SHIFT         13
 #define GITS_TYPER_DEVIDS_MASK          (0x1fUL << GITS_TYPER_DEVIDS_SHIFT)
-#define GITS_TYPER_DEVICE_ID_BITS(r)    (((r & GITS_TYPER_DEVIDS_MASK) >> \
-                                               GITS_TYPER_DEVIDS_SHIFT) + 1)
+#define GITS_TYPER_DEVICE_ID_BITS(r)    ((((r) & GITS_TYPER_DEVIDS_MASK) >> \
+                                                 GITS_TYPER_DEVIDS_SHIFT) + 1)
 
 #define GITS_TYPER_IDBITS_SHIFT         8
 #define GITS_TYPER_IDBITS_MASK          (0x1fUL << GITS_TYPER_IDBITS_SHIFT)
-#define GITS_TYPER_EVENT_ID_BITS(r)     (((r & GITS_TYPER_IDBITS_MASK) >> \
-                                               GITS_TYPER_IDBITS_SHIFT) + 1)
+#define GITS_TYPER_EVENT_ID_BITS(r)     ((((r) & GITS_TYPER_IDBITS_MASK) >> \
+                                                 GITS_TYPER_IDBITS_SHIFT) + 1)
 
 #define GITS_TYPER_ITT_SIZE_SHIFT       4
 #define GITS_TYPER_ITT_SIZE_MASK        (0xfUL << GITS_TYPER_ITT_SIZE_SHIFT)
@@ -75,7 +75,7 @@
 #define GITS_BASER_TYPE_RESERVED7       7UL
 #define GITS_BASER_ENTRY_SIZE_SHIFT     48
 #define GITS_BASER_ENTRY_SIZE(reg)                                       \
-                        (((reg >> GITS_BASER_ENTRY_SIZE_SHIFT) & 0x1f) + 1)
+                        ((((reg) >> GITS_BASER_ENTRY_SIZE_SHIFT) & 0x1f) + 1)
 #define GITS_BASER_SHAREABILITY_SHIFT   10
 #define GITS_BASER_PAGE_SIZE_SHIFT      8
 #define GITS_BASER_SIZE_MASK            0xff
-- 
2.34.1

[PATCH 2/7] xsm/flask: sidtab: Fix MISRA C 2012 Rule 20.7 violations

[Xenia Ragiadakou]

In macros SIDTAB_HASH(), INIT_SIDTAB_LOCK(), SIDTAB_LOCK() and SIDTAB_UNLOCK(),
add parentheses around the macro parameter to prevent against unintended
expansions.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/xsm/flask/ss/sidtab.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/xsm/flask/ss/sidtab.c b/xen/xsm/flask/ss/sidtab.c
index 74babfac9c..69fc3389b3 100644
--- a/xen/xsm/flask/ss/sidtab.c
+++ b/xen/xsm/flask/ss/sidtab.c
@@ -14,11 +14,11 @@
 #include "security.h"
 #include "sidtab.h"
 
-#define SIDTAB_HASH(sid) (sid & SIDTAB_HASH_MASK)
+#define SIDTAB_HASH(sid) ((sid) & SIDTAB_HASH_MASK)
 
-#define INIT_SIDTAB_LOCK(s) spin_lock_init(&s->lock)
-#define SIDTAB_LOCK(s) spin_lock(&s->lock)
-#define SIDTAB_UNLOCK(s) spin_unlock(&s->lock)
+#define INIT_SIDTAB_LOCK(s) spin_lock_init(&(s)->lock)
+#define SIDTAB_LOCK(s) spin_lock(&(s)->lock)
+#define SIDTAB_UNLOCK(s) spin_unlock(&(s)->lock)
 
 int sidtab_init(struct sidtab *s)
 {
-- 
2.34.1

[PATCH 3/7] xen/elf: Fix MISRA C 2012 Rule 20.7 violations

[Xenia Ragiadakou]

In macros ELF32_ST_TYPE() and ELF64_ST_TYPE(), add parentheses around the
macro parameter to prevent against unintended expansions.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/include/xen/elfstructs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/xen/elfstructs.h b/xen/include/xen/elfstructs.h
index 616ebf9269..0a7c558a80 100644
--- a/xen/include/xen/elfstructs.h
+++ b/xen/include/xen/elfstructs.h
@@ -305,11 +305,11 @@ typedef struct {
 
 /* Extract symbol info - st_info */
 #define ELF32_ST_BIND(x)	((x) >> 4)
-#define ELF32_ST_TYPE(x)	(((unsigned int) x) & 0xf)
+#define ELF32_ST_TYPE(x)	(((unsigned int) (x)) & 0xf)
 #define ELF32_ST_INFO(b,t)	(((b) << 4) + ((t) & 0xf))
 
 #define ELF64_ST_BIND(x)	((x) >> 4)
-#define ELF64_ST_TYPE(x)	(((unsigned int) x) & 0xf)
+#define ELF64_ST_TYPE(x)	(((unsigned int) (x)) & 0xf)
 #define ELF64_ST_INFO(b,t)	(((b) << 4) + ((t) & 0xf))
 
 /* Symbol Binding - ELF32_ST_BIND - st_info */
-- 
2.34.1

[PATCH 4/7] xen/vgic: Fix MISRA C 2012 Rule 20.7 violation

[Xenia Ragiadakou]

In macro VGIC_V3_LR_INDEX(), add parentheses around the macro parameter
to prevent against unintended expansions.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/arch/arm/include/asm/new_vgic.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/include/asm/new_vgic.h b/xen/arch/arm/include/asm/new_vgic.h
index ab57fcd91d..b7fa9ab11a 100644
--- a/xen/arch/arm/include/asm/new_vgic.h
+++ b/xen/arch/arm/include/asm/new_vgic.h
@@ -43,7 +43,7 @@ enum vgic_type {
 
 #define VGIC_V2_MAX_LRS         (1 << 6)
 #define VGIC_V3_MAX_LRS         16
-#define VGIC_V3_LR_INDEX(lr)    (VGIC_V3_MAX_LRS - 1 - lr)
+#define VGIC_V3_LR_INDEX(lr)    (VGIC_V3_MAX_LRS - 1 - (lr))
 
 #define VGIC_CONFIG_EDGE        false
 #define VGIC_CONFIG_LEVEL       true
-- 
2.34.1

[PATCH 5/7] xen/rbtree: Fix MISRA C 2012 Rule 20.7 violation

[Xenia Ragiadakou]

In macro __rb_parent(), add parentheses around the macro parameter to prevent
against unintended expansions.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/lib/rbtree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/lib/rbtree.c b/xen/lib/rbtree.c
index 85a4f20313..eb418baabb 100644
--- a/xen/lib/rbtree.c
+++ b/xen/lib/rbtree.c
@@ -46,7 +46,7 @@
 #define		RB_RED		0
 #define		RB_BLACK	1
 
-#define __rb_parent(pc)    ((struct rb_node *)(pc & ~3))
+#define __rb_parent(pc)    ((struct rb_node *)((pc) & ~3))
 
 #define __rb_color(pc)     ((pc) & 1)
 #define __rb_is_black(pc)  __rb_color(pc)
-- 
2.34.1

[PATCH 6/7] xen/arm: processor: Fix MISRA C 2012 Rule 20.7 violations

[Xenia Ragiadakou]

In macros MPIDR_LEVEL_SHIFT() and MPIDR_AFFINITY_LEVEL(), add parentheses
around the macro parameters 'level' and 'mpidr', respectively, to prevent
against unintended expansions.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/arch/arm/include/asm/processor.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h
index 55f56b33bc..1dd81d7d52 100644
--- a/xen/arch/arm/include/asm/processor.h
+++ b/xen/arch/arm/include/asm/processor.h
@@ -127,10 +127,10 @@
 #define MPIDR_LEVEL_MASK        ((1 << MPIDR_LEVEL_BITS) - 1)
 
 #define MPIDR_LEVEL_SHIFT(level) \
-         (((1 << level) >> 1) << MPIDR_LEVEL_BITS_SHIFT)
+         (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT)
 
 #define MPIDR_AFFINITY_LEVEL(mpidr, level) \
-         ((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK)
+         (((mpidr) >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK)
 
 #define AFFINITY_MASK(level)    ~((_AC(0x1,UL) << MPIDR_LEVEL_SHIFT(level)) - 1)
 
-- 
2.34.1

[PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations

[Xenia Ragiadakou]

In macros dt_for_each_property_node(), dt_for_each_device_node() and
dt_for_each_child_node(), add parentheses around the macro parameters that
have the arrow operator applied, to prevent against unintended expansions.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 xen/include/xen/device_tree.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 430a1ef445..6e4391c126 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -222,13 +222,13 @@ dt_find_interrupt_controller(const struct dt_device_match *matches);
 #define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1
 
 #define dt_for_each_property_node(dn, pp)                   \
-    for ( pp = dn->properties; pp != NULL; pp = pp->next )
+    for ( pp = (dn)->properties; pp != NULL; pp = (pp)->next )
 
 #define dt_for_each_device_node(dt, dn)                     \
-    for ( dn = dt; dn != NULL; dn = dn->allnext )
+    for ( dn = dt; dn != NULL; dn = (dn)->allnext )
 
 #define dt_for_each_child_node(dt, dn)                      \
-    for ( dn = dt->child; dn != NULL; dn = dn->sibling )
+    for ( dn = (dt)->child; dn != NULL; dn = (dn)->sibling )
 
 /* Helper to read a big number; size is in cells (not bytes) */
 static inline u64 dt_read_number(const __be32 *cell, int size)
-- 
2.34.1

Re: [PATCH 1/7] xen/arm: gic_v3_its: Fix MISRA C 2012 Rule 20.7 violations

[Stefano Stabellini]

On Fri, 19 Aug 2022, Xenia Ragiadakou wrote:
> In macros GITS_TYPER_DEVICE_ID_BITS(), GITS_TYPER_EVENT_ID_BITS() and
> GITS_BASER_ENTRY_SIZE(), add parentheses around the macro parameter to
> prevent against unintended expansions.
> Realign subsequent lines, if any.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

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


> ---
>  xen/arch/arm/include/asm/gic_v3_its.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/gic_v3_its.h b/xen/arch/arm/include/asm/gic_v3_its.h
> index 94e5cb99c5..168617097f 100644
> --- a/xen/arch/arm/include/asm/gic_v3_its.h
> +++ b/xen/arch/arm/include/asm/gic_v3_its.h
> @@ -46,13 +46,13 @@
>  #define GITS_TYPER_PTA                  BIT(19, UL)
>  #define GITS_TYPER_DEVIDS_SHIFT         13
>  #define GITS_TYPER_DEVIDS_MASK          (0x1fUL << GITS_TYPER_DEVIDS_SHIFT)
> -#define GITS_TYPER_DEVICE_ID_BITS(r)    (((r & GITS_TYPER_DEVIDS_MASK) >> \
> -                                               GITS_TYPER_DEVIDS_SHIFT) + 1)
> +#define GITS_TYPER_DEVICE_ID_BITS(r)    ((((r) & GITS_TYPER_DEVIDS_MASK) >> \
> +                                                 GITS_TYPER_DEVIDS_SHIFT) + 1)
>  
>  #define GITS_TYPER_IDBITS_SHIFT         8
>  #define GITS_TYPER_IDBITS_MASK          (0x1fUL << GITS_TYPER_IDBITS_SHIFT)
> -#define GITS_TYPER_EVENT_ID_BITS(r)     (((r & GITS_TYPER_IDBITS_MASK) >> \
> -                                               GITS_TYPER_IDBITS_SHIFT) + 1)
> +#define GITS_TYPER_EVENT_ID_BITS(r)     ((((r) & GITS_TYPER_IDBITS_MASK) >> \
> +                                                 GITS_TYPER_IDBITS_SHIFT) + 1)
>  
>  #define GITS_TYPER_ITT_SIZE_SHIFT       4
>  #define GITS_TYPER_ITT_SIZE_MASK        (0xfUL << GITS_TYPER_ITT_SIZE_SHIFT)
> @@ -75,7 +75,7 @@
>  #define GITS_BASER_TYPE_RESERVED7       7UL
>  #define GITS_BASER_ENTRY_SIZE_SHIFT     48
>  #define GITS_BASER_ENTRY_SIZE(reg)                                       \
> -                        (((reg >> GITS_BASER_ENTRY_SIZE_SHIFT) & 0x1f) + 1)
> +                        ((((reg) >> GITS_BASER_ENTRY_SIZE_SHIFT) & 0x1f) + 1)
>  #define GITS_BASER_SHAREABILITY_SHIFT   10
>  #define GITS_BASER_PAGE_SIZE_SHIFT      8
>  #define GITS_BASER_SIZE_MASK            0xff
> -- 
> 2.34.1
> 

Re: [PATCH 2/7] xsm/flask: sidtab: Fix MISRA C 2012 Rule 20.7 violations

[Stefano Stabellini]

On Fri, 19 Aug 2022, Xenia Ragiadakou wrote:
> In macros SIDTAB_HASH(), INIT_SIDTAB_LOCK(), SIDTAB_LOCK() and SIDTAB_UNLOCK(),
> add parentheses around the macro parameter to prevent against unintended
> expansions.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

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


> ---
>  xen/xsm/flask/ss/sidtab.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/xsm/flask/ss/sidtab.c b/xen/xsm/flask/ss/sidtab.c
> index 74babfac9c..69fc3389b3 100644
> --- a/xen/xsm/flask/ss/sidtab.c
> +++ b/xen/xsm/flask/ss/sidtab.c
> @@ -14,11 +14,11 @@
>  #include "security.h"
>  #include "sidtab.h"
>  
> -#define SIDTAB_HASH(sid) (sid & SIDTAB_HASH_MASK)
> +#define SIDTAB_HASH(sid) ((sid) & SIDTAB_HASH_MASK)
>  
> -#define INIT_SIDTAB_LOCK(s) spin_lock_init(&s->lock)
> -#define SIDTAB_LOCK(s) spin_lock(&s->lock)
> -#define SIDTAB_UNLOCK(s) spin_unlock(&s->lock)
> +#define INIT_SIDTAB_LOCK(s) spin_lock_init(&(s)->lock)
> +#define SIDTAB_LOCK(s) spin_lock(&(s)->lock)
> +#define SIDTAB_UNLOCK(s) spin_unlock(&(s)->lock)
>  
>  int sidtab_init(struct sidtab *s)
>  {
> -- 
> 2.34.1
> 
> 

Re: [PATCH 3/7] xen/elf: Fix MISRA C 2012 Rule 20.7 violations

[Stefano Stabellini]

On Fri, 19 Aug 2022, Xenia Ragiadakou wrote:
> In macros ELF32_ST_TYPE() and ELF64_ST_TYPE(), add parentheses around the
> macro parameter to prevent against unintended expansions.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

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


> ---
>  xen/include/xen/elfstructs.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/xen/elfstructs.h b/xen/include/xen/elfstructs.h
> index 616ebf9269..0a7c558a80 100644
> --- a/xen/include/xen/elfstructs.h
> +++ b/xen/include/xen/elfstructs.h
> @@ -305,11 +305,11 @@ typedef struct {
>  
>  /* Extract symbol info - st_info */
>  #define ELF32_ST_BIND(x)	((x) >> 4)
> -#define ELF32_ST_TYPE(x)	(((unsigned int) x) & 0xf)
> +#define ELF32_ST_TYPE(x)	(((unsigned int) (x)) & 0xf)
>  #define ELF32_ST_INFO(b,t)	(((b) << 4) + ((t) & 0xf))
>  
>  #define ELF64_ST_BIND(x)	((x) >> 4)
> -#define ELF64_ST_TYPE(x)	(((unsigned int) x) & 0xf)
> +#define ELF64_ST_TYPE(x)	(((unsigned int) (x)) & 0xf)
>  #define ELF64_ST_INFO(b,t)	(((b) << 4) + ((t) & 0xf))
>  
>  /* Symbol Binding - ELF32_ST_BIND - st_info */
> -- 
> 2.34.1
> 

Re: [PATCH 4/7] xen/vgic: Fix MISRA C 2012 Rule 20.7 violation

[Stefano Stabellini]

On Fri, 19 Aug 2022, Xenia Ragiadakou wrote:
> In macro VGIC_V3_LR_INDEX(), add parentheses around the macro parameter
> to prevent against unintended expansions.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

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


> ---
>  xen/arch/arm/include/asm/new_vgic.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/include/asm/new_vgic.h b/xen/arch/arm/include/asm/new_vgic.h
> index ab57fcd91d..b7fa9ab11a 100644
> --- a/xen/arch/arm/include/asm/new_vgic.h
> +++ b/xen/arch/arm/include/asm/new_vgic.h
> @@ -43,7 +43,7 @@ enum vgic_type {
>  
>  #define VGIC_V2_MAX_LRS         (1 << 6)
>  #define VGIC_V3_MAX_LRS         16
> -#define VGIC_V3_LR_INDEX(lr)    (VGIC_V3_MAX_LRS - 1 - lr)
> +#define VGIC_V3_LR_INDEX(lr)    (VGIC_V3_MAX_LRS - 1 - (lr))
>  
>  #define VGIC_CONFIG_EDGE        false
>  #define VGIC_CONFIG_LEVEL       true
> -- 
> 2.34.1
> 

Re: [PATCH 5/7] xen/rbtree: Fix MISRA C 2012 Rule 20.7 violation

[Stefano Stabellini]

On Fri, 19 Aug 2022, Xenia Ragiadakou wrote:
> In macro __rb_parent(), add parentheses around the macro parameter to prevent
> against unintended expansions.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

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


> ---
>  xen/lib/rbtree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/lib/rbtree.c b/xen/lib/rbtree.c
> index 85a4f20313..eb418baabb 100644
> --- a/xen/lib/rbtree.c
> +++ b/xen/lib/rbtree.c
> @@ -46,7 +46,7 @@
>  #define		RB_RED		0
>  #define		RB_BLACK	1
>  
> -#define __rb_parent(pc)    ((struct rb_node *)(pc & ~3))
> +#define __rb_parent(pc)    ((struct rb_node *)((pc) & ~3))
>  
>  #define __rb_color(pc)     ((pc) & 1)
>  #define __rb_is_black(pc)  __rb_color(pc)
> -- 
> 2.34.1
> 

Re: [PATCH 6/7] xen/arm: processor: Fix MISRA C 2012 Rule 20.7 violations

[Stefano Stabellini]

On Fri, 19 Aug 2022, Xenia Ragiadakou wrote:
> In macros MPIDR_LEVEL_SHIFT() and MPIDR_AFFINITY_LEVEL(), add parentheses
> around the macro parameters 'level' and 'mpidr', respectively, to prevent
> against unintended expansions.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

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


> ---
>  xen/arch/arm/include/asm/processor.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h
> index 55f56b33bc..1dd81d7d52 100644
> --- a/xen/arch/arm/include/asm/processor.h
> +++ b/xen/arch/arm/include/asm/processor.h
> @@ -127,10 +127,10 @@
>  #define MPIDR_LEVEL_MASK        ((1 << MPIDR_LEVEL_BITS) - 1)
>  
>  #define MPIDR_LEVEL_SHIFT(level) \
> -         (((1 << level) >> 1) << MPIDR_LEVEL_BITS_SHIFT)
> +         (((1 << (level)) >> 1) << MPIDR_LEVEL_BITS_SHIFT)
>  
>  #define MPIDR_AFFINITY_LEVEL(mpidr, level) \
> -         ((mpidr >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK)
> +         (((mpidr) >> MPIDR_LEVEL_SHIFT(level)) & MPIDR_LEVEL_MASK)
>  
>  #define AFFINITY_MASK(level)    ~((_AC(0x1,UL) << MPIDR_LEVEL_SHIFT(level)) - 1)
>  
> -- 
> 2.34.1
> 

Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations

[Stefano Stabellini]

On Fri, 19 Aug 2022, Xenia Ragiadakou wrote:
> In macros dt_for_each_property_node(), dt_for_each_device_node() and
> dt_for_each_child_node(), add parentheses around the macro parameters that
> have the arrow operator applied, to prevent against unintended expansions.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

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


> ---
>  xen/include/xen/device_tree.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 430a1ef445..6e4391c126 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -222,13 +222,13 @@ dt_find_interrupt_controller(const struct dt_device_match *matches);
>  #define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1
>  
>  #define dt_for_each_property_node(dn, pp)                   \
> -    for ( pp = dn->properties; pp != NULL; pp = pp->next )
> +    for ( pp = (dn)->properties; pp != NULL; pp = (pp)->next )
>  
>  #define dt_for_each_device_node(dt, dn)                     \
> -    for ( dn = dt; dn != NULL; dn = dn->allnext )
> +    for ( dn = dt; dn != NULL; dn = (dn)->allnext )
>  
>  #define dt_for_each_child_node(dt, dn)                      \
> -    for ( dn = dt->child; dn != NULL; dn = dn->sibling )
> +    for ( dn = (dt)->child; dn != NULL; dn = (dn)->sibling )
>  
>  /* Helper to read a big number; size is in cells (not bytes) */
>  static inline u64 dt_read_number(const __be32 *cell, int size)
> -- 
> 2.34.1
> 

Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations

[Jan Beulich]

On 19.08.2022 21:43, Xenia Ragiadakou wrote:
> In macros dt_for_each_property_node(), dt_for_each_device_node() and
> dt_for_each_child_node(), add parentheses around the macro parameters that
> have the arrow operator applied, to prevent against unintended expansions.

Why is this relevant only when -> is used? For comparisons and the rhs of
assignments it's as relevant, ad even for the lhs of assignments I doubt
it can be generally omitted.

> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -222,13 +222,13 @@ dt_find_interrupt_controller(const struct dt_device_match *matches);
>  #define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1
>  
>  #define dt_for_each_property_node(dn, pp)                   \
> -    for ( pp = dn->properties; pp != NULL; pp = pp->next )
> +    for ( pp = (dn)->properties; pp != NULL; pp = (pp)->next )
>  
>  #define dt_for_each_device_node(dt, dn)                     \
> -    for ( dn = dt; dn != NULL; dn = dn->allnext )
> +    for ( dn = dt; dn != NULL; dn = (dn)->allnext )
>  
>  #define dt_for_each_child_node(dt, dn)                      \
> -    for ( dn = dt->child; dn != NULL; dn = dn->sibling )
> +    for ( dn = (dt)->child; dn != NULL; dn = (dn)->sibling )
>  
>  /* Helper to read a big number; size is in cells (not bytes) */
>  static inline u64 dt_read_number(const __be32 *cell, int size)

Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations

[Xenia Ragiadakou]

Hi Jan,

On 8/22/22 12:59, Jan Beulich wrote:
> On 19.08.2022 21:43, Xenia Ragiadakou wrote:
>> In macros dt_for_each_property_node(), dt_for_each_device_node() and
>> dt_for_each_child_node(), add parentheses around the macro parameters that
>> have the arrow operator applied, to prevent against unintended expansions.
> 
> Why is this relevant only when -> is used? For comparisons and the rhs of
> assignments it's as relevant, ad even for the lhs of assignments I doubt
> it can be generally omitted.

Yes, I agree with you but some older patches that I sent that were 
adding parentheses around the lhs of the assignments were not accepted 
and I thought that the rhs of the assignments as well these comparisons 
fall to the same category.

Personally, I would expect to see parentheses, also, around the macro 
parameters that are used as the lhs or the rhs of assignments, the 
operands of comparison or the arguments of a function.
Not only because they can prevent against unintentional bugs but because 
the parentheses help me to identify more easily the macro parameters 
when reading a macro definition. I totally understand that for other 
people parentheses may reduce readability.

> 
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -222,13 +222,13 @@ dt_find_interrupt_controller(const struct dt_device_match *matches);
>>   #define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1
>>   
>>   #define dt_for_each_property_node(dn, pp)                   \
>> -    for ( pp = dn->properties; pp != NULL; pp = pp->next )
>> +    for ( pp = (dn)->properties; pp != NULL; pp = (pp)->next )
>>   
>>   #define dt_for_each_device_node(dt, dn)                     \
>> -    for ( dn = dt; dn != NULL; dn = dn->allnext )
>> +    for ( dn = dt; dn != NULL; dn = (dn)->allnext )
>>   
>>   #define dt_for_each_child_node(dt, dn)                      \
>> -    for ( dn = dt->child; dn != NULL; dn = dn->sibling )
>> +    for ( dn = (dt)->child; dn != NULL; dn = (dn)->sibling )
>>   
>>   /* Helper to read a big number; size is in cells (not bytes) */
>>   static inline u64 dt_read_number(const __be32 *cell, int size)
> 

-- 
Xenia

Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations

[Jan Beulich]

On 22.08.2022 12:43, Xenia Ragiadakou wrote:
> On 8/22/22 12:59, Jan Beulich wrote:
>> On 19.08.2022 21:43, Xenia Ragiadakou wrote:
>>> In macros dt_for_each_property_node(), dt_for_each_device_node() and
>>> dt_for_each_child_node(), add parentheses around the macro parameters that
>>> have the arrow operator applied, to prevent against unintended expansions.
>>
>> Why is this relevant only when -> is used? For comparisons and the rhs of
>> assignments it's as relevant, ad even for the lhs of assignments I doubt
>> it can be generally omitted.
> 
> Yes, I agree with you but some older patches that I sent that were 
> adding parentheses around the lhs of the assignments were not accepted 
> and I thought that the rhs of the assignments as well these comparisons 
> fall to the same category.
> 
> Personally, I would expect to see parentheses, also, around the macro 
> parameters that are used as the lhs or the rhs of assignments, the 
> operands of comparison or the arguments of a function.
> Not only because they can prevent against unintentional bugs but because 
> the parentheses help me to identify more easily the macro parameters 
> when reading a macro definition. I totally understand that for other 
> people parentheses may reduce readability.

Afair Julien's comments were very specific to the lhs of assignments.
So at the very least everything else ought to be parenthesized imo.

Jan+

Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations

[Xenia Ragiadakou]

On 8/22/22 14:48, Jan Beulich wrote:
> On 22.08.2022 12:43, Xenia Ragiadakou wrote:
>> On 8/22/22 12:59, Jan Beulich wrote:
>>> On 19.08.2022 21:43, Xenia Ragiadakou wrote:
>>>> In macros dt_for_each_property_node(), dt_for_each_device_node() and
>>>> dt_for_each_child_node(), add parentheses around the macro parameters that
>>>> have the arrow operator applied, to prevent against unintended expansions.
>>>
>>> Why is this relevant only when -> is used? For comparisons and the rhs of
>>> assignments it's as relevant, ad even for the lhs of assignments I doubt
>>> it can be generally omitted.
>>
>> Yes, I agree with you but some older patches that I sent that were
>> adding parentheses around the lhs of the assignments were not accepted
>> and I thought that the rhs of the assignments as well these comparisons
>> fall to the same category.
>>
>> Personally, I would expect to see parentheses, also, around the macro
>> parameters that are used as the lhs or the rhs of assignments, the
>> operands of comparison or the arguments of a function.
>> Not only because they can prevent against unintentional bugs but because
>> the parentheses help me to identify more easily the macro parameters
>> when reading a macro definition. I totally understand that for other
>> people parentheses may reduce readability.
> 
> Afair Julien's comments were very specific to the lhs of assignments.
> So at the very least everything else ought to be parenthesized imo.
> 

So, IIUC, the only deviations from the MISRA C 2012 Rule 20.7 will be 
for macro parameters used as the lhs of assignments and function arguments?

This feels a bit of a hack to parenthesize the macro parameters that are 
used as the rhs of an assignment but not those used as the lhs.
 From previous discussions on the topic, I understood that the 
parentheses are considered needed only when they eliminate operator 
precedence problems, while for the wrong parameter format bugs we can 
rely on the reviewers.

I think we need to decide if the rule will be applied as is and if not 
which will be the deviations along with their justification and add it 
to the document.

-- 
Xenia

Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations

[Jan Beulich]

On 25.08.2022 10:02, Xenia Ragiadakou wrote:
> On 8/22/22 14:48, Jan Beulich wrote:
>> On 22.08.2022 12:43, Xenia Ragiadakou wrote:
>>> On 8/22/22 12:59, Jan Beulich wrote:
>>>> On 19.08.2022 21:43, Xenia Ragiadakou wrote:
>>>>> In macros dt_for_each_property_node(), dt_for_each_device_node() and
>>>>> dt_for_each_child_node(), add parentheses around the macro parameters that
>>>>> have the arrow operator applied, to prevent against unintended expansions.
>>>>
>>>> Why is this relevant only when -> is used? For comparisons and the rhs of
>>>> assignments it's as relevant, ad even for the lhs of assignments I doubt
>>>> it can be generally omitted.
>>>
>>> Yes, I agree with you but some older patches that I sent that were
>>> adding parentheses around the lhs of the assignments were not accepted
>>> and I thought that the rhs of the assignments as well these comparisons
>>> fall to the same category.
>>>
>>> Personally, I would expect to see parentheses, also, around the macro
>>> parameters that are used as the lhs or the rhs of assignments, the
>>> operands of comparison or the arguments of a function.
>>> Not only because they can prevent against unintentional bugs but because
>>> the parentheses help me to identify more easily the macro parameters
>>> when reading a macro definition. I totally understand that for other
>>> people parentheses may reduce readability.
>>
>> Afair Julien's comments were very specific to the lhs of assignments.
>> So at the very least everything else ought to be parenthesized imo.
>>
> 
> So, IIUC, the only deviations from the MISRA C 2012 Rule 20.7 will be 
> for macro parameters used as the lhs of assignments and function arguments?

Afaic I don't consider that discussion settled.

> This feels a bit of a hack to parenthesize the macro parameters that are 
> used as the rhs of an assignment but not those used as the lhs.

lhs and rhs of assignments are quite different, and hence making such a
distinction wouldn't look to be a "hack" to me. In fact I've always
considered this part of the language somewhat strange: To me
parenthesizing e.g. an identifier already makes it (visually) an
rvalue. Leaving aside odd (and easy to spot as odd) uses at the call
sizes, I don't think I can come up with a case where parentheses are
really needed. Anything needing parenthesizing actually yields an
rvalue afaics, thus causing a diagnostic anyway.

>  From previous discussions on the topic, I understood that the 
> parentheses are considered needed only when they eliminate operator 
> precedence problems, while for the wrong parameter format bugs we can 
> rely on the reviewers.
> 
> I think we need to decide if the rule will be applied as is and if not 
> which will be the deviations along with their justification and add it 
> to the document.

Yes. But this shouldn't hinder adjustments for all other, non-
controversial cases.

Jan

Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations

[Stefano Stabellini]

CC MISRA C working group

Short summary: we are discussing whether the following is sufficient to
address MISRA C Rule 20.7, and also in general for safety:


 #define dt_for_each_property_node(dn, pp)                   \
-    for ( pp = dn->properties; pp != NULL; pp = pp->next )
+    for ( pp = (dn)->properties; pp != NULL; pp = (pp)->next )


as you can see "dn" has been parenthesizing because is used as a rhs,
while "pp" has *not* because it is used as lhs.

More below.


On Thu, 25 Aug 2022, Jan Beulich wrote:
> On 25.08.2022 10:02, Xenia Ragiadakou wrote:
> > On 8/22/22 14:48, Jan Beulich wrote:
> >> On 22.08.2022 12:43, Xenia Ragiadakou wrote:
> >>> On 8/22/22 12:59, Jan Beulich wrote:
> >>>> On 19.08.2022 21:43, Xenia Ragiadakou wrote:
> >>>>> In macros dt_for_each_property_node(), dt_for_each_device_node() and
> >>>>> dt_for_each_child_node(), add parentheses around the macro parameters that
> >>>>> have the arrow operator applied, to prevent against unintended expansions.
> >>>>
> >>>> Why is this relevant only when -> is used? For comparisons and the rhs of
> >>>> assignments it's as relevant, ad even for the lhs of assignments I doubt
> >>>> it can be generally omitted.
> >>>
> >>> Yes, I agree with you but some older patches that I sent that were
> >>> adding parentheses around the lhs of the assignments were not accepted
> >>> and I thought that the rhs of the assignments as well these comparisons
> >>> fall to the same category.
> >>>
> >>> Personally, I would expect to see parentheses, also, around the macro
> >>> parameters that are used as the lhs or the rhs of assignments, the
> >>> operands of comparison or the arguments of a function.
> >>> Not only because they can prevent against unintentional bugs but because
> >>> the parentheses help me to identify more easily the macro parameters
> >>> when reading a macro definition. I totally understand that for other
> >>> people parentheses may reduce readability.
> >>
> >> Afair Julien's comments were very specific to the lhs of assignments.
> >> So at the very least everything else ought to be parenthesized imo.
> >>
> > 
> > So, IIUC, the only deviations from the MISRA C 2012 Rule 20.7 will be 
> > for macro parameters used as the lhs of assignments and function arguments?
> 
> Afaic I don't consider that discussion settled.
> 
> > This feels a bit of a hack to parenthesize the macro parameters that are 
> > used as the rhs of an assignment but not those used as the lhs.
> 
> lhs and rhs of assignments are quite different, and hence making such a
> distinction wouldn't look to be a "hack" to me. In fact I've always
> considered this part of the language somewhat strange: To me
> parenthesizing e.g. an identifier already makes it (visually) an
> rvalue. Leaving aside odd (and easy to spot as odd) uses at the call
> sizes, I don't think I can come up with a case where parentheses are
> really needed. Anything needing parenthesizing actually yields an
> rvalue afaics, thus causing a diagnostic anyway.

Although I can see where you are coming from, parenthesizing an
identifier doesn't actually make it an rvalue. Also it is a lot simpler
to understand, review, and apply a policy that says:

"all macro parameters are parenthesized"

compared to a policy that says:

"most macro paremeters are parenthesized, let's go into the details of
which ones are parenthesized and which ones are not, including examples
and corner cases"

For simplicity, I would go with the simplest version, the MISRA version.

I am assuming that the MISRA Rule 20.7 requires that "pp" is also
parenthesized. Roberto, is that correct?


> >  From previous discussions on the topic, I understood that the 
> > parentheses are considered needed only when they eliminate operator 
> > precedence problems, while for the wrong parameter format bugs we can 
> > rely on the reviewers.
> > 
> > I think we need to decide if the rule will be applied as is and if not 
> > which will be the deviations along with their justification and add it 
> > to the document.
> 
> Yes. But this shouldn't hinder adjustments for all other, non-
> controversial cases.

It looks like we need a discussion and see where the majority of the
team is on this issue. I prefer the original MISRA version for
simplicity, but I also think it is OK if we make a small customization
to it. In that case, we would add the extra explanation and details to
docs/misra/rules.rst.

However, as we make the decision we also need to take into account that
if we don't go with the vanilla MISRA rule, there is a price to pay: all
the MISRA scanners, including cppcheck, Eclair, Coverity and others would
probably still flag these issues as violations polluting the results
and making the scanners less useful. We might have to mark each
"deviation" with a one-line in-code comment on top, or we would have to
disable automatic scanning for Rule 20.7 altogether. Either option is
not great.

This is actually the main reason why I prefer the vanillay MISRA
version: even if the resulting style might not be as good the the custom
version, we don't need to worry about reviewing this rule because we can
easily get automatic scans for it.


But first, let's confirm whether this change:


 #define dt_for_each_property_node(dn, pp)                   \
-    for ( pp = dn->properties; pp != NULL; pp = pp->next )
+    for ( pp = (dn)->properties; pp != NULL; pp = (pp)->next )


is sufficient to make the violation go away in Eclair or cppcheck.  I am
assuming it is not sufficient, but let's confirm.

Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations

[Jan Beulich]

On 25.08.2022 20:09, Stefano Stabellini wrote:
> But first, let's confirm whether this change:
> 
> 
>  #define dt_for_each_property_node(dn, pp)                   \
> -    for ( pp = dn->properties; pp != NULL; pp = pp->next )
> +    for ( pp = (dn)->properties; pp != NULL; pp = (pp)->next )
> 
> 
> is sufficient to make the violation go away in Eclair or cppcheck.  I am
> assuming it is not sufficient, but let's confirm.

Well, even if for the lhs of assignments there was an exception, this
still wouldn't be sufficient. The minimum needed is

#define dt_for_each_property_node(dn, pp)                   \
    for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next )

Jan

Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations

[Xenia Ragiadakou]

On 8/26/22 09:21, Jan Beulich wrote:
> On 25.08.2022 20:09, Stefano Stabellini wrote:
>> But first, let's confirm whether this change:
>>
>>
>>   #define dt_for_each_property_node(dn, pp)                   \
>> -    for ( pp = dn->properties; pp != NULL; pp = pp->next )
>> +    for ( pp = (dn)->properties; pp != NULL; pp = (pp)->next )
>>
>>
>> is sufficient to make the violation go away in Eclair or cppcheck.  I am
>> assuming it is not sufficient, but let's confirm.
> 
> Well, even if for the lhs of assignments there was an exception, this
> still wouldn't be sufficient. The minimum needed is
> 
> #define dt_for_each_property_node(dn, pp)                   \
>      for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next )
> 

If pp is assumed to be a valid lvalue, then why it is needed to add 
parentheses here (pp) != NULL ?

For the violations to go away, parentheses should be placed around all 
macro parameters that represent expressions, that is
#define dt_for_each_property_node(dn, pp)                   \
       for ( (pp) = (dn)->properties; (pp) != NULL; (pp) = (pp)->next )

-- 
Xenia

Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations

[Jan Beulich]

On 26.08.2022 09:58, Xenia Ragiadakou wrote:
> On 8/26/22 09:21, Jan Beulich wrote:
>> On 25.08.2022 20:09, Stefano Stabellini wrote:
>>> But first, let's confirm whether this change:
>>>
>>>
>>>   #define dt_for_each_property_node(dn, pp)                   \
>>> -    for ( pp = dn->properties; pp != NULL; pp = pp->next )
>>> +    for ( pp = (dn)->properties; pp != NULL; pp = (pp)->next )
>>>
>>>
>>> is sufficient to make the violation go away in Eclair or cppcheck.  I am
>>> assuming it is not sufficient, but let's confirm.
>>
>> Well, even if for the lhs of assignments there was an exception, this
>> still wouldn't be sufficient. The minimum needed is
>>
>> #define dt_for_each_property_node(dn, pp)                   \
>>      for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next )
>>
> 
> If pp is assumed to be a valid lvalue, then why it is needed to add 
> parentheses here (pp) != NULL ?

Because in one expression is doesn't matter that in another expression
the same identifier is used as the lhs of an assignment. Whether
parentheses are needed should solely depend on the operators in use,
not any further context.

Jan

Re: [PATCH 2/7] xsm/flask: sidtab: Fix MISRA C 2012 Rule 20.7 violations

[Daniel P. Smith]

On 8/19/22 15:43, Xenia Ragiadakou wrote:
> In macros SIDTAB_HASH(), INIT_SIDTAB_LOCK(), SIDTAB_LOCK() and SIDTAB_UNLOCK(),
> add parentheses around the macro parameter to prevent against unintended
> expansions.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> ---
>  xen/xsm/flask/ss/sidtab.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/xsm/flask/ss/sidtab.c b/xen/xsm/flask/ss/sidtab.c
> index 74babfac9c..69fc3389b3 100644
> --- a/xen/xsm/flask/ss/sidtab.c
> +++ b/xen/xsm/flask/ss/sidtab.c
> @@ -14,11 +14,11 @@
>  #include "security.h"
>  #include "sidtab.h"
>  
> -#define SIDTAB_HASH(sid) (sid & SIDTAB_HASH_MASK)
> +#define SIDTAB_HASH(sid) ((sid) & SIDTAB_HASH_MASK)
>  
> -#define INIT_SIDTAB_LOCK(s) spin_lock_init(&s->lock)
> -#define SIDTAB_LOCK(s) spin_lock(&s->lock)
> -#define SIDTAB_UNLOCK(s) spin_unlock(&s->lock)
> +#define INIT_SIDTAB_LOCK(s) spin_lock_init(&(s)->lock)
> +#define SIDTAB_LOCK(s) spin_lock(&(s)->lock)
> +#define SIDTAB_UNLOCK(s) spin_unlock(&(s)->lock)
>  
>  int sidtab_init(struct sidtab *s)
>  {

Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>

Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations

[Stefano Stabellini]

On Fri, 26 Aug 2022, Jan Beulich wrote:
> On 26.08.2022 09:58, Xenia Ragiadakou wrote:
> > On 8/26/22 09:21, Jan Beulich wrote:
> >> On 25.08.2022 20:09, Stefano Stabellini wrote:
> >>> But first, let's confirm whether this change:
> >>>
> >>>
> >>>   #define dt_for_each_property_node(dn, pp)                   \
> >>> -    for ( pp = dn->properties; pp != NULL; pp = pp->next )
> >>> +    for ( pp = (dn)->properties; pp != NULL; pp = (pp)->next )
> >>>
> >>>
> >>> is sufficient to make the violation go away in Eclair or cppcheck.  I am
> >>> assuming it is not sufficient, but let's confirm.
> >>
> >> Well, even if for the lhs of assignments there was an exception, this
> >> still wouldn't be sufficient. The minimum needed is
> >>
> >> #define dt_for_each_property_node(dn, pp)                   \
> >>      for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next )

Thank you for noticing this


> > If pp is assumed to be a valid lvalue, then why it is needed to add 
> > parentheses here (pp) != NULL ?
> 
> Because in one expression is doesn't matter that in another expression
> the same identifier is used as the lhs of an assignment. Whether
> parentheses are needed should solely depend on the operators in use,
> not any further context.

This is the problem with going with a more sophisticated version of the
rule: it is not immediately obvious any longer. I have to read this
explanation three times to appreciate what it means, I don't think a new
contributor would really have any chances of getting this right,
especially as cppcheck/Eclair wouldn't be able to help them either.

Re: [PATCH 0/7] Fix MISRA C 2012 Rule 20.7 violations

[Stefano Stabellini]

Patches 1, 4, and 6 are already committed. I plan to commit patches 2, 3
and 5 in the next couple of days.

Patch 7 needs further discussions and it is best addressed during the
next MISRA C sync-up.


On Fri, 19 Aug 2022, Xenia Ragiadakou wrote:
> Xenia Ragiadakou (7):
>   xen/arm: gic_v3_its: Fix MISRA C 2012 Rule 20.7 violations
>   xsm/flask: sidtab: Fix MISRA C 2012 Rule 20.7 violations
>   xen/elf: Fix MISRA C 2012 Rule 20.7 violations
>   xen/vgic: Fix MISRA C 2012 Rule 20.7 violation
>   xen/rbtree: Fix MISRA C 2012 Rule 20.7 violation
>   xen/arm: processor: Fix MISRA C 2012 Rule 20.7 violations
>   xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations
> 
>  xen/arch/arm/include/asm/gic_v3_its.h | 10 +++++-----
>  xen/arch/arm/include/asm/new_vgic.h   |  2 +-
>  xen/arch/arm/include/asm/processor.h  |  4 ++--
>  xen/include/xen/device_tree.h         |  6 +++---
>  xen/include/xen/elfstructs.h          |  4 ++--
>  xen/lib/rbtree.c                      |  2 +-
>  xen/xsm/flask/ss/sidtab.c             |  8 ++++----
>  7 files changed, 18 insertions(+), 18 deletions(-)
> 
> -- 
> 2.34.1
> 

Re: [PATCH 0/7] Fix MISRA C 2012 Rule 20.7 violations

[Xenia Ragiadakou]

On 9/1/22 01:35, Stefano Stabellini wrote:
> Patches 1, 4, and 6 are already committed. I plan to commit patches 2, 3
> and 5 in the next couple of days.
> 
> Patch 7 needs further discussions and it is best addressed during the
> next MISRA C sync-up.
> 

I would like to share here, before the next MISRA C sync, my 
understandings that will hopefully resolve a wrong impression of mine, 
that I may have spread around, regarding this rule.
There was a misunderstanding regarding the rule 20.7 from my part and I 
think that Jan is absolutely right that parenthesizing macro parameters 
used as function arguments is not required by the rule.

The rule 20.7 states "Expressions resulting from the expansion of macro 
parameters shall be enclosed in parentheses" and in the rationale of the 
rule states "If a macro parameter is not being used as an expression 
then the parentheses are not necessary because no operators are involved.".

Initially, based on the title, my understanding was that it requires for 
the expression resulting from the expansion of the macro to be enclosed 
in parentheses. Then, based on the rule explanation and the examples 
given,  my understanding was that it requires the macro parameters that 
are used as expressions to be enclosed in parentheses.
But, after re-thinking about it, the most probable and what makes more 
sense, is that it require parentheses around the macro parameters that 
are part of an expression and not around those that are used as expressions.

Therefore, macro parameters being used as function arguments are not 
required to be enclosed in parentheses, because the function arguments 
are part of an expression list, not of an expression (comma is evaluated 
as separator, not as operator).
While, macro parameters used as rhs and lhs expressions of the 
assignment operator are required to be enclosed in parentheses because 
they are part of an assignment expression.

I verified that the violation reported by cppcheck is not due to missing 
parentheses around the function argument (though still I have not 
understood the origin of the warning). Also, Eclair does not report it.

Hence, it was a misunderstanding of mine and there is no inconsistency, 
with respect to this rule, in adding parentheses around macro parameters 
used as rhs of assignments. The rule does not require adding parentheses 
around macro parameters used as function arguments and neither cppcheck 
nor Eclair report violation for missing parentheses around macro 
parameters used as function arguments.

> 
> On Fri, 19 Aug 2022, Xenia Ragiadakou wrote:
>> Xenia Ragiadakou (7):
>>    xen/arm: gic_v3_its: Fix MISRA C 2012 Rule 20.7 violations
>>    xsm/flask: sidtab: Fix MISRA C 2012 Rule 20.7 violations
>>    xen/elf: Fix MISRA C 2012 Rule 20.7 violations
>>    xen/vgic: Fix MISRA C 2012 Rule 20.7 violation
>>    xen/rbtree: Fix MISRA C 2012 Rule 20.7 violation
>>    xen/arm: processor: Fix MISRA C 2012 Rule 20.7 violations
>>    xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations
>>
>>   xen/arch/arm/include/asm/gic_v3_its.h | 10 +++++-----
>>   xen/arch/arm/include/asm/new_vgic.h   |  2 +-
>>   xen/arch/arm/include/asm/processor.h  |  4 ++--
>>   xen/include/xen/device_tree.h         |  6 +++---
>>   xen/include/xen/elfstructs.h          |  4 ++--
>>   xen/lib/rbtree.c                      |  2 +-
>>   xen/xsm/flask/ss/sidtab.c             |  8 ++++----
>>   7 files changed, 18 insertions(+), 18 deletions(-)
>>
>> -- 
>> 2.34.1
>>

-- 
Xenia

Re: [PATCH 0/7] Fix MISRA C 2012 Rule 20.7 violations

[Bertrand Marquis]

Hi Xenia,

> On 1 Sep 2022, at 10:27, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
> 
> 
> On 9/1/22 01:35, Stefano Stabellini wrote:
>> Patches 1, 4, and 6 are already committed. I plan to commit patches 2, 3
>> and 5 in the next couple of days.
>> Patch 7 needs further discussions and it is best addressed during the
>> next MISRA C sync-up.
> 
> I would like to share here, before the next MISRA C sync, my understandings that will hopefully resolve a wrong impression of mine, that I may have spread around, regarding this rule.
> There was a misunderstanding regarding the rule 20.7 from my part and I think that Jan is absolutely right that parenthesizing macro parameters used as function arguments is not required by the rule.
> 
> The rule 20.7 states "Expressions resulting from the expansion of macro parameters shall be enclosed in parentheses" and in the rationale of the rule states "If a macro parameter is not being used as an expression then the parentheses are not necessary because no operators are involved.".
> 
> Initially, based on the title, my understanding was that it requires for the expression resulting from the expansion of the macro to be enclosed in parentheses. Then, based on the rule explanation and the examples given,  my understanding was that it requires the macro parameters that are used as expressions to be enclosed in parentheses.
> But, after re-thinking about it, the most probable and what makes more sense, is that it require parentheses around the macro parameters that are part of an expression and not around those that are used as expressions.
> 
> Therefore, macro parameters being used as function arguments are not required to be enclosed in parentheses, because the function arguments are part of an expression list, not of an expression (comma is evaluated as separator, not as operator).
> While, macro parameters used as rhs and lhs expressions of the assignment operator are required to be enclosed in parentheses because they are part of an assignment expression.
> 
> I verified that the violation reported by cppcheck is not due to missing parentheses around the function argument (though still I have not understood the origin of the warning). Also, Eclair does not report it.
> 
> Hence, it was a misunderstanding of mine and there is no inconsistency, with respect to this rule, in adding parentheses around macro parameters used as rhs of assignments. The rule does not require adding parentheses around macro parameters used as function arguments and neither cppcheck nor Eclair report violation for missing parentheses around macro parameters used as function arguments.


Thanks a lot for the detailed explanation :-)

What you say does make sense and I agree with your analysis here, only protect when part of an expression and not use as a subsequent parameter (for a function or an other macro).

Regards
Bertrand

> 
>> On Fri, 19 Aug 2022, Xenia Ragiadakou wrote:
>>> Xenia Ragiadakou (7):
>>>   xen/arm: gic_v3_its: Fix MISRA C 2012 Rule 20.7 violations
>>>   xsm/flask: sidtab: Fix MISRA C 2012 Rule 20.7 violations
>>>   xen/elf: Fix MISRA C 2012 Rule 20.7 violations
>>>   xen/vgic: Fix MISRA C 2012 Rule 20.7 violation
>>>   xen/rbtree: Fix MISRA C 2012 Rule 20.7 violation
>>>   xen/arm: processor: Fix MISRA C 2012 Rule 20.7 violations
>>>   xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations
>>> 
>>>  xen/arch/arm/include/asm/gic_v3_its.h | 10 +++++-----
>>>  xen/arch/arm/include/asm/new_vgic.h   |  2 +-
>>>  xen/arch/arm/include/asm/processor.h  |  4 ++--
>>>  xen/include/xen/device_tree.h         |  6 +++---
>>>  xen/include/xen/elfstructs.h          |  4 ++--
>>>  xen/lib/rbtree.c                      |  2 +-
>>>  xen/xsm/flask/ss/sidtab.c             |  8 ++++----
>>>  7 files changed, 18 insertions(+), 18 deletions(-)
>>> 
>>> -- 
>>> 2.34.1
>>> 
> 
> -- 
> Xenia

Re: [PATCH 0/7] Fix MISRA C 2012 Rule 20.7 violations

[Stefano Stabellini]

On Thu, 1 Sep 2022, Bertrand Marquis wrote:
> Hi Xenia,
> 
> > On 1 Sep 2022, at 10:27, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
> > 
> > 
> > On 9/1/22 01:35, Stefano Stabellini wrote:
> >> Patches 1, 4, and 6 are already committed. I plan to commit patches 2, 3
> >> and 5 in the next couple of days.
> >> Patch 7 needs further discussions and it is best addressed during the
> >> next MISRA C sync-up.
> > 
> > I would like to share here, before the next MISRA C sync, my understandings that will hopefully resolve a wrong impression of mine, that I may have spread around, regarding this rule.
> > There was a misunderstanding regarding the rule 20.7 from my part and I think that Jan is absolutely right that parenthesizing macro parameters used as function arguments is not required by the rule.
> > 
> > The rule 20.7 states "Expressions resulting from the expansion of macro parameters shall be enclosed in parentheses" and in the rationale of the rule states "If a macro parameter is not being used as an expression then the parentheses are not necessary because no operators are involved.".
> > 
> > Initially, based on the title, my understanding was that it requires for the expression resulting from the expansion of the macro to be enclosed in parentheses. Then, based on the rule explanation and the examples given,  my understanding was that it requires the macro parameters that are used as expressions to be enclosed in parentheses.
> > But, after re-thinking about it, the most probable and what makes more sense, is that it require parentheses around the macro parameters that are part of an expression and not around those that are used as expressions.
> > 
> > Therefore, macro parameters being used as function arguments are not required to be enclosed in parentheses, because the function arguments are part of an expression list, not of an expression (comma is evaluated as separator, not as operator).
> > While, macro parameters used as rhs and lhs expressions of the assignment operator are required to be enclosed in parentheses because they are part of an assignment expression.
> > 
> > I verified that the violation reported by cppcheck is not due to missing parentheses around the function argument (though still I have not understood the origin of the warning). Also, Eclair does not report it.
> > 
> > Hence, it was a misunderstanding of mine and there is no inconsistency, with respect to this rule, in adding parentheses around macro parameters used as rhs of assignments. The rule does not require adding parentheses around macro parameters used as function arguments and neither cppcheck nor Eclair report violation for missing parentheses around macro parameters used as function arguments.
> 
> 
> Thanks a lot for the detailed explanation :-)
> 
> What you say does make sense and I agree with your analysis here, only protect when part of an expression and not use as a subsequent parameter (for a function or an other macro).

Yeah I also agree with your analysis, and many thanks for
double-checking the cppcheck and Eclair's reports.

Re: [PATCH 0/7] Fix MISRA C 2012 Rule 20.7 violations

[Xenia Ragiadakou]

On 9/2/22 05:07, Stefano Stabellini wrote:
> On Thu, 1 Sep 2022, Bertrand Marquis wrote:
>> Hi Xenia,
>>
>>> On 1 Sep 2022, at 10:27, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
>>>
>>>
>>> On 9/1/22 01:35, Stefano Stabellini wrote:
>>>> Patches 1, 4, and 6 are already committed. I plan to commit patches 2, 3
>>>> and 5 in the next couple of days.
>>>> Patch 7 needs further discussions and it is best addressed during the
>>>> next MISRA C sync-up.
>>>
>>> I would like to share here, before the next MISRA C sync, my understandings that will hopefully resolve a wrong impression of mine, that I may have spread around, regarding this rule.
>>> There was a misunderstanding regarding the rule 20.7 from my part and I think that Jan is absolutely right that parenthesizing macro parameters used as function arguments is not required by the rule.
>>>
>>> The rule 20.7 states "Expressions resulting from the expansion of macro parameters shall be enclosed in parentheses" and in the rationale of the rule states "If a macro parameter is not being used as an expression then the parentheses are not necessary because no operators are involved.".
>>>
>>> Initially, based on the title, my understanding was that it requires for the expression resulting from the expansion of the macro to be enclosed in parentheses. Then, based on the rule explanation and the examples given,  my understanding was that it requires the macro parameters that are used as expressions to be enclosed in parentheses.
>>> But, after re-thinking about it, the most probable and what makes more sense, is that it require parentheses around the macro parameters that are part of an expression and not around those that are used as expressions.
>>>
>>> Therefore, macro parameters being used as function arguments are not required to be enclosed in parentheses, because the function arguments are part of an expression list, not of an expression (comma is evaluated as separator, not as operator).
>>> While, macro parameters used as rhs and lhs expressions of the assignment operator are required to be enclosed in parentheses because they are part of an assignment expression.
>>>
>>> I verified that the violation reported by cppcheck is not due to missing parentheses around the function argument (though still I have not understood the origin of the warning). Also, Eclair does not report it.
>>>
>>> Hence, it was a misunderstanding of mine and there is no inconsistency, with respect to this rule, in adding parentheses around macro parameters used as rhs of assignments. The rule does not require adding parentheses around macro parameters used as function arguments and neither cppcheck nor Eclair report violation for missing parentheses around macro parameters used as function arguments.
>>
>>
>> Thanks a lot for the detailed explanation :-)
>>
>> What you say does make sense and I agree with your analysis here, only protect when part of an expression and not use as a subsequent parameter (for a function or an other macro).
> 
> Yeah I also agree with your analysis, and many thanks for
> double-checking the cppcheck and Eclair's reports.

Unfortunately in the specific case that I checked, it was not reported 
because it was actually an argument to a macro, not a function.
Eclair does report as violations of Rule 20.7 the macro parameters that 
are used as function arguments and are not enclosed in parentheses.

So, one tool reports it as violation and the other one not.

The same goes, also, for the case where a macro parameter is used as 
index to an array. Eclair reports it as violation while cppcheck does not.

-- 
Xenia

Re: [PATCH 0/7] Fix MISRA C 2012 Rule 20.7 violations

[Stefano Stabellini]

+Roberto

I think we need Roberto's advice on Rule 20.7. (Full thread below.)

The question is on the interpretation of Rule 20.7. Are parenthesis
required by Rule 20.7 in the following cases:

- macro parameters used as function arguments 
- macro parameters used as macro arguments
- macro parameter used as array index
- macro parameter used as lhs in assignment

Some of these cases are interesting because they should function
correctly even without parenthesis, hence the discussion. In particular
parenthesis don't seem necessary at least for the function argument
case.

Regardless of the MISRA C interpretation, Xenia noticed that Eclair
reports violations on these cases (cppcheck does not, I don't know other
checkers).



On Fri, 2 Sep 2022, Xenia Ragiadakou wrote:
> On 9/2/22 05:07, Stefano Stabellini wrote:
> > On Thu, 1 Sep 2022, Bertrand Marquis wrote:
> > > Hi Xenia,
> > > 
> > > > On 1 Sep 2022, at 10:27, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
> > > > 
> > > > 
> > > > On 9/1/22 01:35, Stefano Stabellini wrote:
> > > > > Patches 1, 4, and 6 are already committed. I plan to commit patches 2,
> > > > > 3
> > > > > and 5 in the next couple of days.
> > > > > Patch 7 needs further discussions and it is best addressed during the
> > > > > next MISRA C sync-up.
> > > > 
> > > > I would like to share here, before the next MISRA C sync, my
> > > > understandings that will hopefully resolve a wrong impression of mine,
> > > > that I may have spread around, regarding this rule.
> > > > There was a misunderstanding regarding the rule 20.7 from my part and I
> > > > think that Jan is absolutely right that parenthesizing macro parameters
> > > > used as function arguments is not required by the rule.
> > > > 
> > > > The rule 20.7 states "Expressions resulting from the expansion of macro
> > > > parameters shall be enclosed in parentheses" and in the rationale of the
> > > > rule states "If a macro parameter is not being used as an expression
> > > > then the parentheses are not necessary because no operators are
> > > > involved.".
> > > > 
> > > > Initially, based on the title, my understanding was that it requires for
> > > > the expression resulting from the expansion of the macro to be enclosed
> > > > in parentheses. Then, based on the rule explanation and the examples
> > > > given,  my understanding was that it requires the macro parameters that
> > > > are used as expressions to be enclosed in parentheses.
> > > > But, after re-thinking about it, the most probable and what makes more
> > > > sense, is that it require parentheses around the macro parameters that
> > > > are part of an expression and not around those that are used as
> > > > expressions.
> > > > 
> > > > Therefore, macro parameters being used as function arguments are not
> > > > required to be enclosed in parentheses, because the function arguments
> > > > are part of an expression list, not of an expression (comma is evaluated
> > > > as separator, not as operator).
> > > > While, macro parameters used as rhs and lhs expressions of the
> > > > assignment operator are required to be enclosed in parentheses because
> > > > they are part of an assignment expression.
> > > > 
> > > > I verified that the violation reported by cppcheck is not due to missing
> > > > parentheses around the function argument (though still I have not
> > > > understood the origin of the warning). Also, Eclair does not report it.
> > > > 
> > > > Hence, it was a misunderstanding of mine and there is no inconsistency,
> > > > with respect to this rule, in adding parentheses around macro parameters
> > > > used as rhs of assignments. The rule does not require adding parentheses
> > > > around macro parameters used as function arguments and neither cppcheck
> > > > nor Eclair report violation for missing parentheses around macro
> > > > parameters used as function arguments.
> > > 
> > > 
> > > Thanks a lot for the detailed explanation :-)
> > > 
> > > What you say does make sense and I agree with your analysis here, only
> > > protect when part of an expression and not use as a subsequent parameter
> > > (for a function or an other macro).
> > 
> > Yeah I also agree with your analysis, and many thanks for
> > double-checking the cppcheck and Eclair's reports.
> 
> Unfortunately in the specific case that I checked, it was not reported because
> it was actually an argument to a macro, not a function.
> Eclair does report as violations of Rule 20.7 the macro parameters that are
> used as function arguments and are not enclosed in parentheses.
> 
> So, one tool reports it as violation and the other one not.
> 
> The same goes, also, for the case where a macro parameter is used as index to
> an array. Eclair reports it as violation while cppcheck does not.

Re: [PATCH 0/7] Fix MISRA C 2012 Rule 20.7 violations

[Roberto Bagnara]

On 03/09/22 02:52, Stefano Stabellini wrote:
> +Roberto
> 
> I think we need Roberto's advice on Rule 20.7. (Full thread below.)

Hi there, sorry for the delay: I missed this message.
Please see below, where I took the freedom of rearranging the
cases.

> The question is on the interpretation of Rule 20.7. Are parenthesis
> required by Rule 20.7 in the following cases:
> 
> - macro parameters used as function arguments
 > [...]
 > - macro parameter used as lhs in assignment

You can obtain different semantics depending on whether parentheses
are or are not used (in the macro call and/or macro expansion
depending on the case):


#include <stdio.h>

void g(int v) {
   printf("%d\n", v);
}

#define m1(x, y, ...) g(y)

void f1(int x, int y, ...) {
   g(y);
}

#define p 0, 1

void test1() {
   m1(p, 2);
   f1(p, 2);
}

#define m4(x) x = 4

void f4(int &x) {
   x = 4;
}


void test4() {
   int y;
   int z;
   z = 3;
   m4(y = z);
   printf("%d\n", z);
   z = 3;
   f4(y = z);
   printf("%d\n", z);
}

int main() {
   test1();
   test4();
}

> - macro parameters used as macro arguments

Please note that Rule 20.7 depends on the final expansion:
so whether parentheses are or are not used in a certain
macro body is irrelevant, the point being that, at the
end of all expansions, expressions resulting from the
expansion of macro parameters are enclosed in parentheses.

> - macro parameter used as array index

This is safe today, but my understanding is that in C++23
the [] operator will accept more than one expression.
A similar change might (who knows?) be considered for C26
or even offered before (intentionally or by mistake) by some
C compiler.

> Some of these cases are interesting because they should function
> correctly even without parenthesis, hence the discussion. In particular
> parenthesis don't seem necessary at least for the function argument
> case.

This is not the right spirit for MISRA compliance: why would you want
splitting hairs when inserting a pair of parentheses is so easy?
C and C++ are very complex languages, and the MISRA coding standards
are the result of a (very difficult!) compromise between simplicity
and effectiveness: rules that are exactly targeted to all and only all
the problematic instances would be very difficult to express and to remember.
So, yes: in many cases you might spend time to demonstrate that a particular
(real) MISRA violation does not imply the existence of a real issue,
but this time is not well spent.  Critical code must be boring and obviously
right, in the sense that whomever is reading the code should not be
distracted by thoughts like "there are no parentheses here: am I sure
nothing bad can happen?"

> Regardless of the MISRA C interpretation, Xenia noticed that Eclair
> reports violations on these cases (cppcheck does not, I don't know other
> checkers).

I am not aware of any false positives (or flse negatives) for the
current version of ECLAIR on Rule 20.7.  Nonetheless, ECLAIR can
be configured to selectively deviate on each of the cases mentioned
above by means of checker configuration.  However, as I said,
it only makes sense deviating the rule in the cases where you are
not allowed to add the parentheses (e.g., because both the macro
definition and the macro invocations are in legacy code you are
not allowed to touch).

In contrast, cppcheck is no more than a toy when MISRA compliance
is concerned.  It claims to support 153 out of 175 MISRA C:2012 guidelines.
For 103 of those 153 it has a significant number of false negatives (FN)
and false positives (FP).  I recently participated to an evaluation
of cppcheck 2.8 and here is a summary I can disclose:

Rule 1.3               FP
Rule 2.1               FN
Rule 2.2               FN+FP
Rule 2.4               FN+FP
Rule 2.5               FP
Rule 2.7               FP
Rule 3.2               FN
Rule 4.2               FN
Rule 5.1               FP
Rule 5.3               FN
Rule 5.6               FN+FP
Rule 5.7               FN+FP
Rule 5.8               FN+FP
Rule 5.9               FN+FP
Rule 6.1               FN+FP
Rule 7.1               FN
Rule 7.3               FN
Rule 7.4               FN+FP
Rule 8.1               FN
Rule 8.2               FN+FP
Rule 8.3               FN
Rule 8.4               FP
Rule 8.5               FN+FP
Rule 8.6               FP
Rule 8.7               FN
Rule 8.8               FN
Rule 8.9               FN
Rule 8.10              FN
Rule 8.13              FN
Rule 8.14              FP
Rule 9.1               FN+FP
Rule 9.3               FN
Rule 10.1              FN
Rule 10.2              FN
Rule 10.3              FN+FP
Rule 10.4              FP
Rule 10.5              FN+FP
Rule 10.6              FP
Rule 10.7              FN+FP
Rule 10.8              FP
Rule 11.1              FN+FP
Rule 11.2              FN
Rule 11.3              FN+FP
Rule 11.4              FP
Rule 11.5              FP
Rule 11.7              FN
Rule 11.8              FN+FP
Rule 11.9              FN
Rule 12.1              FN
Rule 12.2              FP
Rule 12.3              FP
Rule 13.1              FN
Rule 13.2              FN
Rule 13.4              FP
Rule 13.5              FN
Rule 13.6              FP
Rule 14.2              FN
Rule 14.3              FN
Rule 15.5              FN+FP
Rule 15.6              FN+FP
Rule 16.1              FN
Rule 16.3              FN
Rule 16.6              FP
Rule 16.7              FP
Rule 17.1              FP
Rule 17.2              FN+FP
Rule 17.4              FN
Rule 17.5              FN
Rule 17.7              FP
Rule 18.1              FN
Rule 18.3              FN
Rule 18.4              FP
Rule 19.1              FN
Rule 19.2              FP
Rule 20.2              FN
Rule 20.4              FP
Rule 20.5              FN
Rule 20.7              FP
Rule 20.9              FN
Rule 20.10             FP
Rule 20.12             FP
Rule 21.1              FN+FP
Rule 21.2              FN
Rule 21.3              FP
Rule 21.6              FP
Rule 21.8              FN+FP
Rule 21.12             FN
Rule 21.13             FP
Rule 21.14             FN
Rule 21.15             FN
Rule 21.16             FN+FP
Rule 21.17             FN
Rule 21.18             FN
Rule 21.19             FN
Rule 21.20             FN
Rule 22.1              FP
Rule 22.2              FN+FP
Rule 22.5              FN
Rule 22.6              FN
Rule 22.7              FN
Rule 22.8              FN+FP
Rule 22.9              FN+FP
Rule 22.10             FP

These results are clearly relative to the testsuite employed:
while very large, it cannot of course reach 100% coverage.
For instance, if you noticed Rule 20.7 reports given by
ECLAIR and not by cppcheck, then maybe line

Rule 20.7              FP

should be

Rule 20.7              FN+FP

If you can let me have an indication of the code that
ECLAIR is flagging for Rule 20.7 and cppcheck does not
flag, I will be happy to double-check.

While the sheer amount of false negatives of cppcheck 2.8 precludes
its use for safety-related development, the many false positives
are also a big problem: people will waste time investigating
them and, unless they have been properly trained on the
MISRA guidelines so as to be able to recognize false positives,
they might be tempted to change the code when there is no
reason to do so.  When the latter thing happens, code quality
will typically decrease.

Kind regards,

    Roberto

> On Fri, 2 Sep 2022, Xenia Ragiadakou wrote:
>> On 9/2/22 05:07, Stefano Stabellini wrote:
>>> On Thu, 1 Sep 2022, Bertrand Marquis wrote:
>>>> Hi Xenia,
>>>>
>>>>> On 1 Sep 2022, at 10:27, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
>>>>>
>>>>>
>>>>> On 9/1/22 01:35, Stefano Stabellini wrote:
>>>>>> Patches 1, 4, and 6 are already committed. I plan to commit patches 2,
>>>>>> 3
>>>>>> and 5 in the next couple of days.
>>>>>> Patch 7 needs further discussions and it is best addressed during the
>>>>>> next MISRA C sync-up.
>>>>>
>>>>> I would like to share here, before the next MISRA C sync, my
>>>>> understandings that will hopefully resolve a wrong impression of mine,
>>>>> that I may have spread around, regarding this rule.
>>>>> There was a misunderstanding regarding the rule 20.7 from my part and I
>>>>> think that Jan is absolutely right that parenthesizing macro parameters
>>>>> used as function arguments is not required by the rule.
>>>>>
>>>>> The rule 20.7 states "Expressions resulting from the expansion of macro
>>>>> parameters shall be enclosed in parentheses" and in the rationale of the
>>>>> rule states "If a macro parameter is not being used as an expression
>>>>> then the parentheses are not necessary because no operators are
>>>>> involved.".
>>>>>
>>>>> Initially, based on the title, my understanding was that it requires for
>>>>> the expression resulting from the expansion of the macro to be enclosed
>>>>> in parentheses. Then, based on the rule explanation and the examples
>>>>> given,  my understanding was that it requires the macro parameters that
>>>>> are used as expressions to be enclosed in parentheses.
>>>>> But, after re-thinking about it, the most probable and what makes more
>>>>> sense, is that it require parentheses around the macro parameters that
>>>>> are part of an expression and not around those that are used as
>>>>> expressions.
>>>>>
>>>>> Therefore, macro parameters being used as function arguments are not
>>>>> required to be enclosed in parentheses, because the function arguments
>>>>> are part of an expression list, not of an expression (comma is evaluated
>>>>> as separator, not as operator).
>>>>> While, macro parameters used as rhs and lhs expressions of the
>>>>> assignment operator are required to be enclosed in parentheses because
>>>>> they are part of an assignment expression.
>>>>>
>>>>> I verified that the violation reported by cppcheck is not due to missing
>>>>> parentheses around the function argument (though still I have not
>>>>> understood the origin of the warning). Also, Eclair does not report it.
>>>>>
>>>>> Hence, it was a misunderstanding of mine and there is no inconsistency,
>>>>> with respect to this rule, in adding parentheses around macro parameters
>>>>> used as rhs of assignments. The rule does not require adding parentheses
>>>>> around macro parameters used as function arguments and neither cppcheck
>>>>> nor Eclair report violation for missing parentheses around macro
>>>>> parameters used as function arguments.
>>>>
>>>>
>>>> Thanks a lot for the detailed explanation :-)
>>>>
>>>> What you say does make sense and I agree with your analysis here, only
>>>> protect when part of an expression and not use as a subsequent parameter
>>>> (for a function or an other macro).
>>>
>>> Yeah I also agree with your analysis, and many thanks for
>>> double-checking the cppcheck and Eclair's reports.
>>
>> Unfortunately in the specific case that I checked, it was not reported because
>> it was actually an argument to a macro, not a function.
>> Eclair does report as violations of Rule 20.7 the macro parameters that are
>> used as function arguments and are not enclosed in parentheses.
>>
>> So, one tool reports it as violation and the other one not.
>>
>> The same goes, also, for the case where a macro parameter is used as index to
>> an array. Eclair reports it as violation while cppcheck does not.
> 

Re: [PATCH 0/7] Fix MISRA C 2012 Rule 20.7 violations

[Jan Beulich]

On 18.09.2022 15:02, Roberto Bagnara wrote:
> On 03/09/22 02:52, Stefano Stabellini wrote:
>> The question is on the interpretation of Rule 20.7. Are parenthesis
>> required by Rule 20.7 in the following cases:
>>
>> - macro parameters used as function arguments
>  > [...]
>  > - macro parameter used as lhs in assignment
> 
> You can obtain different semantics depending on whether parentheses
> are or are not used (in the macro call and/or macro expansion
> depending on the case):
> 
> 
> #include <stdio.h>
> 
> void g(int v) {
>    printf("%d\n", v);
> }
> 
> #define m1(x, y, ...) g(y)
> 
> void f1(int x, int y, ...) {
>    g(y);
> }
> 
> #define p 0, 1
> 
> void test1() {
>    m1(p, 2);
>    f1(p, 2);
> }
> 
> #define m4(x) x = 4
> 
> void f4(int &x) {

Let's focus on C here.

>    x = 4;
> }
> 
> 
> void test4() {
>    int y;
>    int z;
>    z = 3;
>    m4(y = z);
>    printf("%d\n", z);
>    z = 3;
>    f4(y = z);
>    printf("%d\n", z);
> }
> 
> int main() {
>    test1();
>    test4();
> }
> 
>> - macro parameters used as macro arguments
> 
> Please note that Rule 20.7 depends on the final expansion:
> so whether parentheses are or are not used in a certain
> macro body is irrelevant, the point being that, at the
> end of all expansions, expressions resulting from the
> expansion of macro parameters are enclosed in parentheses.
> 
>> - macro parameter used as array index
> 
> This is safe today, but my understanding is that in C++23
> the [] operator will accept more than one expression.
> A similar change might (who knows?) be considered for C26
> or even offered before (intentionally or by mistake) by some
> C compiler.
> 
>> Some of these cases are interesting because they should function
>> correctly even without parenthesis, hence the discussion. In particular
>> parenthesis don't seem necessary at least for the function argument
>> case.
> 
> This is not the right spirit for MISRA compliance: why would you want
> splitting hairs when inserting a pair of parentheses is so easy?

I think I've said so before - too many parentheses harm readability.

> C and C++ are very complex languages, and the MISRA coding standards
> are the result of a (very difficult!) compromise between simplicity
> and effectiveness: rules that are exactly targeted to all and only all
> the problematic instances would be very difficult to express and to remember.
> So, yes: in many cases you might spend time to demonstrate that a particular
> (real) MISRA violation does not imply the existence of a real issue,
> but this time is not well spent.  Critical code must be boring and obviously
> right, in the sense that whomever is reading the code should not be
> distracted by thoughts like "there are no parentheses here: am I sure
> nothing bad can happen?"

I also did indicate before that "(x) = ..." visually (but not
syntactically) can raise the question of whether the left side actually
is an lvalue.

Jan

Re: [PATCH 0/7] Fix MISRA C 2012 Rule 20.7 violations

[Xenia Ragiadakou]

Hi Roberto,

On 9/18/22 16:02, Roberto Bagnara wrote:
> On 03/09/22 02:52, Stefano Stabellini wrote:
>> +Roberto
>>
>> I think we need Roberto's advice on Rule 20.7. (Full thread below.)
> 
> Hi there, sorry for the delay: I missed this message.
> Please see below, where I took the freedom of rearranging the
> cases.
> 
>> The question is on the interpretation of Rule 20.7. Are parenthesis
>> required by Rule 20.7 in the following cases:
>>
>> - macro parameters used as function arguments
>  > [...]
>  > - macro parameter used as lhs in assignment
> 
> You can obtain different semantics depending on whether parentheses
> are or are not used (in the macro call and/or macro expansion
> depending on the case):
> 
> 
> #include <stdio.h>
> 
> void g(int v) {
>    printf("%d\n", v);
> }
> 
> #define m1(x, y, ...) g(y)
> 
> void f1(int x, int y, ...) {
>    g(y);
> }
> 
> #define p 0, 1
> 
> void test1() {
>    m1(p, 2);
>    f1(p, 2);
> }
> 

In the example above something bothers me. Let me explain.

Running the above example gives:
2
1

The results differ mainly because m1() is substituted before p.
Thus, adding parentheses around the macro parameter 'y' of m1() i.e
#define m1(x, y, ...) g((y))
has no impact.

If the example is changed into the following:

#include <stdio.h>

void g(int v) {
    printf("%d\n", v);
}

#define m1(y, ...) g(y)

void f1(int y, ...) {
    g(y);
}

#define p 0, 1

void test1() {
     m1(p, 2);
     f1(p, 2);
}

if no parentheses are added around 'y' in the definition of m1(), the 
compiler complains with "too many arguments to function g".
If parentheses are added around 'y', the compiler does not complain but 
the behavior will still differ and the result will be
1
0

This happens because in the case of m1(), p is interpreted as an 
expression (due to the parentheses added there) and the comma is 
evaluated as a comma operator, while in f1(), p is interpreted as a list 
of expressions and the comma is evaluated as a comma separator.

Hence, in my opinion, parentheses should not be added around macro 
parameters used as function arguments because they can hide a bug due to 
missing parentheses around the entire macro definition.
Since macro 'p' is supposed to represent an expression, and the 
semantics of the comma token are those of a comma operator and not a 
comma separator, then parentheses need to be placed around the entire 
macro definition i.e
#define p (0, 1)

AFAIK, there is no requirement in MISRA C guidelines to add parentheses 
around the entire macro definition when it is used as an expression and 
this is something I cannot understand.
Unless I got it all wrong I guess ...

> #define m4(x) x = 4
> 
> void f4(int &x) {
>    x = 4;
> }
> 
> 
> void test4() {
>    int y;
>    int z;
>    z = 3;
>    m4(y = z);
>    printf("%d\n", z);
>    z = 3;
>    f4(y = z);
>    printf("%d\n", z);
> }
> 
> int main() {
>    test1();
>    test4();
> }
> 
>> - macro parameters used as macro arguments
> 
> Please note that Rule 20.7 depends on the final expansion:
> so whether parentheses are or are not used in a certain
> macro body is irrelevant, the point being that, at the
> end of all expansions, expressions resulting from the
> expansion of macro parameters are enclosed in parentheses.
> 
>> - macro parameter used as array index
> 
> This is safe today, but my understanding is that in C++23
> the [] operator will accept more than one expression.
> A similar change might (who knows?) be considered for C26
> or even offered before (intentionally or by mistake) by some
> C compiler.
> 

Can a deviation being added in the basis of C99 standard since according 
to the standard, E1[E2] is identical to (*((E1)+(E2))), and therefore, 
macro parameters used as subscript expressions are implicitly
parenthesized and can be exempted from the rule.


>> Some of these cases are interesting because they should function
>> correctly even without parenthesis, hence the discussion. In particular
>> parenthesis don't seem necessary at least for the function argument
>> case.
> 
> This is not the right spirit for MISRA compliance: why would you want
> splitting hairs when inserting a pair of parentheses is so easy?
> C and C++ are very complex languages, and the MISRA coding standards
> are the result of a (very difficult!) compromise between simplicity
> and effectiveness: rules that are exactly targeted to all and only all
> the problematic instances would be very difficult to express and to 
> remember.
> So, yes: in many cases you might spend time to demonstrate that a 
> particular
> (real) MISRA violation does not imply the existence of a real issue,
> but this time is not well spent.  Critical code must be boring and 
> obviously
> right, in the sense that whomever is reading the code should not be
> distracted by thoughts like "there are no parentheses here: am I sure
> nothing bad can happen?"
> 
>> Regardless of the MISRA C interpretation, Xenia noticed that Eclair
>> reports violations on these cases (cppcheck does not, I don't know other
>> checkers).
> 
> I am not aware of any false positives (or flse negatives) for the
> current version of ECLAIR on Rule 20.7.  Nonetheless, ECLAIR can
> be configured to selectively deviate on each of the cases mentioned
> above by means of checker configuration.  However, as I said,
> it only makes sense deviating the rule in the cases where you are
> not allowed to add the parentheses (e.g., because both the macro
> definition and the macro invocations are in legacy code you are
> not allowed to touch).
> 
> In contrast, cppcheck is no more than a toy when MISRA compliance
> is concerned.  It claims to support 153 out of 175 MISRA C:2012 guidelines.
> For 103 of those 153 it has a significant number of false negatives (FN)
> and false positives (FP).  I recently participated to an evaluation
> of cppcheck 2.8 and here is a summary I can disclose:
> 
> Rule 1.3               FP
> Rule 2.1               FN
> Rule 2.2               FN+FP
> Rule 2.4               FN+FP
> Rule 2.5               FP
> Rule 2.7               FP
> Rule 3.2               FN
> Rule 4.2               FN
> Rule 5.1               FP
> Rule 5.3               FN
> Rule 5.6               FN+FP
> Rule 5.7               FN+FP
> Rule 5.8               FN+FP
> Rule 5.9               FN+FP
> Rule 6.1               FN+FP
> Rule 7.1               FN
> Rule 7.3               FN
> Rule 7.4               FN+FP
> Rule 8.1               FN
> Rule 8.2               FN+FP
> Rule 8.3               FN
> Rule 8.4               FP
> Rule 8.5               FN+FP
> Rule 8.6               FP
> Rule 8.7               FN
> Rule 8.8               FN
> Rule 8.9               FN
> Rule 8.10              FN
> Rule 8.13              FN
> Rule 8.14              FP
> Rule 9.1               FN+FP
> Rule 9.3               FN
> Rule 10.1              FN
> Rule 10.2              FN
> Rule 10.3              FN+FP
> Rule 10.4              FP
> Rule 10.5              FN+FP
> Rule 10.6              FP
> Rule 10.7              FN+FP
> Rule 10.8              FP
> Rule 11.1              FN+FP
> Rule 11.2              FN
> Rule 11.3              FN+FP
> Rule 11.4              FP
> Rule 11.5              FP
> Rule 11.7              FN
> Rule 11.8              FN+FP
> Rule 11.9              FN
> Rule 12.1              FN
> Rule 12.2              FP
> Rule 12.3              FP
> Rule 13.1              FN
> Rule 13.2              FN
> Rule 13.4              FP
> Rule 13.5              FN
> Rule 13.6              FP
> Rule 14.2              FN
> Rule 14.3              FN
> Rule 15.5              FN+FP
> Rule 15.6              FN+FP
> Rule 16.1              FN
> Rule 16.3              FN
> Rule 16.6              FP
> Rule 16.7              FP
> Rule 17.1              FP
> Rule 17.2              FN+FP
> Rule 17.4              FN
> Rule 17.5              FN
> Rule 17.7              FP
> Rule 18.1              FN
> Rule 18.3              FN
> Rule 18.4              FP
> Rule 19.1              FN
> Rule 19.2              FP
> Rule 20.2              FN
> Rule 20.4              FP
> Rule 20.5              FN
> Rule 20.7              FP
> Rule 20.9              FN
> Rule 20.10             FP
> Rule 20.12             FP
> Rule 21.1              FN+FP
> Rule 21.2              FN
> Rule 21.3              FP
> Rule 21.6              FP
> Rule 21.8              FN+FP
> Rule 21.12             FN
> Rule 21.13             FP
> Rule 21.14             FN
> Rule 21.15             FN
> Rule 21.16             FN+FP
> Rule 21.17             FN
> Rule 21.18             FN
> Rule 21.19             FN
> Rule 21.20             FN
> Rule 22.1              FP
> Rule 22.2              FN+FP
> Rule 22.5              FN
> Rule 22.6              FN
> Rule 22.7              FN
> Rule 22.8              FN+FP
> Rule 22.9              FN+FP
> Rule 22.10             FP
> 
> These results are clearly relative to the testsuite employed:
> while very large, it cannot of course reach 100% coverage.
> For instance, if you noticed Rule 20.7 reports given by
> ECLAIR and not by cppcheck, then maybe line
> 
> Rule 20.7              FP
> 
> should be
> 
> Rule 20.7              FN+FP
> 
> If you can let me have an indication of the code that
> ECLAIR is flagging for Rule 20.7 and cppcheck does not
> flag, I will be happy to double-check.

ECLAIR flags as violations of Rule 20.7 the cases where unparenthesized 
macro parameters are used as (1) function arguments or (2) array 
indexes, while cppcheck does not.

For instance:
(1) in xen/arch/arm/include/asm/atomic.h
#define read_atomic(p) ({                                               \
     union { typeof(*(p)) val; char c[0]; } x_;                          \
     read_atomic_size(p, x_.c, sizeof(*(p)));                            \
     x_.val;                                                             \
})
ECLAIR flags as violations missing parentheses around 'p', when used as 
an argument of read_atomic_size().

(2) in xen/arch/arm/arm64/cpufeature.c
#define SANITIZE_REG(field, num, reg)  \
	sanitize_reg(&system_cpuinfo.field.bits[num], new->field.bits[num], \
				 #reg, ftr_##reg)
ECLAIR flags as violations missing parentheses around 'num'.

> 
> While the sheer amount of false negatives of cppcheck 2.8 precludes
> its use for safety-related development, the many false positives
> are also a big problem: people will waste time investigating
> them and, unless they have been properly trained on the
> MISRA guidelines so as to be able to recognize false positives,
> they might be tempted to change the code when there is no
> reason to do so.  When the latter thing happens, code quality
> will typically decrease.
> 
> Kind regards,
> 
>     Roberto
> 
>> On Fri, 2 Sep 2022, Xenia Ragiadakou wrote:
>>> On 9/2/22 05:07, Stefano Stabellini wrote:
>>>> On Thu, 1 Sep 2022, Bertrand Marquis wrote:
>>>>> Hi Xenia,
>>>>>
>>>>>> On 1 Sep 2022, at 10:27, Xenia Ragiadakou <burzalodowa@gmail.com> 
>>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> On 9/1/22 01:35, Stefano Stabellini wrote:
>>>>>>> Patches 1, 4, and 6 are already committed. I plan to commit 
>>>>>>> patches 2,
>>>>>>> 3
>>>>>>> and 5 in the next couple of days.
>>>>>>> Patch 7 needs further discussions and it is best addressed during 
>>>>>>> the
>>>>>>> next MISRA C sync-up.
>>>>>>
>>>>>> I would like to share here, before the next MISRA C sync, my
>>>>>> understandings that will hopefully resolve a wrong impression of 
>>>>>> mine,
>>>>>> that I may have spread around, regarding this rule.
>>>>>> There was a misunderstanding regarding the rule 20.7 from my part 
>>>>>> and I
>>>>>> think that Jan is absolutely right that parenthesizing macro 
>>>>>> parameters
>>>>>> used as function arguments is not required by the rule.
>>>>>>
>>>>>> The rule 20.7 states "Expressions resulting from the expansion of 
>>>>>> macro
>>>>>> parameters shall be enclosed in parentheses" and in the rationale 
>>>>>> of the
>>>>>> rule states "If a macro parameter is not being used as an expression
>>>>>> then the parentheses are not necessary because no operators are
>>>>>> involved.".
>>>>>>
>>>>>> Initially, based on the title, my understanding was that it 
>>>>>> requires for
>>>>>> the expression resulting from the expansion of the macro to be 
>>>>>> enclosed
>>>>>> in parentheses. Then, based on the rule explanation and the examples
>>>>>> given,  my understanding was that it requires the macro parameters 
>>>>>> that
>>>>>> are used as expressions to be enclosed in parentheses.
>>>>>> But, after re-thinking about it, the most probable and what makes 
>>>>>> more
>>>>>> sense, is that it require parentheses around the macro parameters 
>>>>>> that
>>>>>> are part of an expression and not around those that are used as
>>>>>> expressions.
>>>>>>
>>>>>> Therefore, macro parameters being used as function arguments are not
>>>>>> required to be enclosed in parentheses, because the function 
>>>>>> arguments
>>>>>> are part of an expression list, not of an expression (comma is 
>>>>>> evaluated
>>>>>> as separator, not as operator).
>>>>>> While, macro parameters used as rhs and lhs expressions of the
>>>>>> assignment operator are required to be enclosed in parentheses 
>>>>>> because
>>>>>> they are part of an assignment expression.
>>>>>>
>>>>>> I verified that the violation reported by cppcheck is not due to 
>>>>>> missing
>>>>>> parentheses around the function argument (though still I have not
>>>>>> understood the origin of the warning). Also, Eclair does not 
>>>>>> report it.
>>>>>>
>>>>>> Hence, it was a misunderstanding of mine and there is no 
>>>>>> inconsistency,
>>>>>> with respect to this rule, in adding parentheses around macro 
>>>>>> parameters
>>>>>> used as rhs of assignments. The rule does not require adding 
>>>>>> parentheses
>>>>>> around macro parameters used as function arguments and neither 
>>>>>> cppcheck
>>>>>> nor Eclair report violation for missing parentheses around macro
>>>>>> parameters used as function arguments.
>>>>>
>>>>>
>>>>> Thanks a lot for the detailed explanation :-)
>>>>>
>>>>> What you say does make sense and I agree with your analysis here, only
>>>>> protect when part of an expression and not use as a subsequent 
>>>>> parameter
>>>>> (for a function or an other macro).
>>>>
>>>> Yeah I also agree with your analysis, and many thanks for
>>>> double-checking the cppcheck and Eclair's reports.
>>>
>>> Unfortunately in the specific case that I checked, it was not 
>>> reported because
>>> it was actually an argument to a macro, not a function.
>>> Eclair does report as violations of Rule 20.7 the macro parameters 
>>> that are
>>> used as function arguments and are not enclosed in parentheses.
>>>
>>> So, one tool reports it as violation and the other one not.
>>>
>>> The same goes, also, for the case where a macro parameter is used as 
>>> index to
>>> an array. Eclair reports it as violation while cppcheck does not.
>>

-- 
Xenia

Re: [PATCH 0/7] Fix MISRA C 2012 Rule 20.7 violations

[Roberto Bagnara]

Hi Xenia.  Please see below.

On 9/26/22 10:50, Xenia Ragiadakou wrote:
> On 9/18/22 16:02, Roberto Bagnara wrote:
>>> The question is on the interpretation of Rule 20.7. Are parenthesis
>>> required by Rule 20.7 in the following cases:
>>>
>>> - macro parameters used as function arguments
>>  > [...]
>>  > - macro parameter used as lhs in assignment
>>
>> You can obtain different semantics depending on whether parentheses
>> are or are not used (in the macro call and/or macro expansion
>> depending on the case):
>>
>>
>> #include <stdio.h>
>>
>> void g(int v) {
>>    printf("%d\n", v);
>> }
>>
>> #define m1(x, y, ...) g(y)
>>
>> void f1(int x, int y, ...) {
>>    g(y);
>> }
>>
>> #define p 0, 1
>>
>> void test1() {
>>    m1(p, 2);
>>    f1(p, 2);
>> }
>>
> 
> In the example above something bothers me. Let me explain.
> 
> Running the above example gives:
> 2
> 1
> 
> The results differ mainly because m1() is substituted before p.
> Thus, adding parentheses around the macro parameter 'y' of m1() i.e
> #define m1(x, y, ...) g((y))
> has no impact.
> 
> If the example is changed into the following:
> 
> #include <stdio.h>
> 
> void g(int v) {
>     printf("%d\n", v);
> }
> 
> #define m1(y, ...) g(y)
> 
> void f1(int y, ...) {
>     g(y);
> }
> 
> #define p 0, 1
> 
> void test1() {
>      m1(p, 2);
>      f1(p, 2);
> }
> 
> if no parentheses are added around 'y' in the definition of m1(), the compiler complains with "too many arguments to function g".
> If parentheses are added around 'y', the compiler does not complain but the behavior will still differ and the result will be
> 1
> 0
> 
> This happens because in the case of m1(), p is interpreted as an expression (due to the parentheses added there) and the comma is evaluated as a comma operator, while in f1(), p is interpreted as a list of expressions and the comma is evaluated as a comma separator.
> 
> Hence, in my opinion, parentheses should not be added around macro parameters used as function arguments because they can hide a bug due to missing parentheses around the entire macro definition.
> Since macro 'p' is supposed to represent an expression, and the semantics of the comma token are those of a comma operator and not a comma separator, then parentheses need to be placed around the entire macro definition i.e
> #define p (0, 1)

Your analysis is correct: the example was meant only to show that
the use of a macro or a function with the same actual parameters
and apparently equivalent bodies can make a difference and that the
addition of parentheses (around the body of p, as you suggest, or around
the occurrence of p in the call to f1()) can avoid this problem.
All this, however, is outside the scope of Rule 20.7, so the example
may have been confusing: sorry about that.

> AFAIK, there is no requirement in MISRA C guidelines to add parentheses around the entire macro definition when it is used as an expression and this is something I cannot understand.
> Unless I got it all wrong I guess ...

Yes, this is known and it is has also been brought to the attention of
the MISRA C working group.

> Can a deviation being added in the basis of C99 standard since according to the standard, E1[E2] is identical to (*((E1)+(E2))), and therefore, macro parameters used as subscript expressions are implicitly
> parenthesized and can be exempted from the rule.

Sure, you can always deviate any non-mandatory guideline: just be ware
of the fact that complying is often cheaper than deviating.

>> For instance, if you noticed Rule 20.7 reports given by
>> ECLAIR and not by cppcheck, then maybe line
>>
>> Rule 20.7              FP
>>
>> should be
>>
>> Rule 20.7              FN+FP
>>
>> If you can let me have an indication of the code that
>> ECLAIR is flagging for Rule 20.7 and cppcheck does not
>> flag, I will be happy to double-check.
> 
> ECLAIR flags as violations of Rule 20.7 the cases where unparenthesized macro parameters are used as (1) function arguments or (2) array indexes, while cppcheck does not.
> 
> For instance:
> (1) in xen/arch/arm/include/asm/atomic.h
> #define read_atomic(p) ({                                               \
>      union { typeof(*(p)) val; char c[0]; } x_;                          \
>      read_atomic_size(p, x_.c, sizeof(*(p)));                            \
>      x_.val;                                                             \
> })
> ECLAIR flags as violations missing parentheses around 'p', when used as an argument of read_atomic_size().

ECLAIR is right in reporting these violations of Rule 20.7;
these are false negatives of cppcheck.

> (2) in xen/arch/arm/arm64/cpufeature.c
> #define SANITIZE_REG(field, num, reg)  \
>      sanitize_reg(&system_cpuinfo.field.bits[num], new->field.bits[num], \
>                   #reg, ftr_##reg)
> ECLAIR flags as violations missing parentheses around 'num'.

Same as above.

I am probably repeating myself, but the MISRA guidelines are the result
of carefully-chosen compromises between the simplicity of the guideline
and its ability to protect against the targeted bad thing.  As Rule 20.7
is required, any violation will have to be deviated by projects that
have MISRA-compliance among their objectives.
Kind regards,

   Roberto

>>> On Fri, 2 Sep 2022, Xenia Ragiadakou wrote:
>>>> On 9/2/22 05:07, Stefano Stabellini wrote:
>>>>> On Thu, 1 Sep 2022, Bertrand Marquis wrote:
>>>>>> Hi Xenia,
>>>>>>
>>>>>>> On 1 Sep 2022, at 10:27, Xenia Ragiadakou <burzalodowa@gmail.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 9/1/22 01:35, Stefano Stabellini wrote:
>>>>>>>> Patches 1, 4, and 6 are already committed. I plan to commit patches 2,
>>>>>>>> 3
>>>>>>>> and 5 in the next couple of days.
>>>>>>>> Patch 7 needs further discussions and it is best addressed during the
>>>>>>>> next MISRA C sync-up.
>>>>>>>
>>>>>>> I would like to share here, before the next MISRA C sync, my
>>>>>>> understandings that will hopefully resolve a wrong impression of mine,
>>>>>>> that I may have spread around, regarding this rule.
>>>>>>> There was a misunderstanding regarding the rule 20.7 from my part and I
>>>>>>> think that Jan is absolutely right that parenthesizing macro parameters
>>>>>>> used as function arguments is not required by the rule.
>>>>>>>
>>>>>>> The rule 20.7 states "Expressions resulting from the expansion of macro
>>>>>>> parameters shall be enclosed in parentheses" and in the rationale of the
>>>>>>> rule states "If a macro parameter is not being used as an expression
>>>>>>> then the parentheses are not necessary because no operators are
>>>>>>> involved.".
>>>>>>>
>>>>>>> Initially, based on the title, my understanding was that it requires for
>>>>>>> the expression resulting from the expansion of the macro to be enclosed
>>>>>>> in parentheses. Then, based on the rule explanation and the examples
>>>>>>> given,  my understanding was that it requires the macro parameters that
>>>>>>> are used as expressions to be enclosed in parentheses.
>>>>>>> But, after re-thinking about it, the most probable and what makes more
>>>>>>> sense, is that it require parentheses around the macro parameters that
>>>>>>> are part of an expression and not around those that are used as
>>>>>>> expressions.
>>>>>>>
>>>>>>> Therefore, macro parameters being used as function arguments are not
>>>>>>> required to be enclosed in parentheses, because the function arguments
>>>>>>> are part of an expression list, not of an expression (comma is evaluated
>>>>>>> as separator, not as operator).
>>>>>>> While, macro parameters used as rhs and lhs expressions of the
>>>>>>> assignment operator are required to be enclosed in parentheses because
>>>>>>> they are part of an assignment expression.
>>>>>>>
>>>>>>> I verified that the violation reported by cppcheck is not due to missing
>>>>>>> parentheses around the function argument (though still I have not
>>>>>>> understood the origin of the warning). Also, Eclair does not report it.
>>>>>>>
>>>>>>> Hence, it was a misunderstanding of mine and there is no inconsistency,
>>>>>>> with respect to this rule, in adding parentheses around macro parameters
>>>>>>> used as rhs of assignments. The rule does not require adding parentheses
>>>>>>> around macro parameters used as function arguments and neither cppcheck
>>>>>>> nor Eclair report violation for missing parentheses around macro
>>>>>>> parameters used as function arguments.
>>>>>>
>>>>>>
>>>>>> Thanks a lot for the detailed explanation :-)
>>>>>>
>>>>>> What you say does make sense and I agree with your analysis here, only
>>>>>> protect when part of an expression and not use as a subsequent parameter
>>>>>> (for a function or an other macro).
>>>>>
>>>>> Yeah I also agree with your analysis, and many thanks for
>>>>> double-checking the cppcheck and Eclair's reports.
>>>>
>>>> Unfortunately in the specific case that I checked, it was not reported because
>>>> it was actually an argument to a macro, not a function.
>>>> Eclair does report as violations of Rule 20.7 the macro parameters that are
>>>> used as function arguments and are not enclosed in parentheses.
>>>>
>>>> So, one tool reports it as violation and the other one not.
>>>>
>>>> The same goes, also, for the case where a macro parameter is used as index to
>>>> an array. Eclair reports it as violation while cppcheck does not.
>>>
> 

Re: [PATCH 0/7] Fix MISRA C 2012 Rule 20.7 violations

[Xenia Ragiadakou]

Hi Roberto,

On 9/28/22 17:11, Roberto Bagnara wrote:
> On 9/26/22 10:50, Xenia Ragiadakou wrote:
>> On 9/18/22 16:02, Roberto Bagnara wrote:
>>>> The question is on the interpretation of Rule 20.7. Are parenthesis
>>>> required by Rule 20.7 in the following cases:
>>>>
>>>> - macro parameters used as function arguments
>>>  > [...]
>>>  > - macro parameter used as lhs in assignment
>>>
>>> You can obtain different semantics depending on whether parentheses
>>> are or are not used (in the macro call and/or macro expansion
>>> depending on the case):
>>>
>>>
>>> #include <stdio.h>
>>>
>>> void g(int v) {
>>>    printf("%d\n", v);
>>> }
>>>
>>> #define m1(x, y, ...) g(y)
>>>
>>> void f1(int x, int y, ...) {
>>>    g(y);
>>> }
>>>
>>> #define p 0, 1
>>>
>>> void test1() {
>>>    m1(p, 2);
>>>    f1(p, 2);
>>> }
>>>
>>
>> In the example above something bothers me. Let me explain.
>>
>> Running the above example gives:
>> 2
>> 1
>>
>> The results differ mainly because m1() is substituted before p.
>> Thus, adding parentheses around the macro parameter 'y' of m1() i.e
>> #define m1(x, y, ...) g((y))
>> has no impact.
>>
>> If the example is changed into the following:
>>
>> #include <stdio.h>
>>
>> void g(int v) {
>>     printf("%d\n", v);
>> }
>>
>> #define m1(y, ...) g(y)
>>
>> void f1(int y, ...) {
>>     g(y);
>> }
>>
>> #define p 0, 1
>>
>> void test1() {
>>      m1(p, 2);
>>      f1(p, 2);
>> }
>>
>> if no parentheses are added around 'y' in the definition of m1(), the 
>> compiler complains with "too many arguments to function g".
>> If parentheses are added around 'y', the compiler does not complain 
>> but the behavior will still differ and the result will be
>> 1
>> 0
>>
>> This happens because in the case of m1(), p is interpreted as an 
>> expression (due to the parentheses added there) and the comma is 
>> evaluated as a comma operator, while in f1(), p is interpreted as a 
>> list of expressions and the comma is evaluated as a comma separator.
>>
>> Hence, in my opinion, parentheses should not be added around macro 
>> parameters used as function arguments because they can hide a bug due 
>> to missing parentheses around the entire macro definition.
>> Since macro 'p' is supposed to represent an expression, and the 
>> semantics of the comma token are those of a comma operator and not a 
>> comma separator, then parentheses need to be placed around the entire 
>> macro definition i.e
>> #define p (0, 1)
> 
> Your analysis is correct: the example was meant only to show that
> the use of a macro or a function with the same actual parameters
> and apparently equivalent bodies can make a difference and that the
> addition of parentheses (around the body of p, as you suggest, or around
> the occurrence of p in the call to f1()) can avoid this problem.
> All this, however, is outside the scope of Rule 20.7, so the example
> may have been confusing: sorry about that.
> 
>> AFAIK, there is no requirement in MISRA C guidelines to add 
>> parentheses around the entire macro definition when it is used as an 
>> expression and this is something I cannot understand.
>> Unless I got it all wrong I guess ...
> 
> Yes, this is known and it is has also been brought to the attention of
> the MISRA C working group.
> 
>> Can a deviation being added in the basis of C99 standard since 
>> according to the standard, E1[E2] is identical to (*((E1)+(E2))), and 
>> therefore, macro parameters used as subscript expressions are implicitly
>> parenthesized and can be exempted from the rule.
> 
> Sure, you can always deviate any non-mandatory guideline: just be ware
> of the fact that complying is often cheaper than deviating.
> 
>>> For instance, if you noticed Rule 20.7 reports given by
>>> ECLAIR and not by cppcheck, then maybe line
>>>
>>> Rule 20.7              FP
>>>
>>> should be
>>>
>>> Rule 20.7              FN+FP
>>>
>>> If you can let me have an indication of the code that
>>> ECLAIR is flagging for Rule 20.7 and cppcheck does not
>>> flag, I will be happy to double-check.
>>
>> ECLAIR flags as violations of Rule 20.7 the cases where 
>> unparenthesized macro parameters are used as (1) function arguments or 
>> (2) array indexes, while cppcheck does not.
>>
>> For instance:
>> (1) in xen/arch/arm/include/asm/atomic.h
>> #define read_atomic(p) ({                                               \
>>      union { typeof(*(p)) val; char c[0]; } 
>> x_;                          \
>>      read_atomic_size(p, x_.c, 
>> sizeof(*(p)));                            \
>>      
>> x_.val;                                                             \
>> })
>> ECLAIR flags as violations missing parentheses around 'p', when used 
>> as an argument of read_atomic_size().
> 
> ECLAIR is right in reporting these violations of Rule 20.7;
> these are false negatives of cppcheck.
>

AFAIU, the rationale of Rule 20.7 is to ensure that the precedence of 
the expression, produced after the macro parameter expansion, will be 
higher (a parenthesized expression is a primary expression and has the 
highest precedence) than the precedence of any operator performed on 
that expression after the substitution.
These two examples refer to cases where either no operator is applied to 
the expression or the applied operator precedence is the lowest possible 
(comma operator) and for this reason the rationale of the guideline may 
be considered insufficient to justify the need of parentheses.
I guess, that ECLAIR flags the above as violations because there is no 
formal exemption in the body of the rule.
Cppcheck intentionally considers those cases compliant but unfortunately 
there is no justification in the commit messages of the respective changes.

>> (2) in xen/arch/arm/arm64/cpufeature.c
>> #define SANITIZE_REG(field, num, reg)  \
>>      sanitize_reg(&system_cpuinfo.field.bits[num], 
>> new->field.bits[num], \
>>                   #reg, ftr_##reg)
>> ECLAIR flags as violations missing parentheses around 'num'.
> 
> Same as above.
> 
> I am probably repeating myself, but the MISRA guidelines are the result
> of carefully-chosen compromises between the simplicity of the guideline
> and its ability to protect against the targeted bad thing.As Rule 20.7
> is required, any violation will have to be deviated by projects that
> have MISRA-compliance among their objectives.

There are two things that have come to my attention and may cause 
confusion around Rule 20.7. They may have also been brought to the 
attention of the MISRA C working group.

1) Rule 12.1 (Advisory), which suggests the use of parentheses to make 
operator precedence explicit, does not require the operands of a comma 
operator/separator to be parenthesized because it recognizes that 
overuse of parentheses can clutter the code.
Since both Rules 20.7 and 12.1 aim to address basically the same issue, 
why they are not aligned?

2) Rule 20.7 maps to the CERT-C Rule PRE01 which has a formal exemption 
(PRE01-C-EX1) for the above case. Maybe this sounds irrelevant but it 
struck me as odd.

I forgot in my previous email to thank you for your help that it's 
really much appreciated and needed. Thanks a lot.

> Kind regards,
> 
>    Roberto
> 
>>>> On Fri, 2 Sep 2022, Xenia Ragiadakou wrote:
>>>>> On 9/2/22 05:07, Stefano Stabellini wrote:
>>>>>> On Thu, 1 Sep 2022, Bertrand Marquis wrote:
>>>>>>> Hi Xenia,
>>>>>>>
>>>>>>>> On 1 Sep 2022, at 10:27, Xenia Ragiadakou 
>>>>>>>> <burzalodowa@gmail.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 9/1/22 01:35, Stefano Stabellini wrote:
>>>>>>>>> Patches 1, 4, and 6 are already committed. I plan to commit 
>>>>>>>>> patches 2,
>>>>>>>>> 3
>>>>>>>>> and 5 in the next couple of days.
>>>>>>>>> Patch 7 needs further discussions and it is best addressed 
>>>>>>>>> during the
>>>>>>>>> next MISRA C sync-up.
>>>>>>>>
>>>>>>>> I would like to share here, before the next MISRA C sync, my
>>>>>>>> understandings that will hopefully resolve a wrong impression of 
>>>>>>>> mine,
>>>>>>>> that I may have spread around, regarding this rule.
>>>>>>>> There was a misunderstanding regarding the rule 20.7 from my 
>>>>>>>> part and I
>>>>>>>> think that Jan is absolutely right that parenthesizing macro 
>>>>>>>> parameters
>>>>>>>> used as function arguments is not required by the rule.
>>>>>>>>
>>>>>>>> The rule 20.7 states "Expressions resulting from the expansion 
>>>>>>>> of macro
>>>>>>>> parameters shall be enclosed in parentheses" and in the 
>>>>>>>> rationale of the
>>>>>>>> rule states "If a macro parameter is not being used as an 
>>>>>>>> expression
>>>>>>>> then the parentheses are not necessary because no operators are
>>>>>>>> involved.".
>>>>>>>>
>>>>>>>> Initially, based on the title, my understanding was that it 
>>>>>>>> requires for
>>>>>>>> the expression resulting from the expansion of the macro to be 
>>>>>>>> enclosed
>>>>>>>> in parentheses. Then, based on the rule explanation and the 
>>>>>>>> examples
>>>>>>>> given,  my understanding was that it requires the macro 
>>>>>>>> parameters that
>>>>>>>> are used as expressions to be enclosed in parentheses.
>>>>>>>> But, after re-thinking about it, the most probable and what 
>>>>>>>> makes more
>>>>>>>> sense, is that it require parentheses around the macro 
>>>>>>>> parameters that
>>>>>>>> are part of an expression and not around those that are used as
>>>>>>>> expressions.
>>>>>>>>
>>>>>>>> Therefore, macro parameters being used as function arguments are 
>>>>>>>> not
>>>>>>>> required to be enclosed in parentheses, because the function 
>>>>>>>> arguments
>>>>>>>> are part of an expression list, not of an expression (comma is 
>>>>>>>> evaluated
>>>>>>>> as separator, not as operator).
>>>>>>>> While, macro parameters used as rhs and lhs expressions of the
>>>>>>>> assignment operator are required to be enclosed in parentheses 
>>>>>>>> because
>>>>>>>> they are part of an assignment expression.
>>>>>>>>
>>>>>>>> I verified that the violation reported by cppcheck is not due to 
>>>>>>>> missing
>>>>>>>> parentheses around the function argument (though still I have not
>>>>>>>> understood the origin of the warning). Also, Eclair does not 
>>>>>>>> report it.
>>>>>>>>
>>>>>>>> Hence, it was a misunderstanding of mine and there is no 
>>>>>>>> inconsistency,
>>>>>>>> with respect to this rule, in adding parentheses around macro 
>>>>>>>> parameters
>>>>>>>> used as rhs of assignments. The rule does not require adding 
>>>>>>>> parentheses
>>>>>>>> around macro parameters used as function arguments and neither 
>>>>>>>> cppcheck
>>>>>>>> nor Eclair report violation for missing parentheses around macro
>>>>>>>> parameters used as function arguments.
>>>>>>>
>>>>>>>
>>>>>>> Thanks a lot for the detailed explanation :-)
>>>>>>>
>>>>>>> What you say does make sense and I agree with your analysis here, 
>>>>>>> only
>>>>>>> protect when part of an expression and not use as a subsequent 
>>>>>>> parameter
>>>>>>> (for a function or an other macro).
>>>>>>
>>>>>> Yeah I also agree with your analysis, and many thanks for
>>>>>> double-checking the cppcheck and Eclair's reports.
>>>>>
>>>>> Unfortunately in the specific case that I checked, it was not 
>>>>> reported because
>>>>> it was actually an argument to a macro, not a function.
>>>>> Eclair does report as violations of Rule 20.7 the macro parameters 
>>>>> that are
>>>>> used as function arguments and are not enclosed in parentheses.
>>>>>
>>>>> So, one tool reports it as violation and the other one not.
>>>>>
>>>>> The same goes, also, for the case where a macro parameter is used 
>>>>> as index to
>>>>> an array. Eclair reports it as violation while cppcheck does not.
>>>>
>>

-- 
Xenia