Re: [PATCH 2/2] xen/arm: asm/atomic.h: Fix MISRA C 2012 Rule 2.5 violation

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 28 Jul 2022, at 11:21, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 28/07/2022 10:45, Bertrand Marquis wrote:
>>> On 28 Jul 2022, at 10:35, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> 
>>> 
>>> On 28/07/2022 08:57, Bertrand Marquis wrote:
>>>> Hi Julien,
>>> 
>>> Hi Bertrand,
>>> 
>>>>> On 27 Jul 2022, at 16:46, Julien Grall <julien@xxxxxxx> wrote:
>>>>> 
>>>>> Hi Xenia,
>>>>> 
>>>>> On 27/07/2022 16:32, Xenia Ragiadakou wrote:
>>>>>> Remove unused macro atomic_xchg().
>>>>>> Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
>>>>>> ---
>>>>>> xen/arch/arm/include/asm/atomic.h | 2 --
>>>>>> 1 file changed, 2 deletions(-)
>>>>>> diff --git a/xen/arch/arm/include/asm/atomic.h 
>>>>>> b/xen/arch/arm/include/asm/atomic.h
>>>>>> index f5ef744b4b..a2dc125291 100644
>>>>>> --- a/xen/arch/arm/include/asm/atomic.h
>>>>>> +++ b/xen/arch/arm/include/asm/atomic.h
>>>>>> @@ -223,8 +223,6 @@ static inline int atomic_add_unless(atomic_t *v, int 
>>>>>> a, int u)
>>>>>> return __atomic_add_unless(v, a, u);
>>>>>> }
>>>>>> -#define atomic_xchg(v, new) (xchg(&((v)->counter), new))
>>>>>> -
>>>>> 
>>>>> While I agree this is unused today, the wrapper is quite trivial and part 
>>>>> of the generic API (x86 also provides one). So I am not in favor of 
>>>>> removing it just to please MISRA.
>>>>> 
>>>>> That said, if Bertrand and Stefano agrees with removing it then you 
>>>>> should also remove the x86 version to avoid inconsistency.
>>>> I think we can keep this and maybe add a comment on top to document a 
>>>> known violation:
>>>> /* TODO: MISRA_VIOLATION 2.5 */
>>> 
>>> While I am fine with this goal of the comment (i.e. indicating where Xen is 
>>> not MISRA compliant), I think this is one place where I would rather not 
>>> want one because it can get stale if someones decide to use the function.
>> I think the one doing that will have to update the comment otherwise we will 
>> never manage to have an analysis without findings.
> 
> I was under the impression that Xen will never officially follow some of the 
> MISRA rules. So I would expect the tools to be able to detect such cases so 
> we don't have to add a comment for every deviation on something we will never 
> support.
> 
>> Having those kind of comments in the code for violation also means that they 
>> must be updated if the violation is solved.
> 
> Right, but for thing like unused function, this is quite easy to miss by both 
> the developer and reviewers. So we are going to end up to comments for 
> nothing.
> 
>> Maybe we will need a run ignoring those to identify possible violations 
>> which are not violations anymore but this might be hard to do.
> 
> TBH, I think it would be best if we can teach the tools to ignore certain 
> rules.

Definitely it is possible to instruct the tool to ignore this you are right and 
for 2.5 we should (for some reason I was under the impression that we said we 
would follow 2.5 but accept deviations).

@Xenia: please ignore and do not add a comment for this.

I think we will need to distinguish 2 kind of not following:
- not following at all (disable in the tools)
- accepting some deviations (documented in the code)

As much as we can, I think we should target the second unless we have a lot of 
violations.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall