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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 28 Jul 2022, Bertrand Marquis wrote:
> > 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).

Absolutely possible, basically we (the community) are the ones providing
the list of rules to the MISRA C checkers.


> @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)

Yes, exactly right.


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

+1