Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
On Fri, 24 Feb 2023, Luca Fancellu wrote:
> Hi Stefano,
>
> >> Hi Jan,
> >>
> >> my personal opinion is that we can’t handle them for files that needs to
> >> be kept
> >> in sync from an external source, because we can’t justify findings or tag
> >> false
> >> positives, if we are lucky we might fix the non compliances but even in
> >> that case
> >> there might be times when the fix goes through and cases where the fix is
> >> objected
> >> by the maintainers just because their project is not following the MISRA
> >> standard.
> >>
> >> On our side, what we can do is track these files and from time to time
> >> check that
> >> they are still eligible to backport, because when they are not anymore we
> >> could
> >> just port them to Xen coding style and start doing direct changes.
> >>
> >>
> >> @Stefano/@Bertrand/@Julien thanks for your effort on the list, yesterday
> >> morning
> >> I’ve also had a look on the Michal’s file list and I’ve tracked down the
> >> origin, here
> >> my findings, maybe we could merge your list with mine?
> >
> > Yes to merge the two lists, but I think we should keep only items that we
> > actually need for the sake of having a cleaner baseline. So I would only
> > add things we need today to reduce the noise on cppcheck results
> > (excluding 20.7 and also excluding unusedStructMember which I think we
> > should probably stop scanning with cppcheck altogether).
>
> Yes I thought about excluding unusedStructMember, I see there are a lot of
> findings on x86
> which are not real findings, it’s just that the tool has not the complete
> picture.
>
> Here the ways are two:
> 1) exclude globally unusedStructMember
> 2) exclude unusedStructMember only on some selected files (available only on
> cppcheck)
> this requires some work to add a field to this list, like
> “cppcheck-error-exclude” and a list,
> comma separated, of error-id to be suppressed from the corresponding file.
For now, I would exclude globally
> Regarding the list, I merged your list with mine (and Michal’s work) to
> create a complete list,
> I think it’s better to have it complete because all those files are external
> and even if today we don’t
> have findings for some of these files, we could have some in the future, and
> since we know today
> that we can’t do direct changes to them, in my opinion it’s better to list
> them now instead of forgetting
> them later.
>
> I left out for now these files to start a discussion for them, because I
> think they should be included in
> the analysis:
>
> common/symbols-dummy.c:
> this file accepts direct changes, cppcheck complains about misra-c2012-8.4
> but I think it is right, also
> Coverity complains about the same findings, they can be fixed adding
> declarations on symbols.h I think
> and removing the declarations from symbol.c module
>
> common/version.c:
> Apart from unusedStructMember, cppcheck is confused only on 2 findings that
> compares one local symbol
> and one linker defined symbol, could we have just one tag to suppress those
> two findings instead of removing
> completely the file? This file is under our control, so we could push
> changes.
>
> include/xen/lib.h:
> Findings are from the bsearch function, which is derived from Linux, but I
> can see the codestyle is the Xen style
> and it seems (I might be wrong) that it has diverged from Linux, so we
> might do changes and fix the findings that
> are correct, void* arithmetic is working because gcc make it work assigning
> a sizeof of 1, using char* pointers we
> have the same result without having the undefined behaviour (correct me if
> I am wrong).
>
> include/xen/sort.h:
> Also this one is derived from Linux, but seems that we converted it to our
> coding style and we can do direct changes,
> the same reasoning about void* pointers arithmetic applies here.
>
>
> What are your thoughts?
I am good with this
> Here the merged list, capturing also Jan comments:
>
> {
>"version": "1.0",
>"content": [
>{
>"rel_path": "arch/arm/arm64/cpufeature.c",
>"comment": "Imported from Linux, ignore for now"
>},
>{
>"rel_path": "arch/arm/arm64/insn.c",
>"comment": "Imported on Linux"
>},
>{
>"rel_path": "arch/arm/arm64/lib/find_next_bit.c",
>"comment": "Imported from Linux, ignore for now"
>},
>{
>"rel_path": "arch/x86/acpi/boot.c",
>"comment": "Imported from Linux, ignore for now"
>},
>{
>"rel_path": "arch/x86/acpi/cpu_idle.c",
>"comment": "Imported from Linux, ignore for now"
>},
>{
>"rel_path": "arch/x86/acpi/cpufreq/cpufreq.c",
>"comment": "Imported from Linux, ignore for now"
>},
>{
>"rel_path": "arch/x86/acpi/cpuidle_menu.c",
>"comment": "Imported from
Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
Hi Stefano,
>> Hi Jan,
>>
>> my personal opinion is that we can’t handle them for files that needs to be
>> kept
>> in sync from an external source, because we can’t justify findings or tag
>> false
>> positives, if we are lucky we might fix the non compliances but even in that
>> case
>> there might be times when the fix goes through and cases where the fix is
>> objected
>> by the maintainers just because their project is not following the MISRA
>> standard.
>>
>> On our side, what we can do is track these files and from time to time check
>> that
>> they are still eligible to backport, because when they are not anymore we
>> could
>> just port them to Xen coding style and start doing direct changes.
>>
>>
>> @Stefano/@Bertrand/@Julien thanks for your effort on the list, yesterday
>> morning
>> I’ve also had a look on the Michal’s file list and I’ve tracked down the
>> origin, here
>> my findings, maybe we could merge your list with mine?
>
> Yes to merge the two lists, but I think we should keep only items that we
> actually need for the sake of having a cleaner baseline. So I would only
> add things we need today to reduce the noise on cppcheck results
> (excluding 20.7 and also excluding unusedStructMember which I think we
> should probably stop scanning with cppcheck altogether).
Yes I thought about excluding unusedStructMember, I see there are a lot of
findings on x86
which are not real findings, it’s just that the tool has not the complete
picture.
Here the ways are two:
1) exclude globally unusedStructMember
2) exclude unusedStructMember only on some selected files (available only on
cppcheck)
this requires some work to add a field to this list, like
“cppcheck-error-exclude” and a list,
comma separated, of error-id to be suppressed from the corresponding file.
Regarding the list, I merged your list with mine (and Michal’s work) to create
a complete list,
I think it’s better to have it complete because all those files are external
and even if today we don’t
have findings for some of these files, we could have some in the future, and
since we know today
that we can’t do direct changes to them, in my opinion it’s better to list them
now instead of forgetting
them later.
I left out for now these files to start a discussion for them, because I think
they should be included in
the analysis:
common/symbols-dummy.c:
this file accepts direct changes, cppcheck complains about misra-c2012-8.4
but I think it is right, also
Coverity complains about the same findings, they can be fixed adding
declarations on symbols.h I think
and removing the declarations from symbol.c module
common/version.c:
Apart from unusedStructMember, cppcheck is confused only on 2 findings that
compares one local symbol
and one linker defined symbol, could we have just one tag to suppress those
two findings instead of removing
completely the file? This file is under our control, so we could push changes.
include/xen/lib.h:
Findings are from the bsearch function, which is derived from Linux, but I
can see the codestyle is the Xen style
and it seems (I might be wrong) that it has diverged from Linux, so we might
do changes and fix the findings that
are correct, void* arithmetic is working because gcc make it work assigning a
sizeof of 1, using char* pointers we
have the same result without having the undefined behaviour (correct me if I
am wrong).
include/xen/sort.h:
Also this one is derived from Linux, but seems that we converted it to our
coding style and we can do direct changes,
the same reasoning about void* pointers arithmetic applies here.
What are your thoughts?
Here the merged list, capturing also Jan comments:
{
"version": "1.0",
"content": [
{
"rel_path": "arch/arm/arm64/cpufeature.c",
"comment": "Imported from Linux, ignore for now"
},
{
"rel_path": "arch/arm/arm64/insn.c",
"comment": "Imported on Linux"
},
{
"rel_path": "arch/arm/arm64/lib/find_next_bit.c",
"comment": "Imported from Linux, ignore for now"
},
{
"rel_path": "arch/x86/acpi/boot.c",
"comment": "Imported from Linux, ignore for now"
},
{
"rel_path": "arch/x86/acpi/cpu_idle.c",
"comment": "Imported from Linux, ignore for now"
},
{
"rel_path": "arch/x86/acpi/cpufreq/cpufreq.c",
"comment": "Imported from Linux, ignore for now"
},
{
"rel_path": "arch/x86/acpi/cpuidle_menu.c",
"comment": "Imported from Linux, ignore for now"
},
{
"rel_path": "arch/x86/acpi/lib.c",
"comment": "Imported from Linux, ignore for now"
},
{
"rel_path": "arch/x86/cpu/amd.c",
"comment": "Imported from Linux, ignore for now"
},
{
"rel_path": "arch/x86/cpu/centaur.c",
Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
On 17.02.2023 02:44, Stefano Stabellini wrote: > On Thu, 16 Feb 2023, Luca Fancellu wrote: >>> On 16 Feb 2023, at 08:19, Jan Beulich wrote: >>> On 16.02.2023 00:49, Stefano Stabellini wrote: On Wed, 15 Feb 2023, Julien Grall wrote: > 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. >>> >>> I guess you mean s/would/could/. Personally I find "imported from Linux" >>> as an entirely unacceptable justification: Why would the origin of a file >>> matter on whether it has violations? Dealing with the violations may be >>> more cumbersome (because preferably the adjustments would go to the >>> original files first). Yet not dealing with them - especially if there >>> are many - reduces the benefit of the work we do quite a bit, because it >>> may leave much more work for downstreams to do to actually be able to do >>> any certification. That may go to the extent of questioning why we would >>> bother dealing with a few dozen violations if hundreds remain but are >>> hidden. > > Yes, we need to figure out a way to deal with these files. However, at > the moment they are getting in the way of easier low hanging fruits. > > One "easy" low hanging fruit is to use cppcheck to scan new patches for > new MISRA violations. That would give immediate benefits to the > community. It is not easy to "diff" results with cppcheck and in any > case it would help a lot if we had a cleaner baseline because it would > make it far easier to read the results and make sense of them. > > The goal of this exclusion list is to give us that: a cleaner baseline > so that we can make progress faster on low hanging fruits. This list is > not meant to be fixed and stay unchanged for a long time. In fact, it > could even live under automation/ as part of the gitlab-ci test that > triggers cppcheck, if we prefer. Okay, that sounds reasonable to me as an intermediate goal. But then description of the change and justifications should also state so imo (for the latter e.g. "from Linux, ignore for now"). Jan
Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
On Thu, 16 Feb 2023, Luca Fancellu wrote: > > On 16 Feb 2023, at 08:19, Jan Beulich wrote: > > On 16.02.2023 00:49, Stefano Stabellini wrote: > >> On Wed, 15 Feb 2023, Julien Grall wrote: > >>> 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. > > > > I guess you mean s/would/could/. Personally I find "imported from Linux" > > as an entirely unacceptable justification: Why would the origin of a file > > matter on whether it has violations? Dealing with the violations may be > > more cumbersome (because preferably the adjustments would go to the > > original files first). Yet not dealing with them - especially if there > > are many - reduces the benefit of the work we do quite a bit, because it > > may leave much more work for downstreams to do to actually be able to do > > any certification. That may go to the extent of questioning why we would > > bother dealing with a few dozen violations if hundreds remain but are > > hidden. Yes, we need to figure out a way to deal with these files. However, at the moment they are getting in the way of easier low hanging fruits. One "easy" low hanging fruit is to use cppcheck to scan new patches for new MISRA violations. That would give immediate benefits to the community. It is not easy to "diff" results with cppcheck and in any case it would help a lot if we had a cleaner baseline because it would make it far easier to read the results and make sense of them. The goal of this exclusion list is to give us that: a cleaner baseline so that we can make progress faster on low hanging fruits. This list is not meant to be fixed and stay unchanged for a long time. In fact, it could even live under automation/ as part of the gitlab-ci test that triggers cppcheck, if we prefer. > Hi Jan, > > my personal opinion is that we can’t handle them for files that needs to be > kept > in sync from an external source, because we can’t justify findings or tag > false > positives, if we are lucky we might fix the non compliances but even in that > case > there might be times when the fix goes through and cases where the fix is > objected > by the maintainers just because their project is not following the MISRA > standard. > > On our side, what we can do is track these files and from time to time check > that > they are still eligible to backport, because when they are not anymore we > could > just port them to Xen coding style and start doing direct changes. > > > @Stefano/@Bertrand/@Julien thanks for your effort on the list, yesterday > morning > I’ve also had a look on the Michal’s file list and I’ve tracked down the > origin, here > my findings, maybe we could merge your list with mine? Yes to merge the two lists, but I think we should keep only items that we actually need for the sake of having a cleaner baseline. So I would only add things we need today to reduce the noise on cppcheck results (excluding 20.7 and also excluding unusedStructMember which I think
Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
> On 16 Feb 2023, at 08:19, Jan Beulich wrote:
>
> On 16.02.2023 00:49, Stefano Stabellini wrote:
>> On Wed, 15 Feb 2023, Julien Grall wrote:
>>> 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.
>
> I guess you mean s/would/could/. Personally I find "imported from Linux"
> as an entirely unacceptable justification: Why would the origin of a file
> matter on whether it has violations? Dealing with the violations may be
> more cumbersome (because preferably the adjustments would go to the
> original files first). Yet not dealing with them - especially if there
> are many - reduces the benefit of the work we do quite a bit, because it
> may leave much more work for downstreams to do to actually be able to do
> any certification. That may go to the extent of questioning why we would
> bother dealing with a few dozen violations if hundreds remain but are
> hidden.
Hi Jan,
my personal opinion is that we can’t handle them for files that needs to be kept
in sync from an external source, because we can’t justify findings or tag false
positives, if we are lucky we might fix the non compliances but even in that
case
there might be times when the fix goes through and cases where the fix is
objected
by the maintainers just because their project is not following the MISRA
standard.
On our side, what we can do is track these files and from time to time check
that
they are still eligible to backport, because when they are not anymore we could
just port them to Xen coding style and start doing direct changes.
@Stefano/@Bertrand/@Julien thanks for your effort on the list, yesterday morning
I’ve also had a look on the Michal’s file list and I’ve tracked down the
origin, here
my findings, maybe we could merge your list with mine?
{
"version": "1.0",
"content": [
{
"rel_path": "arch/arm/arm64/cpufeature.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/arm/arm64/insn.c",
"comment": "Imported on Linux"
},
{
"rel_path": "arch/arm/arm64/lib/find_next_bit.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/acpi/boot.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/acpi/cpu_idle.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/acpi/cpufreq/cpufreq.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/acpi/cpuidle_menu.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/acpi/lib.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/cpu/amd.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/cpu/centaur.c",
"comment": "Imported from Linux"
},
{
"rel_path": "arch/x86/cpu/common.c",
Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
On 16.02.2023 00:49, Stefano Stabellini wrote: > On Wed, 15 Feb 2023, Julien Grall wrote: >> 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. I guess you mean s/would/could/. Personally I find "imported from Linux" as an entirely unacceptable justification: Why would the origin of a file matter on whether it has violations? Dealing with the violations may be more cumbersome (because preferably the adjustments would go to the original files first). Yet not dealing with them - especially if there are many - reduces the benefit of the work we do quite a bit, because it may leave much more work for downstreams to do to actually be able to do any certification. That may go to the extent of questioning why we would bother dealing with a few dozen violations if hundreds remain but are hidden. Jan
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"
Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
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)? 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. Cheers, -- Julien Grall
Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
On Tue, 14 Feb 2023, Jan Beulich wrote:
> On 14.02.2023 09:56, Luca Fancellu wrote:
> > --- a/docs/misra/exclude-list.json
> > +++ b/docs/misra/exclude-list.json
> > @@ -1,4 +1,209 @@
> > {
> > "version": "1.0",
> > -"content": []
> > +"content": [
> > +{
> > +"rel_path": "arch/arm/arm64/cpufeature.c"
> > +},
> > +{
> > +"rel_path": "arch/arm/arm64/insn.c"
> > +},
> > +{
> > +"rel_path": "arch/arm/arm64/lib/find_next_bit.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/acpi/boot.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/acpi/cpu_idle.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/acpi/cpufreq/cpufreq.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/acpi/cpuidle_menu.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/acpi/lib.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/acpi/power.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/cpu/amd.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/cpu/centaur.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/cpu/common.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/cpu/hygon.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/cpu/intel.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/cpu/intel_cacheinfo.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/cpu/mcheck/mce-apei.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/cpu/mcheck/non-fatal.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/cpu/mtrr/*"
> > +},
> > +{
> > +"rel_path": "arch/x86/cpu/mwait-idle.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/delay.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/dmi_scan.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/domain.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/genapic/*"
> > +},
> > +{
> > +"rel_path": "arch/x86/i387.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/irq.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/mpparse.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/srat.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/time.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/traps.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/usercopy.c"
> > +},
> > +{
> > +"rel_path": "arch/x86/x86_64/mmconf-fam10h.c"
> > +},
> > +{
> > +"rel_path": "common/bitmap.c"
> > +},
> > +{
> > +"rel_path": "common/bunzip2.c"
> > +},
> > +{
> > +"rel_path": "common/cpu.c"
> > +},
> > +{
> > +"rel_path": "common/earlycpio.c"
> > +},
> > +{
> > +"rel_path": "common/inflate.c"
> > +},
> > +{
> > +"rel_path": "common/libfdt/*"
> > +},
> > +{
> > +"rel_path": "common/lz4/decompress.c"
> > +},
> > +{
> > +"rel_path": "common/notifier.c"
> > +},
> > +{
> > +"rel_path": "common/radix-tree.c"
> > +},
> > +{
> > +"rel_path": "common/rcupdate.c"
> > +},
> > +{
> > +"rel_path": "common/softirq.c"
> > +},
> > +{
> > +"rel_path": "common/tasklet.c"
> > +},
> > +{
> > +"rel_path": "common/ubsan/ubsan.c"
> > +},
> > +{
> > +"rel_path": "common/un*.c"
> > +},
> > +{
> > +"rel_path": "common/vsprintf.c"
> > +},
> > +{
> > +"rel_path": "common/xz/*"
> > +},
> > +{
> > +"rel_path": "common/zstd/*"
> > +},
> > +{
> > +"rel_path": "crypto/rijndael.c"
> > +},
> > +{
> > +"rel_path": "crypto/vmac.c"
> > +},
> > +{
> > +"rel_path": "drivers/acpi/apei/*"
> > +},
> > +{
> > +"rel_path": "drivers/acpi/hwregs.c"
> > +},
> > +{
> > +"rel_path": "drivers/acpi/numa.c"
> > +},
> > +{
> > +"rel_path": "drivers/acpi/osl.c"
> > +},
> > +{
> > +"rel_path": "drivers/acpi/reboot.c"
> > +},
> > +{
> > +"rel_path": "drivers/acpi/tables.c"
> > +},
> > +{
> > +"rel_path": "drivers/acpi/tables/*"
> > +},
> > +{
> > +
Re: [PATCH 2/2] xen/misra: add entries to exclude-list.json
On 14.02.2023 09:56, Luca Fancellu wrote:
> --- a/docs/misra/exclude-list.json
> +++ b/docs/misra/exclude-list.json
> @@ -1,4 +1,209 @@
> {
> "version": "1.0",
> -"content": []
> +"content": [
> +{
> +"rel_path": "arch/arm/arm64/cpufeature.c"
> +},
> +{
> +"rel_path": "arch/arm/arm64/insn.c"
> +},
> +{
> +"rel_path": "arch/arm/arm64/lib/find_next_bit.c"
> +},
> +{
> +"rel_path": "arch/x86/acpi/boot.c"
> +},
> +{
> +"rel_path": "arch/x86/acpi/cpu_idle.c"
> +},
> +{
> +"rel_path": "arch/x86/acpi/cpufreq/cpufreq.c"
> +},
> +{
> +"rel_path": "arch/x86/acpi/cpuidle_menu.c"
> +},
> +{
> +"rel_path": "arch/x86/acpi/lib.c"
> +},
> +{
> +"rel_path": "arch/x86/acpi/power.c"
> +},
> +{
> +"rel_path": "arch/x86/cpu/amd.c"
> +},
> +{
> +"rel_path": "arch/x86/cpu/centaur.c"
> +},
> +{
> +"rel_path": "arch/x86/cpu/common.c"
> +},
> +{
> +"rel_path": "arch/x86/cpu/hygon.c"
> +},
> +{
> +"rel_path": "arch/x86/cpu/intel.c"
> +},
> +{
> +"rel_path": "arch/x86/cpu/intel_cacheinfo.c"
> +},
> +{
> +"rel_path": "arch/x86/cpu/mcheck/mce-apei.c"
> +},
> +{
> +"rel_path": "arch/x86/cpu/mcheck/non-fatal.c"
> +},
> +{
> +"rel_path": "arch/x86/cpu/mtrr/*"
> +},
> +{
> +"rel_path": "arch/x86/cpu/mwait-idle.c"
> +},
> +{
> +"rel_path": "arch/x86/delay.c"
> +},
> +{
> +"rel_path": "arch/x86/dmi_scan.c"
> +},
> +{
> +"rel_path": "arch/x86/domain.c"
> +},
> +{
> +"rel_path": "arch/x86/genapic/*"
> +},
> +{
> +"rel_path": "arch/x86/i387.c"
> +},
> +{
> +"rel_path": "arch/x86/irq.c"
> +},
> +{
> +"rel_path": "arch/x86/mpparse.c"
> +},
> +{
> +"rel_path": "arch/x86/srat.c"
> +},
> +{
> +"rel_path": "arch/x86/time.c"
> +},
> +{
> +"rel_path": "arch/x86/traps.c"
> +},
> +{
> +"rel_path": "arch/x86/usercopy.c"
> +},
> +{
> +"rel_path": "arch/x86/x86_64/mmconf-fam10h.c"
> +},
> +{
> +"rel_path": "common/bitmap.c"
> +},
> +{
> +"rel_path": "common/bunzip2.c"
> +},
> +{
> +"rel_path": "common/cpu.c"
> +},
> +{
> +"rel_path": "common/earlycpio.c"
> +},
> +{
> +"rel_path": "common/inflate.c"
> +},
> +{
> +"rel_path": "common/libfdt/*"
> +},
> +{
> +"rel_path": "common/lz4/decompress.c"
> +},
> +{
> +"rel_path": "common/notifier.c"
> +},
> +{
> +"rel_path": "common/radix-tree.c"
> +},
> +{
> +"rel_path": "common/rcupdate.c"
> +},
> +{
> +"rel_path": "common/softirq.c"
> +},
> +{
> +"rel_path": "common/tasklet.c"
> +},
> +{
> +"rel_path": "common/ubsan/ubsan.c"
> +},
> +{
> +"rel_path": "common/un*.c"
> +},
> +{
> +"rel_path": "common/vsprintf.c"
> +},
> +{
> +"rel_path": "common/xz/*"
> +},
> +{
> +"rel_path": "common/zstd/*"
> +},
> +{
> +"rel_path": "crypto/rijndael.c"
> +},
> +{
> +"rel_path": "crypto/vmac.c"
> +},
> +{
> +"rel_path": "drivers/acpi/apei/*"
> +},
> +{
> +"rel_path": "drivers/acpi/hwregs.c"
> +},
> +{
> +"rel_path": "drivers/acpi/numa.c"
> +},
> +{
> +"rel_path": "drivers/acpi/osl.c"
> +},
> +{
> +"rel_path": "drivers/acpi/reboot.c"
> +},
> +{
> +"rel_path": "drivers/acpi/tables.c"
> +},
> +{
> +"rel_path": "drivers/acpi/tables/*"
> +},
> +{
> +"rel_path": "drivers/acpi/utilities/*"
> +},
> +{
> +"rel_path": "drivers/char/ehci-dbgp.c"
> +},
> +{
> +"rel_path": "drivers/char/xhci-dbc.c"
> +},
> +{
> +"rel_path": "drivers/cpufreq/*"
> +},
> +{
> +"rel_path": "drivers/video/font_*"
> +},
> +{
> +
[PATCH 2/2] xen/misra: add entries to exclude-list.json
Add entries to the exclude-list.json for those files that need to be
excluded from the analysis scan.
Signed-off-by: Luca Fancellu
Signed-off-by: Michal Orzel
---
This list is originated from Michal's work here:
https://patchwork.kernel.org/project/xen-devel/patch/20221116092032.4423-1-michal.or...@amd.com/#25123099
the only files removed are the *arm/smmu* that, if I understand
correctly, we would like to include in the scan.
---
---
docs/misra/exclude-list.json | 207 ++-
1 file changed, 206 insertions(+), 1 deletion(-)
diff --git a/docs/misra/exclude-list.json b/docs/misra/exclude-list.json
index 1fb0ac67747b..3a6dc0809af5 100644
--- a/docs/misra/exclude-list.json
+++ b/docs/misra/exclude-list.json
@@ -1,4 +1,209 @@
{
"version": "1.0",
-"content": []
+"content": [
+{
+"rel_path": "arch/arm/arm64/cpufeature.c"
+},
+{
+"rel_path": "arch/arm/arm64/insn.c"
+},
+{
+"rel_path": "arch/arm/arm64/lib/find_next_bit.c"
+},
+{
+"rel_path": "arch/x86/acpi/boot.c"
+},
+{
+"rel_path": "arch/x86/acpi/cpu_idle.c"
+},
+{
+"rel_path": "arch/x86/acpi/cpufreq/cpufreq.c"
+},
+{
+"rel_path": "arch/x86/acpi/cpuidle_menu.c"
+},
+{
+"rel_path": "arch/x86/acpi/lib.c"
+},
+{
+"rel_path": "arch/x86/acpi/power.c"
+},
+{
+"rel_path": "arch/x86/cpu/amd.c"
+},
+{
+"rel_path": "arch/x86/cpu/centaur.c"
+},
+{
+"rel_path": "arch/x86/cpu/common.c"
+},
+{
+"rel_path": "arch/x86/cpu/hygon.c"
+},
+{
+"rel_path": "arch/x86/cpu/intel.c"
+},
+{
+"rel_path": "arch/x86/cpu/intel_cacheinfo.c"
+},
+{
+"rel_path": "arch/x86/cpu/mcheck/mce-apei.c"
+},
+{
+"rel_path": "arch/x86/cpu/mcheck/non-fatal.c"
+},
+{
+"rel_path": "arch/x86/cpu/mtrr/*"
+},
+{
+"rel_path": "arch/x86/cpu/mwait-idle.c"
+},
+{
+"rel_path": "arch/x86/delay.c"
+},
+{
+"rel_path": "arch/x86/dmi_scan.c"
+},
+{
+"rel_path": "arch/x86/domain.c"
+},
+{
+"rel_path": "arch/x86/genapic/*"
+},
+{
+"rel_path": "arch/x86/i387.c"
+},
+{
+"rel_path": "arch/x86/irq.c"
+},
+{
+"rel_path": "arch/x86/mpparse.c"
+},
+{
+"rel_path": "arch/x86/srat.c"
+},
+{
+"rel_path": "arch/x86/time.c"
+},
+{
+"rel_path": "arch/x86/traps.c"
+},
+{
+"rel_path": "arch/x86/usercopy.c"
+},
+{
+"rel_path": "arch/x86/x86_64/mmconf-fam10h.c"
+},
+{
+"rel_path": "common/bitmap.c"
+},
+{
+"rel_path": "common/bunzip2.c"
+},
+{
+"rel_path": "common/cpu.c"
+},
+{
+"rel_path": "common/earlycpio.c"
+},
+{
+"rel_path": "common/inflate.c"
+},
+{
+"rel_path": "common/libfdt/*"
+},
+{
+"rel_path": "common/lz4/decompress.c"
+},
+{
+"rel_path": "common/notifier.c"
+},
+{
+"rel_path": "common/radix-tree.c"
+},
+{
+"rel_path": "common/rcupdate.c"
+},
+{
+"rel_path": "common/softirq.c"
+},
+{
+"rel_path": "common/tasklet.c"
+},
+{
+"rel_path": "common/ubsan/ubsan.c"
+},
+{
+"rel_path": "common/un*.c"
+},
+{
+"rel_path": "common/vsprintf.c"
+},
+{
+"rel_path": "common/xz/*"
+},
+{
+"rel_path": "common/zstd/*"
+},
+{
+"rel_path": "crypto/rijndael.c"
+},
+{
+"rel_path": "crypto/vmac.c"
+},
+{
+"rel_path": "drivers/acpi/apei/*"
+},
+{
+"rel_path": "drivers/acpi/hwregs.c"
+},
+{
+"rel_path": "drivers/acpi/numa.c"
+},
+{
+"rel_path": "drivers/acpi/osl.c"
+},
+{
+"rel_path": "drivers/acpi/reboot.c"
+},
+{
+"rel_path": "drivers/acpi/tables.c"
+},
+{
+"rel_path": "drivers/acpi/tables/*"
+},
+{
+"rel_path": "drivers/acpi/utilities/*"
+},
+{
+"rel_path": "drivers/char/ehci-dbgp.c"
+
