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

[Jan Beulich <jbeulich@xxxxxxxx>]

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