Re: [RFC PATCH 01/18] arm: cppcheck: misra rule 20.7 deviations for alternative.h

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 20 Dec 2022, Julien Grall wrote:
> Hi Luca,
> 
> On 20/12/2022 08:50, Luca Fancellu wrote:
> > Cppcheck reports violations of rule 20.7 in two macros of
> > alternative.h.
> > 
> > The first one is in the __ALT_PTR macro where cppcheck wants to have
> > parentheses on the second operand of a member access operator, which
> > is not allowed from the c language syntax, furthermore, the macro
> > parameter is never used as an expression, hence we can suppress the
> > finding.
> 
> Looking at the Misra Rule 20.7 examples [1], this looks similar to
> GET_MEMBER(). So I don't understand why cppcheck is complaining.
> 
> > 
> > The second finding is in the __ALTERNATIVE_CFG macro, where cppcheck
> > wants to have parentheses on the macro arguments, but the macro
> > parameters are never used as expressions and are used only for text
> > substitution, so cppcheck is not taking into account the context of
> > use of the macro arguments and adding parenthesis would break the
> > code, so we can suppress the finding.
> 
> This reads like you want to report a bug against cppcheck.

+1

 
> > No error was shown by eclair and coverity for those lines.
> > 
> > Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> > ---
> >   docs/misra/false-positive-cppcheck.json | 14 ++++++++++++++
> >   xen/arch/arm/include/asm/alternative.h  |  2 ++
> 
> This file is meant to be close to Linux. From my understanding of the
> discussion yesterday, we didn't want to add deviation there.

Yeah. We are still finalizing the list of Linux files in Xen
(https://marc.info/?l=xen-devel&m=166859007703945) so we don't have a
programmatic way to check if something should be scanned or not. At the
moment it is easy to make mistakes. I hope will get it committed soon,
then we can blacklist anything listed under
docs/misra/external-files.txt (automatically ask cppcheck to avoid
scanning files listed in docs/misra/external-files.txt).


> >   2 files changed, 16 insertions(+)
> > 
> > diff --git a/docs/misra/false-positive-cppcheck.json
> > b/docs/misra/false-positive-cppcheck.json
> > index 5d4da2ce6170..5e7d9377f60b 100644
> > --- a/docs/misra/false-positive-cppcheck.json
> > +++ b/docs/misra/false-positive-cppcheck.json
> > @@ -3,6 +3,20 @@
> >       "content": [
> >           {
> >               "id": "SAF-0-false-positive-cppcheck",
> > +            "violation-id": "misra-c2012-20.7",
> > +            "tool-version": "2.7",
> > +            "name": "R20.7 second operand of member-access operator",
> > +            "text": "The second operand of a member access operator shall
> > be a name of a member of the type pointed to, so in this particular case it
> > is wrong to use parentheses on the macro parameter."
> > +        },
> > +        {
> > +            "id": "SAF-1-false-positive-cppcheck",
> > +            "violation-id": "misra-c2012-20.7",
> > +            "tool-version": "2.7",
> > +            "name": "R20.7 C macro parameters used only for text
> > substitution",
> > +            "text": "The macro parameters used in this case are not part of
> > an expression, they are used for text substitution."
> > +        },
> In both cases the constructs are commonly used in Xen to generate code. So I
> am a bit concerned to have to add deviation everywhere cppcheck is wrong.
> 
> More generally, we are still at the beginning of MISRA in Xen and I don't
> think we should start with adding deviations from bugs in the tools. Instead,
> we should report those bugs and hopefully by the time Xen is mostly MISRA
> complaint the tools will not report the false positive.
> 
> If they are still then we can decide to add deviations.

I also think we should report cppcheck bugs.

That said, I think the reason why Luca submitted this series is that we
are actually not far from having cppcheck running automatically as part
of gitlab-ci to scan staging and new patches.

Enabling automatic cppcheck scans is something within our reach, I'd say
it is only few weeks away. That's actually going to start giving some
concrete benefits for our work on MISRA C. We'll get automatic bug
reports and quality reports, which is pretty cool.

Aside from the list of Linux files, we also need to deal with false
positives otherwise the reports will be too "noisy". If we correctly
silence the outstanding false-positive cppcheck reports, we can actually
have a clean "zero issues" report by cppcheck on staging with the
current list of MISRA C rules supported by docs/misra/rules.rst.

And there are only few cppcheck false positives to deal with. Most of
them are for Rule 20.7. During the last FuSa call, I actually suggested
to skip Rule 20.7 when it comes to scanning for issues (i.e. blacklist
Rule 20.7).

Luca suggested that before doing that we could at least try to
fix/deviate the violations reported by cppcheck (they are far fewer than
the ones reported by Eclair). So here we are :-)



So in short:

- This is not the first of many series flooding xen-devel adding
  deviations; this is the only series we need to get zero
  false-positives from cppcheck now

- With this series, and with the linux files to skip, we could add
  cppcheck to gitlab-ci today

- Alternatively, we could disable rule 20.7 for now

- We could wait for cppcheck to improve and address our bug reports, but
  it is likely not going to enable us to make progress quickly. If we
  want to wait, I would rather disable 20.7 until cppcheck improves so
  that we can still add cppcheck to gitlab-ci soon. I think that would
  be OK too


What do you guys think? Nice automatic 20.7 scans for all patches and
for staging, but with the undesirable false-positive comments, or
no-20.7 scans but no comments in the tree?