Re: [PATCH] Add more rules to docs/misra/rules.rst

[Stefano Stabellini <stefano.stabellini@xxxxxxx>]

On Thu, 26 Jan 2023, Jan Beulich wrote:
> On 26.01.2023 16:59, Stefano Stabellini wrote:
> > On Thu, 26 Jan 2023, Jan Beulich wrote:
> >> On 25.01.2023 21:57, Stefano Stabellini wrote:
> >>> From: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> >>>
> >>> As agreed during the last MISRA C discussion, I am adding the following
> >>> MISRA C rules: 7.1, 7.3, 18.3.
> >>>
> >>> I am also adding 13.1 and 18.2 that were "agreed pending an analysis on
> >>> the amount of violations".
> >>>
> >>> In the case of 13.1 there are zero violations reported by cppcheck.
> >>>
> >>> In the case of 18.2, there are zero violations reported by cppcheck
> >>> after deviating the linker symbols, as discussed.
> >>
> >> I find this suspicious.
> > 
> > Hi Jan, you are right to be suspicious about 18.2 :-)  cppcheck is
> > clearly not doing a great job at finding violations. Here is the full
> > picture:
> > 
> > - cppcheck finds 3 violations, obviously related to linker symbols
> > specifically common/version.c:xen_build_init and
> > xen/lib/ctors.c:init_constructors
> > 
> > - Coverity finds 9 violations, not sure which ones
> > 
> > - Eclair finds 56 total on x86. Eclair is always the strictest of the
> >   three tools and is flagging:
> >   - the usage of the guest_mode macro in x86/traps.c and other places
> >   - the usage of the NEED_OP/NEED_IP macros in common/lzo.c
> >   the remaining violations should be less than 10
> > 
> > 
> >> See e.g. ((pg) - frame_table) expressions both Arm
> >> and x86 have. frame_table is neither a linker generated symbol, nor does
> >> it represent something that the compiler (or static analysis tools) would
> >> recognized as an "object". Still, the entire frame table of course
> >> effectively is an object (array), yet there's no way for any tool to
> >> actually recognize the array dimension.
> > 
> > I used cppcheck in my original email because it is the only tool today
> > where I can add a deviation as an in-code comment, re-run the scan,
> > and what happens (see the number of violations go down.)
> > 
> > However also considering that Coverity reports less than 10, and Eclair
> > reports more but due to only 2-3 macros, I think 18.2 should be
> > manageable.
> 
> That's not the conclusion I would draw. If none of the three finds what
> ought to be found, I'm not convinced this can be considered "manageable".
> Subsequent tool improvements may change the picture quite unexpectedly.

Keep in mind that there is always the possibility that a new version of
the tool will detect more violations. In that case, we'll have the
choice whether:
- to fix/deviate them
- not to upgrade the tool
- to skip checking this specific rule with the specific tool in question


So let me make a concrete example based on cppcheck, given that is the
one we understand better from an integration point of view. Let's say
that tomorrow we introduce a gitlab-ci job that automatically scans
xen.git using cppcheck and docs/misra/rules.rst. The job fails when it
detects *new* violations. All is good for now.

Then one day we upgrade cppcheck to a new version. The new cppcheck
version detects way more violations. We have a few options:
- hold back the upgrade in gitlab-ci until we fix more violations or
  deviate them
- keep the rule in docs/misra/rules.rst but skip checking for the rule
  in gitlab-ci using the mechanism introduced by Luca, and upgrade
  cppcheck
- remove the rule from docs/misra/rules.rst, and upgrade cppcheck


We have ways to deal with this situation well. I think the decision
whether to add a rule to docs/misra/rules.rst should be primarily based
on whether the rule makes sense for Xen. And of course it also makes
sense to have a look at the number of violations because having a rule
in docs/misra/rules.rst that no tool can scan properly, or with
thousands of violations, is not useful.

Coming back to 18.2: it makes sense for Xen and the scanners today work
well with this rule, so I think we are fine.

(In retrospect we should have thought twice before adding 20.7 given all
the troubles that the rule is giving us with the scanners. We are
learning. There is still the option of removing 20.7, we can discuss in
the next meeting.)