[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
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" } ] }
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |