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 13:19, Jan Beulich wrote:
> > On 19.10.2023 13:12, Simone Ballarin wrote:
> > > On 19/10/23 11:35, Jan Beulich wrote:
> > > > On 19.10.2023 03:06, Stefano Stabellini wrote:
> > > > > On Wed, 18 Oct 2023, Simone Ballarin wrote:
> > > > > > --- 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.
> > > > 
> > > > Not exactly. It's a per-CPU variable access on Arm, but involves a
> > > > function call on x86. Still that function call has no other side
> > > > effects, but I guess Misra wouldn't honor this.
> > > > 
> > > > I'm afraid though that the suggested change violates rule 2.2, as
> > > > the two new assignments are dead code when !CONFIG_TRACEBUFFER.
> > > 
> > > I confirm that there is no problem in X86: I will simply propose a patch
> > > adding __attribute_pure__ to get_cpu_info.
> > 
> > I specifically did not suggest that, because I'm afraid the "pure" attribute
> > may not be used there. Besides this attribute typically being discarded for
> > inline functions in favor of the compiler's own judgement, it would allow
> > the compiler to squash multiple invocations. This might even be desirable in
> > most cases, but would break across a stack pointer change. (In this context
> > you also need to keep in mind late optimizations done by LTO.)
> > 
> > Jan
> > 
> 
> Ok, in this case I will just configure ECLAIR to consider it without effects.

I think that's fine, just remember to either use SAF or document under
docs/misra/deviations.rst