Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

Hi Julien and all,

Summarizing here on the list what I had with Julien and Bertrand this
morning.


On Wed, 15 Feb 2023, Julien Grall wrote:
> Hi Stefano,
> 
> On 14/02/2023 22:25, Stefano Stabellini wrote:
> > > Patch 1's example has a "comment" field, which no entry makes use of.
> > > Without that, how does it become clear _why_ a particular file is to
> > > be excluded? The patch description here also doesn't provide any
> > > justification ...
> > 
> > It would be good to have a couple of pre-canned justifications as the
> > reason for excluding one file could be different from the reason for
> > excluding another file. Some of the reasons:
> 
> I think the reasons should be ambiguous. This is ...
> 
> > - imported from Linux
> 
> ... the case here but...
> 
> This reason is pretty clear to me but...
> 
> > - too many false positives
> 
> ... not here. What is too many?
>
> > That said, we don't necessarily need to know the exact reason for
> > excluding one file to be able to start scanning xen with cppcheck
> > automatically. If someone wants to remove a file from the exclude list
> > in the future they just need to show that cppcheck does a good job at
> > scanning the file and we can handle the number of violations.
> 
> I disagree. A good reasoning from the start will be helpful to decide when we
> can remove a file from the list. Furthermore, we need to set good example for
> any new file we want to exclude.
> 
> Furthermore, if we exclude some files, then it will be difficult for the
> reviewers to know when they can be removed from the list. What if this is fine
> with CPPCheck but not EClair (or any other)?

Yes, the reason would help. In previous incarnations of this work, there
was a request for detailed information on external files, such as:
- where this file is coming from
- if coming from Linux, which version of Linux
- maintenance status
- coding style

But this is not what you are asking. You are only asking for a reason
and "imported from Linux" would be good enough. Please correct me if I
am wrong.

If that is the case, I think it is doable. Most of these files would end
up with "imported from Linux" as a reason. That would be fine.

 
> > I take that with this exclude list, not accounting for rule 20.7,
> > cppcheck reports zero violations overall?
> 
> So the goal right now is to have zero violations from the start? Shouldn't it
> instead be that we don't increase the number violations?
> 
> If so, we would want a baseline including the files that have "too many
> violation". The advantage is it will be easier to reduce the violations in
> small chunk and the reviewer can confirm that a patch help.

Yes, we want to be able to compared and see if a patch introduces new
violations, but that's not easy to do with cppcheck and it would help a
lot if we had a cleaner baseline with only few violations. Otherwise
there is too much noise.


To help make progress faster, I took a stab at writing an
exclude-list.json with plausible reasons that should drastically reduce
the number of violations reported by cppcheck, see below.

With the below:

- There are a total of only 18 violations on x86 (Once we ignore the
  unusedStructMember reports that look bogus. We should add
  unusedStructMember to the rules exclusion list.)

- There are a total of only 12 violations on ARM64.

- You can repro my findings, applying this series, updating
  docs/misra/exclude-list.json, and calling
  ./scripts/xen-analysis.py --cppcheck-bin=/local/repos/cppcheck/cppcheck 
--run-cppcheck -- CROSS_COMPILE="x86_64-linux-gnu-" XEN_TARGET_ARCH=x86_64



{
    "version": "1.0",
    "content": [
        {
            "rel_path": "common/bunzip2.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/libfdt/*",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/livepatch_elf.c",
            "comment" : "not in scope initially as it generates many violations 
and it is not enabled in safety configurations"
        },
        {
            "rel_path": "common/lzo.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/symbols-dummy.c",
            "comment" : "MISRA is not relevant to this file"
        },
        {
            "rel_path": "common/xz/*",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/zstd/*",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/unlz4.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/unlzma.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/unlzo.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "common/version.c",
            "comment" : "cppcheck cannot understand the tricks with linker 
symbols"
        },
        {
            "rel_path": "include/xen/lib.h",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "include/xen/sort.h",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "lib/list-sort.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "lib/rbtree.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "lib/xxhash32.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "lib/xxhash64.c",
            "comment" : "imported from Linux"
        },
        {
            "rel_path": "xsm/flask/*",
            "comment" : "not in scope initially as it generates many violations 
and it is not enabled in safety configurations"
        }
    ]
}