Re: [XEN PATCH 4/4] xen: address violations of MISRA C:2012 Rule 13.1

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 19 Oct 2023, Simone Ballarin wrote:
> On 19/10/23 03:06, Stefano Stabellini wrote:
> > On Wed, 18 Oct 2023, Simone Ballarin wrote:
> > > Rule 13.1: Initializer lists shall not contain persistent side effects
> > > 
> > > This patch moves expressions with side-effects outside the initializer
> > > lists.
> > > 
> > > No functional changes.
> > > 
> > > Signed-off-by: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
> > > ---
> > >   xen/common/sched/core.c    | 16 ++++++++++++----
> > >   xen/drivers/char/ns16550.c |  4 +++-
> > >   2 files changed, 15 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> > > index 12deefa745..519884f989 100644
> > > --- a/xen/common/sched/core.c
> > > +++ b/xen/common/sched/core.c
> > > @@ -1504,6 +1504,8 @@ long vcpu_yield(void)
> > >   {
> > >       struct vcpu * v=current;
> > >       spinlock_t *lock;
> > > +    domid_t domain_id;
> > > +    int vcpu_id;
> > >         rcu_read_lock(&sched_res_rculock);
> > >   @@ -1515,7 +1517,9 @@ long vcpu_yield(void)
> > >         SCHED_STAT_CRANK(vcpu_yield);
> > >   -    TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id,
> > > current->vcpu_id);
> > > +    domain_id = current->domain->domain_id;
> > > +    vcpu_id = current->vcpu_id;
> > > +    TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id);
> > 
> > Also here it looks like accessing the current pointer is considered a
> > side effect. Why? This is a just a global variable that is only accessed
> > for reading.
> > 
> > 
> > >       raise_softirq(SCHEDULE_SOFTIRQ);
> > >       return 0;
> > >   }
> > > @@ -1888,14 +1892,17 @@ ret_t do_sched_op(int cmd,
> > > XEN_GUEST_HANDLE_PARAM(void) arg)
> > >       case SCHEDOP_shutdown:
> > >       {
> > >           struct sched_shutdown sched_shutdown;
> > > +        domid_t domain_id;
> > > +        int vcpu_id;
> > >             ret = -EFAULT;
> > >           if ( copy_from_guest(&sched_shutdown, arg, 1) )
> > >               break;
> > >   +        domain_id = current->domain->domain_id;
> > > +        vcpu_id = current->vcpu_id;
> > >           TRACE_3D(TRC_SCHED_SHUTDOWN,
> > > -                 current->domain->domain_id, current->vcpu_id,
> > > -                 sched_shutdown.reason);
> > > +                 domain_id, vcpu_id, sched_shutdown.reason);
> > >           ret = domain_shutdown(current->domain,
> > > (u8)sched_shutdown.reason);
> > >             break;
> > > @@ -1905,13 +1912,14 @@ ret_t do_sched_op(int cmd,
> > > XEN_GUEST_HANDLE_PARAM(void) arg)
> > >       {
> > >           struct sched_shutdown sched_shutdown;
> > >           struct domain *d = current->domain;
> > > +        int vcpu_id = current->vcpu_id;
> > >             ret = -EFAULT;
> > >           if ( copy_from_guest(&sched_shutdown, arg, 1) )
> > >               break;
> > >             TRACE_3D(TRC_SCHED_SHUTDOWN_CODE,
> > > -                 d->domain_id, current->vcpu_id, sched_shutdown.reason);
> > > +                 d->domain_id, vcpu_id, sched_shutdown.reason);
> > >             spin_lock(&d->shutdown_lock);
> > >           if ( d->shutdown_code == SHUTDOWN_CODE_INVALID )
> > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> > > index 28ddedd50d..0b3d8b2a30 100644
> > > --- a/xen/drivers/char/ns16550.c
> > > +++ b/xen/drivers/char/ns16550.c
> > > @@ -445,10 +445,12 @@ static void __init cf_check
> > > ns16550_init_postirq(struct serial_port *port)
> > >               struct msi_info msi = {
> > >                   .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
> > >                                    uart->ps_bdf[2]),
> > > -                .irq = rc = uart->irq,
> > > +                .irq = uart->irq,
> > >                   .entry_nr = 1
> > >               };
> > >   +            rc = uart->irq;
> > 
> > What's the side effect here? If the side effect is rc = uart->irq, why
> > is it considered a side effect?
> > 
> 
> Yes, rc = uart->irq is the side-effect: it writes a variable
> declared outside the context of the expression.
> 
> Consider the following example:
> 
> int rc;
> 
> struct S s = {
>   .x = rc = 2,
>   .y = rc = 3
> };
> 
> What's the value of rc?

OK thanks for the explanation.