[XEN PATCH v2 0/3] address violations of MISRA C Rule 20.7

[XEN PATCH v2 0/3] address violations of MISRA C Rule 20.7

[Nicola Vetrini]

Hi all,

this series aims to refactor some macros that cause violations of MISRA C Rule
20.7 ("Expressions resulting from the expansion of macro parameters shall be
enclosed in parentheses"). All the macros touched by these patches are in some
way involved in violations, and the strategy adopted to bring them into
compliance is to add parentheses around macro arguments where needed.

Patch 1/3 is derived from that of v1; patches 2 and 3 are new to this series.

Nicola Vetrini (3):
  drivers: char: address violation of MISRA C Rule 20.7
  xen/unaligned: address violation of MISRA C Rule 20.7
  xen/pci: address violations of MISRA C Rule 20.7

 tools/include/xen-tools/common-macros.h | 2 +-
 xen/drivers/char/omap-uart.c            | 5 +++--
 xen/include/xen/pci_regs.h              | 6 +++---
 xen/include/xen/unaligned.h             | 2 +-
 xen/include/xen/vpci.h                  | 2 +-
 5 files changed, 9 insertions(+), 8 deletions(-)

-- 
2.34.1

[XEN PATCH v2 1/3] drivers: char: address violation of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional chage.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- drop excess parentheses from val parameter.
---
 xen/drivers/char/omap-uart.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
index 03b5b66e7acb..e0128225f927 100644
--- a/xen/drivers/char/omap-uart.c
+++ b/xen/drivers/char/omap-uart.c
@@ -48,8 +48,9 @@
 /* System configuration register */
 #define UART_OMAP_SYSC_DEF_CONF   0x0d   /* autoidle mode, wakeup is enabled */
 
-#define omap_read(uart, off)       readl((uart)->regs + (off<<REG_SHIFT))
-#define omap_write(uart, off, val) writel((val), (uart)->regs + (off<<REG_SHIFT))
+#define omap_read(uart, off)       readl((uart)->regs + ((off) << REG_SHIFT))
+#define omap_write(uart, off, val) writel(val, (uart)->regs + \
+                                               ((off) << REG_SHIFT))
 
 static struct omap_uart {
     u32 baud, clock_hz, data_bits, parity, stop_bits, fifo_size;
-- 
2.34.1

Re: [XEN PATCH v2 1/3] drivers: char: address violation of MISRA C Rule 20.7

[Stefano Stabellini]

On Tue, 30 Apr 2024, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional chage.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

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


> ---
> Changes in v2:
> - drop excess parentheses from val parameter.
> ---
>  xen/drivers/char/omap-uart.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
> index 03b5b66e7acb..e0128225f927 100644
> --- a/xen/drivers/char/omap-uart.c
> +++ b/xen/drivers/char/omap-uart.c
> @@ -48,8 +48,9 @@
>  /* System configuration register */
>  #define UART_OMAP_SYSC_DEF_CONF   0x0d   /* autoidle mode, wakeup is enabled */
>  
> -#define omap_read(uart, off)       readl((uart)->regs + (off<<REG_SHIFT))
> -#define omap_write(uart, off, val) writel((val), (uart)->regs + (off<<REG_SHIFT))
> +#define omap_read(uart, off)       readl((uart)->regs + ((off) << REG_SHIFT))
> +#define omap_write(uart, off, val) writel(val, (uart)->regs + \
> +                                               ((off) << REG_SHIFT))

the alignment looks off but could be fixed on commit


>  static struct omap_uart {
>      u32 baud, clock_hz, data_bits, parity, stop_bits, fifo_size;

Re: [XEN PATCH v2 1/3] drivers: char: address violation of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-05-01 21:57, Stefano Stabellini wrote:
> On Tue, 30 Apr 2024, Nicola Vetrini wrote:
>> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
>> of macro parameters shall be enclosed in parentheses". Therefore, some
>> macro definitions should gain additional parentheses to ensure that 
>> all
>> current and future users will be safe with respect to expansions that
>> can possibly alter the semantics of the passed-in macro parameter.
>> 
>> No functional chage.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
>> ---
>> Changes in v2:
>> - drop excess parentheses from val parameter.
>> ---
>>  xen/drivers/char/omap-uart.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/xen/drivers/char/omap-uart.c 
>> b/xen/drivers/char/omap-uart.c
>> index 03b5b66e7acb..e0128225f927 100644
>> --- a/xen/drivers/char/omap-uart.c
>> +++ b/xen/drivers/char/omap-uart.c
>> @@ -48,8 +48,9 @@
>>  /* System configuration register */
>>  #define UART_OMAP_SYSC_DEF_CONF   0x0d   /* autoidle mode, wakeup is 
>> enabled */
>> 
>> -#define omap_read(uart, off)       readl((uart)->regs + 
>> (off<<REG_SHIFT))
>> -#define omap_write(uart, off, val) writel((val), (uart)->regs + 
>> (off<<REG_SHIFT))
>> +#define omap_read(uart, off)       readl((uart)->regs + ((off) << 
>> REG_SHIFT))
>> +#define omap_write(uart, off, val) writel(val, (uart)->regs + \
>> +                                               ((off) << REG_SHIFT))
> 
> the alignment looks off but could be fixed on commit
> 

Can you clarify what you mean here? I aligned readl and writeln and the 
operands in writel to avoid the line being too long.

Thanks,

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

Re: [XEN PATCH v2 1/3] drivers: char: address violation of MISRA C Rule 20.7

[Jan Beulich]

On 03.05.2024 09:29, Nicola Vetrini wrote:
> On 2024-05-01 21:57, Stefano Stabellini wrote:
>> On Tue, 30 Apr 2024, Nicola Vetrini wrote:
>>> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
>>> of macro parameters shall be enclosed in parentheses". Therefore, some
>>> macro definitions should gain additional parentheses to ensure that 
>>> all
>>> current and future users will be safe with respect to expansions that
>>> can possibly alter the semantics of the passed-in macro parameter.
>>>
>>> No functional chage.
>>>
>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>>
>>> ---
>>> Changes in v2:
>>> - drop excess parentheses from val parameter.
>>> ---
>>>  xen/drivers/char/omap-uart.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/drivers/char/omap-uart.c 
>>> b/xen/drivers/char/omap-uart.c
>>> index 03b5b66e7acb..e0128225f927 100644
>>> --- a/xen/drivers/char/omap-uart.c
>>> +++ b/xen/drivers/char/omap-uart.c
>>> @@ -48,8 +48,9 @@
>>>  /* System configuration register */
>>>  #define UART_OMAP_SYSC_DEF_CONF   0x0d   /* autoidle mode, wakeup is 
>>> enabled */
>>>
>>> -#define omap_read(uart, off)       readl((uart)->regs + 
>>> (off<<REG_SHIFT))
>>> -#define omap_write(uart, off, val) writel((val), (uart)->regs + 
>>> (off<<REG_SHIFT))
>>> +#define omap_read(uart, off)       readl((uart)->regs + ((off) << 
>>> REG_SHIFT))
>>> +#define omap_write(uart, off, val) writel(val, (uart)->regs + \
>>> +                                               ((off) << REG_SHIFT))
>>
>> the alignment looks off but could be fixed on commit
>>
> 
> Can you clarify what you mean here? I aligned readl and writeln and the 
> operands in writel to avoid the line being too long.

#define omap_write(uart, off, val) writel(val, \
                                          (uart)->regs + ((off) << REG_SHIFT))

The main point being that before you start splitting an argument following
another one on the same line, you'd move that argument to a new line.

Jan

Re: [XEN PATCH v2 1/3] drivers: char: address violation of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-05-03 12:10, Jan Beulich wrote:
> On 03.05.2024 09:29, Nicola Vetrini wrote:
>> On 2024-05-01 21:57, Stefano Stabellini wrote:
>>> On Tue, 30 Apr 2024, Nicola Vetrini wrote:
>>>> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
>>>> of macro parameters shall be enclosed in parentheses". Therefore, 
>>>> some
>>>> macro definitions should gain additional parentheses to ensure that
>>>> all
>>>> current and future users will be safe with respect to expansions 
>>>> that
>>>> can possibly alter the semantics of the passed-in macro parameter.
>>>> 
>>>> No functional chage.
>>>> 
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> 
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>> 
>>> 
>>>> ---
>>>> Changes in v2:
>>>> - drop excess parentheses from val parameter.
>>>> ---
>>>>  xen/drivers/char/omap-uart.c | 5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/xen/drivers/char/omap-uart.c
>>>> b/xen/drivers/char/omap-uart.c
>>>> index 03b5b66e7acb..e0128225f927 100644
>>>> --- a/xen/drivers/char/omap-uart.c
>>>> +++ b/xen/drivers/char/omap-uart.c
>>>> @@ -48,8 +48,9 @@
>>>>  /* System configuration register */
>>>>  #define UART_OMAP_SYSC_DEF_CONF   0x0d   /* autoidle mode, wakeup 
>>>> is
>>>> enabled */
>>>> 
>>>> -#define omap_read(uart, off)       readl((uart)->regs +
>>>> (off<<REG_SHIFT))
>>>> -#define omap_write(uart, off, val) writel((val), (uart)->regs +
>>>> (off<<REG_SHIFT))
>>>> +#define omap_read(uart, off)       readl((uart)->regs + ((off) <<
>>>> REG_SHIFT))
>>>> +#define omap_write(uart, off, val) writel(val, (uart)->regs + \
>>>> +                                               ((off) << 
>>>> REG_SHIFT))
>>> 
>>> the alignment looks off but could be fixed on commit
>>> 
>> 
>> Can you clarify what you mean here? I aligned readl and writeln and 
>> the
>> operands in writel to avoid the line being too long.
> 
> #define omap_write(uart, off, val) writel(val, \
>                                           (uart)->regs + ((off) << 
> REG_SHIFT))
> 
> The main point being that before you start splitting an argument 
> following
> another one on the same line, you'd move that argument to a new line.
> 
> Jan

Oh, ok. Didn't think of that. Thanks, can amend it

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

[XEN PATCH v2 2/3] xen/unaligned: address violation of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Somewhat surprisingly, the change in the tools directory is also needed, otherwise
some CI build jobs fail (see e.g. [1]). This is not undefined behaviour
as long as the two definitions are kept in sync, following section
6.10.3p2 of the C99 standard, but having the definition in common-macros.h
is still a potential problem.

[1] https://gitlab.com/xen-project/people/bugseng/xen/-/jobs/6742878558
---
 tools/include/xen-tools/common-macros.h | 2 +-
 xen/include/xen/unaligned.h             | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h
index 07aed92684b5..60912225cb7a 100644
--- a/tools/include/xen-tools/common-macros.h
+++ b/tools/include/xen-tools/common-macros.h
@@ -102,7 +102,7 @@
 
 #define put_unaligned_t(type, val, ptr) do {                        \
     struct { type x; } __packed *ptr_ = (typeof(ptr_))(ptr);        \
-    ptr_->x = val;                                                  \
+    ptr_->x = (val);                                                \
 } while (0)
 
 #define get_unaligned(ptr)      get_unaligned_t(typeof(*(ptr)), ptr)
diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
index 3eda0ece1199..2e8238d45c54 100644
--- a/xen/include/xen/unaligned.h
+++ b/xen/include/xen/unaligned.h
@@ -19,7 +19,7 @@
 
 #define put_unaligned_t(type, val, ptr) do {				\
 	struct { type x; } __packed *ptr_ = (typeof(ptr_))(ptr);	\
-	ptr_->x = val;							\
+	ptr_->x = (val);							\
 } while (0)
 
 #define get_unaligned(ptr)	get_unaligned_t(typeof(*(ptr)), ptr)
-- 
2.34.1

Re: [XEN PATCH v2 2/3] xen/unaligned: address violation of MISRA C Rule 20.7

[Jan Beulich]

On 30.04.2024 16:28, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> Somewhat surprisingly, the change in the tools directory is also needed, otherwise
> some CI build jobs fail (see e.g. [1]). This is not undefined behaviour
> as long as the two definitions are kept in sync, following section
> 6.10.3p2 of the C99 standard, but having the definition in common-macros.h
> is still a potential problem.
> 
> [1] https://gitlab.com/xen-project/people/bugseng/xen/-/jobs/6742878558

This is pretty absurd, and the use of the Xen header in
xg_dom_decompress_unsafe_zstd.c should probably have gone away with the
introduction of the unaligned macros into ...

> --- a/tools/include/xen-tools/common-macros.h
> +++ b/tools/include/xen-tools/common-macros.h
> @@ -102,7 +102,7 @@
>  
>  #define put_unaligned_t(type, val, ptr) do {                        \
>      struct { type x; } __packed *ptr_ = (typeof(ptr_))(ptr);        \
> -    ptr_->x = val;                                                  \
> +    ptr_->x = (val);                                                \
>  } while (0)
>  
>  #define get_unaligned(ptr)      get_unaligned_t(typeof(*(ptr)), ptr)

... here. We simply cannot assume the two definitions can indefinitely
be kept in sync.

> --- a/xen/include/xen/unaligned.h
> +++ b/xen/include/xen/unaligned.h
> @@ -19,7 +19,7 @@
>  
>  #define put_unaligned_t(type, val, ptr) do {				\
>  	struct { type x; } __packed *ptr_ = (typeof(ptr_))(ptr);	\
> -	ptr_->x = val;							\
> +	ptr_->x = (val);							\

Nit: One of the padding tabs then wants dropping.

Jan

Re: [XEN PATCH v2 2/3] xen/unaligned: address violation of MISRA C Rule 20.7

[Nicola Vetrini]

On 2024-04-30 17:13, Jan Beulich wrote:
> On 30.04.2024 16:28, Nicola Vetrini wrote:
>> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
>> of macro parameters shall be enclosed in parentheses". Therefore, some
>> macro definitions should gain additional parentheses to ensure that 
>> all
>> current and future users will be safe with respect to expansions that
>> can possibly alter the semantics of the passed-in macro parameter.
>> 
>> No functional change.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>> Somewhat surprisingly, the change in the tools directory is also 
>> needed, otherwise
>> some CI build jobs fail (see e.g. [1]). This is not undefined 
>> behaviour
>> as long as the two definitions are kept in sync, following section
>> 6.10.3p2 of the C99 standard, but having the definition in 
>> common-macros.h
>> is still a potential problem.
>> 
>> [1] 
>> https://gitlab.com/xen-project/people/bugseng/xen/-/jobs/6742878558
> 
> This is pretty absurd, and the use of the Xen header in
> xg_dom_decompress_unsafe_zstd.c should probably have gone away with the
> introduction of the unaligned macros into ...
> 

I agree. I assumed it was known/deliberate.

>> --- a/tools/include/xen-tools/common-macros.h
>> +++ b/tools/include/xen-tools/common-macros.h
>> @@ -102,7 +102,7 @@
>> 
>>  #define put_unaligned_t(type, val, ptr) do {                        \
>>      struct { type x; } __packed *ptr_ = (typeof(ptr_))(ptr);        \
>> -    ptr_->x = val;                                                  \
>> +    ptr_->x = (val);                                                \
>>  } while (0)
>> 
>>  #define get_unaligned(ptr)      get_unaligned_t(typeof(*(ptr)), ptr)
> 
> ... here. We simply cannot assume the two definitions can indefinitely
> be kept in sync.
> 
>> --- a/xen/include/xen/unaligned.h
>> +++ b/xen/include/xen/unaligned.h
>> @@ -19,7 +19,7 @@
>> 
>>  #define put_unaligned_t(type, val, ptr) do {				\
>>  	struct { type x; } __packed *ptr_ = (typeof(ptr_))(ptr);	\
>> -	ptr_->x = val;							\
>> +	ptr_->x = (val);							\
> 
> Nit: One of the padding tabs then wants dropping.
> 
> Jan

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

[XEN PATCH v2 3/3] xen/pci: address violations of MISRA C Rule 20.7

[Nicola Vetrini]

MISRA C Rule 20.7 states: "Expressions resulting from the expansion
of macro parameters shall be enclosed in parentheses". Therefore, some
macro definitions should gain additional parentheses to ensure that all
current and future users will be safe with respect to expansions that
can possibly alter the semantics of the passed-in macro parameter.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
 xen/include/xen/pci_regs.h | 6 +++---
 xen/include/xen/vpci.h     | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/include/xen/pci_regs.h b/xen/include/xen/pci_regs.h
index 9909b27425a5..0bc18efabb74 100644
--- a/xen/include/xen/pci_regs.h
+++ b/xen/include/xen/pci_regs.h
@@ -445,9 +445,9 @@
 #define PCI_EXP_RTSTA		32	/* Root Status */
 
 /* Extended Capabilities (PCI-X 2.0 and Express) */
-#define PCI_EXT_CAP_ID(header)		(header & 0x0000ffff)
-#define PCI_EXT_CAP_VER(header)		((header >> 16) & 0xf)
-#define PCI_EXT_CAP_NEXT(header)	((header >> 20) & 0xffc)
+#define PCI_EXT_CAP_ID(header)		((header) & 0x0000ffff)
+#define PCI_EXT_CAP_VER(header)		(((header) >> 16) & 0xf)
+#define PCI_EXT_CAP_NEXT(header)	(((header) >> 20) & 0xffc)
 
 #define PCI_EXT_CAP_ID_ERR	1
 #define PCI_EXT_CAP_ID_VC	2
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index e89c571890b2..6e4c972f35ed 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -23,7 +23,7 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
 
 #define REGISTER_VPCI_INIT(x, p)                \
   static vpci_register_init_t *const x##_entry  \
-               __used_section(".data.vpci." p) = x
+               __used_section(".data.vpci." p) = (x)
 
 /* Assign vPCI to device by adding handlers. */
 int __must_check vpci_assign_device(struct pci_dev *pdev);
-- 
2.34.1

Re: [XEN PATCH v2 3/3] xen/pci: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 30.04.2024 16:28, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [XEN PATCH v2 3/3] xen/pci: address violations of MISRA C Rule 20.7

[Jan Beulich]

On 30.04.2024 16:28, Nicola Vetrini wrote:
> MISRA C Rule 20.7 states: "Expressions resulting from the expansion
> of macro parameters shall be enclosed in parentheses". Therefore, some
> macro definitions should gain additional parentheses to ensure that all
> current and future users will be safe with respect to expansions that
> can possibly alter the semantics of the passed-in macro parameter.
> 
> No functional change.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
>  xen/include/xen/pci_regs.h | 6 +++---
>  xen/include/xen/vpci.h     | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)

I notice you committed this with just my ack. I'm pretty sure Roger doesn't
mind, but formally his ack would have been required (hence why I didn't
commit it already before my vacation).

Jan