[PATCH 0/3] address violation of MISRA C Rule 13.6

[PATCH 0/3] address violation of MISRA C Rule 13.6

[Alessandro Zucchelli]

This series aims to address several violations of Rule 13.6 which states the
following: The operand of the `sizeof' operator shall not contain any expression
which has potential side effects.

Alessandro Zucchelli (3):
  common/kernel: address violation of MISRA C Rule 13.6
  xen/event: address violation of MISRA C Rule 13.6
  common/softirq: address violation of MISRA C Rule 13.6

 xen/common/kernel.c     | 5 +++--
 xen/common/softirq.c    | 3 ++-
 xen/include/xen/event.h | 8 +++++---
 3 files changed, 10 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/3] common/kernel: address violation of MISRA C Rule 13.6

[Alessandro Zucchelli]

In the file common/kernel.c macro ARRAY_SIZE is called with argument
current->domain->handle.
Once expanded, this ARRAY_SIZE's argument is used in sizeof operations
and thus 'current', being a macro that expands to a function
call with potential side effects, generates a violation.

To address this violation the value of current->domain is therefore
stored in a variable called 'd' before passing it to macro ARRAY_SIZE.

No functional change.

Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
---
 xen/common/kernel.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index b44b2439ca..76b4f27aef 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -660,14 +660,15 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     case XENVER_guest_handle:
     {
+        struct domain *d = current->domain;
         xen_domain_handle_t hdl;
 
         if ( deny )
             memset(&hdl, 0, ARRAY_SIZE(hdl));
 
-        BUILD_BUG_ON(ARRAY_SIZE(current->domain->handle) != ARRAY_SIZE(hdl));
+        BUILD_BUG_ON(ARRAY_SIZE(d->handle) != ARRAY_SIZE(hdl));
 
-        if ( copy_to_guest(arg, deny ? hdl : current->domain->handle,
+        if ( copy_to_guest(arg, deny ? hdl : d->handle,
                            ARRAY_SIZE(hdl) ) )
             return -EFAULT;
         return 0;
-- 
2.34.1

Re: [PATCH 1/3] common/kernel: address violation of MISRA C Rule 13.6

[Jan Beulich]

On 25.06.2024 12:14, Alessandro Zucchelli wrote:
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -660,14 +660,15 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>      case XENVER_guest_handle:
>      {
> +        struct domain *d = current->domain;

Can a (new) variable thus initialized please be consistently named currd?

>          xen_domain_handle_t hdl;
>  
>          if ( deny )
>              memset(&hdl, 0, ARRAY_SIZE(hdl));
>  
> -        BUILD_BUG_ON(ARRAY_SIZE(current->domain->handle) != ARRAY_SIZE(hdl));
> +        BUILD_BUG_ON(ARRAY_SIZE(d->handle) != ARRAY_SIZE(hdl));

Wasn't there the intention to exclude BUILD_BUG_ON() for specifically this
(and any other similar) rule?

Jan

> -        if ( copy_to_guest(arg, deny ? hdl : current->domain->handle,
> +        if ( copy_to_guest(arg, deny ? hdl : d->handle,
>                             ARRAY_SIZE(hdl) ) )
>              return -EFAULT;
>          return 0;

Re: [PATCH 1/3] common/kernel: address violation of MISRA C Rule 13.6

[Stefano Stabellini]

On Tue, 25 Jun 2024, Jan Beulich wrote:
> On 25.06.2024 12:14, Alessandro Zucchelli wrote:
> > --- a/xen/common/kernel.c
> > +++ b/xen/common/kernel.c
> > @@ -660,14 +660,15 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >  
> >      case XENVER_guest_handle:
> >      {
> > +        struct domain *d = current->domain;
> 
> Can a (new) variable thus initialized please be consistently named currd?
> 
> >          xen_domain_handle_t hdl;
> >  
> >          if ( deny )
> >              memset(&hdl, 0, ARRAY_SIZE(hdl));
> >  
> > -        BUILD_BUG_ON(ARRAY_SIZE(current->domain->handle) != ARRAY_SIZE(hdl));
> > +        BUILD_BUG_ON(ARRAY_SIZE(d->handle) != ARRAY_SIZE(hdl));
> 
> Wasn't there the intention to exclude BUILD_BUG_ON() for specifically this
> (and any other similar) rule?

+1

I think if we could do that it would be ideal because those are the
difficult cases are only meant are build checks so there is no point in
applying to MISRA to them.


> > -        if ( copy_to_guest(arg, deny ? hdl : current->domain->handle,
> > +        if ( copy_to_guest(arg, deny ? hdl : d->handle,
> >                             ARRAY_SIZE(hdl) ) )
> >              return -EFAULT;
> >          return 0;
> 

Re: [PATCH 1/3] common/kernel: address violation of MISRA C Rule 13.6

[Alessandro Zucchelli]

On 2024-06-26 03:13, Stefano Stabellini wrote:

Hi,
> On Tue, 25 Jun 2024, Jan Beulich wrote:
>> On 25.06.2024 12:14, Alessandro Zucchelli wrote:
>> > --- a/xen/common/kernel.c
>> > +++ b/xen/common/kernel.c
>> > @@ -660,14 +660,15 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> >
>> >      case XENVER_guest_handle:
>> >      {
>> > +        struct domain *d = current->domain;
>> 
>> Can a (new) variable thus initialized please be consistently named 
>> currd?
>> 
>> >          xen_domain_handle_t hdl;
>> >
>> >          if ( deny )
>> >              memset(&hdl, 0, ARRAY_SIZE(hdl));
>> >
>> > -        BUILD_BUG_ON(ARRAY_SIZE(current->domain->handle) != ARRAY_SIZE(hdl));
>> > +        BUILD_BUG_ON(ARRAY_SIZE(d->handle) != ARRAY_SIZE(hdl));
>> 
>> Wasn't there the intention to exclude BUILD_BUG_ON() for specifically 
>> this
>> (and any other similar) rule?
> 
> +1

Yes, this macro will be deviated, you may ignore this patch.

> 
> I think if we could do that it would be ideal because those are the
> difficult cases are only meant are build checks so there is no point in
> applying to MISRA to them.
> 
> 
>> > -        if ( copy_to_guest(arg, deny ? hdl : current->domain->handle,
>> > +        if ( copy_to_guest(arg, deny ? hdl : d->handle,
>> >                             ARRAY_SIZE(hdl) ) )
>> >              return -EFAULT;
>> >          return 0;
>> 

-- 
Alessandro Zucchelli, B.Sc.

Software Engineer, BUGSENG (https://bugseng.com)

[PATCH 2/3] xen/event: address violation of MISRA C Rule 13.6

[Alessandro Zucchelli]

In the file include/xen/event.h macro set_bit is called with argument
current->pause_flags.
Once expanded this set_bit's argument is used in sizeof operations
and thus 'current', being a macro that expands to a function
call with potential side effects, generates a violation.

To address this violation the value of current is therefore stored in a
variable called 'v' before passing it to macro set_bit.

No functional change.

Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
---
 xen/include/xen/event.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index f1472ea1eb..48b79f3d62 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -183,13 +183,14 @@ static bool evtchn_usable(const struct evtchn *evtchn)
 /* Wait on a Xen-attached event channel. */
 #define wait_on_xen_event_channel(port, condition)                      \
     do {                                                                \
+        struct vcpu *v = current;                                       \
         if ( condition )                                                \
             break;                                                      \
-        set_bit(_VPF_blocked_in_xen, &current->pause_flags);            \
+        set_bit(_VPF_blocked_in_xen, &v->pause_flags);                  \
         smp_mb(); /* set blocked status /then/ re-evaluate condition */ \
         if ( condition )                                                \
         {                                                               \
-            clear_bit(_VPF_blocked_in_xen, &current->pause_flags);      \
+            clear_bit(_VPF_blocked_in_xen, &v->pause_flags);            \
             break;                                                      \
         }                                                               \
         raise_softirq(SCHEDULE_SOFTIRQ);                                \
@@ -198,7 +199,8 @@ static bool evtchn_usable(const struct evtchn *evtchn)
 
 #define prepare_wait_on_xen_event_channel(port)                         \
     do {                                                                \
-        set_bit(_VPF_blocked_in_xen, &current->pause_flags);            \
+        struct vcpu *v = current;                                       \
+        set_bit(_VPF_blocked_in_xen, &v->pause_flags);                  \
         raise_softirq(SCHEDULE_SOFTIRQ);                                \
         smp_mb(); /* set blocked status /then/ caller does his work */  \
     } while ( 0 )
-- 
2.34.1

Re: [PATCH 2/3] xen/event: address violation of MISRA C Rule 13.6

[Stefano Stabellini]

On Tue, 25 Jun 2024, Alessandro Zucchelli wrote:
> In the file include/xen/event.h macro set_bit is called with argument
> current->pause_flags.
> Once expanded this set_bit's argument is used in sizeof operations
> and thus 'current', being a macro that expands to a function
> call with potential side effects, generates a violation.
> 
> To address this violation the value of current is therefore stored in a
> variable called 'v' before passing it to macro set_bit.
> 
> No functional change.
> 
> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

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

Re: [PATCH 2/3] xen/event: address violation of MISRA C Rule 13.6

[Jan Beulich]

On 25.06.2024 12:14, Alessandro Zucchelli wrote:
> --- a/xen/include/xen/event.h
> +++ b/xen/include/xen/event.h
> @@ -183,13 +183,14 @@ static bool evtchn_usable(const struct evtchn *evtchn)
>  /* Wait on a Xen-attached event channel. */
>  #define wait_on_xen_event_channel(port, condition)                      \
>      do {                                                                \
> +        struct vcpu *v = current;                                       \

First: As recently indicated elsewhere, any new latching of "current" into
a local var should use "curr" or something derived from it (see below), not
"v".

Second: Macro local variables are at (certain) risk of colliding with outer
scope variables. Therefore the common thing we do is add an underscore.
Disagreement continues to exist for whether to prefix them or have them be
suffixes. An affirmative statement as to Misra's take on underscore-prefixed
variables in situations like these would be appreciated. Sadly what the C
standard has is somewhat tied to the C library, and hence leaves room for
interpretation (and hence for disagreement).

Jan

Re: [PATCH 2/3] xen/event: address violation of MISRA C Rule 13.6

[Jan Beulich]

On 01.07.2024 10:16, Jan Beulich wrote:
> On 25.06.2024 12:14, Alessandro Zucchelli wrote:
>> --- a/xen/include/xen/event.h
>> +++ b/xen/include/xen/event.h
>> @@ -183,13 +183,14 @@ static bool evtchn_usable(const struct evtchn *evtchn)
>>  /* Wait on a Xen-attached event channel. */
>>  #define wait_on_xen_event_channel(port, condition)                      \
>>      do {                                                                \
>> +        struct vcpu *v = current;                                       \
> 
> First: As recently indicated elsewhere, any new latching of "current" into
> a local var should use "curr" or something derived from it (see below), not
> "v".
> 
> Second: Macro local variables are at (certain) risk of colliding with outer
> scope variables. Therefore the common thing we do is add an underscore.
> Disagreement continues to exist for whether to prefix them or have them be
> suffixes. An affirmative statement as to Misra's take on underscore-prefixed
> variables in situations like these would be appreciated. Sadly what the C
> standard has is somewhat tied to the C library, and hence leaves room for
> interpretation (and hence for disagreement).

Why was this patch committed unchanged, considering the comments above?

Jan

Re: [PATCH 2/3] xen/event: address violation of MISRA C Rule 13.6

[Stefano Stabellini]

On Wed, 31 Jul 2024, Jan Beulich wrote:
> On 01.07.2024 10:16, Jan Beulich wrote:
> > On 25.06.2024 12:14, Alessandro Zucchelli wrote:
> >> --- a/xen/include/xen/event.h
> >> +++ b/xen/include/xen/event.h
> >> @@ -183,13 +183,14 @@ static bool evtchn_usable(const struct evtchn *evtchn)
> >>  /* Wait on a Xen-attached event channel. */
> >>  #define wait_on_xen_event_channel(port, condition)                      \
> >>      do {                                                                \
> >> +        struct vcpu *v = current;                                       \
> > 
> > First: As recently indicated elsewhere, any new latching of "current" into
> > a local var should use "curr" or something derived from it (see below), not
> > "v".
> > 
> > Second: Macro local variables are at (certain) risk of colliding with outer
> > scope variables. Therefore the common thing we do is add an underscore.
> > Disagreement continues to exist for whether to prefix them or have them be
> > suffixes. An affirmative statement as to Misra's take on underscore-prefixed
> > variables in situations like these would be appreciated. Sadly what the C
> > standard has is somewhat tied to the C library, and hence leaves room for
> > interpretation (and hence for disagreement).
> 
> Why was this patch committed unchanged, considering the comments above?

Sorry, Jan. I didn't do it on purpose. Due to the code freeze, I added
this patch to my for-4.19 queue after acking it. Later, when you
provided your review comments, I forgot to remove the patch from my
for-4.19 queue.

If you want it can be reverted but it is easier if you send a small
incremental patch with your suggestion and I'll ack it straight away.

[PATCH 3/3] common/softirq: address violation of MISRA C Rule 13.6

[Alessandro Zucchelli]

In the file common/softirq macro set_bit is called with argument
smp_processor_id.
Once expanded this set_bit's argument is used in sizeof operations
and thus 'smp_processor_id', being a macro that expands to a
function call with potential side effects, generates a violation.

To address this violation the value of smp_processor_id is therefore
stored in a variable called 'cpu' before passing it to macro set_bit.

No functional change.

Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
---
 xen/common/softirq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/common/softirq.c b/xen/common/softirq.c
index bee4a82009..c5f3870534 100644
--- a/xen/common/softirq.c
+++ b/xen/common/softirq.c
@@ -139,7 +139,8 @@ void cpu_raise_softirq_batch_finish(void)
 
 void raise_softirq(unsigned int nr)
 {
-    set_bit(nr, &softirq_pending(smp_processor_id()));
+    unsigned int cpu = smp_processor_id();
+    set_bit(nr, &softirq_pending(cpu));
 }
 
 /*
-- 
2.34.1

Re: [PATCH 3/3] common/softirq: address violation of MISRA C Rule 13.6

[Stefano Stabellini]

On Tue, 25 Jun 2024, Alessandro Zucchelli wrote:
> In the file common/softirq macro set_bit is called with argument
> smp_processor_id.
> Once expanded this set_bit's argument is used in sizeof operations
> and thus 'smp_processor_id', being a macro that expands to a
> function call with potential side effects, generates a violation.
> 
> To address this violation the value of smp_processor_id is therefore
> stored in a variable called 'cpu' before passing it to macro set_bit.
> 
> No functional change.
> 
> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

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

Re: [PATCH 3/3] common/softirq: address violation of MISRA C Rule 13.6

[Jan Beulich]

On 25.06.2024 12:14, Alessandro Zucchelli wrote:
> In the file common/softirq macro set_bit is called with argument
> smp_processor_id.
> Once expanded this set_bit's argument is used in sizeof operations
> and thus 'smp_processor_id', being a macro that expands to a
> function call with potential side effects, generates a violation.

Noticing only now, but applicable also to patch 2: "expands" isn't quite
right, is it? That's true for x86, but apparently not for Arm. Unless I
managed to overlook something there. So perhaps "may expand" instead?

> --- a/xen/common/softirq.c
> +++ b/xen/common/softirq.c
> @@ -139,7 +139,8 @@ void cpu_raise_softirq_batch_finish(void)
>  
>  void raise_softirq(unsigned int nr)
>  {
> -    set_bit(nr, &softirq_pending(smp_processor_id()));
> +    unsigned int cpu = smp_processor_id();
> +    set_bit(nr, &softirq_pending(cpu));
>  }

Nit (style): Blank line between declaration(s) and statement(s) please.

I guess both aspects could be taken care of while committing.

Jan