| Message ID | 20230125205735.2662514-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add more rules to docs/misra/rules.rst | expand |
On 25.01.2023 21:57, Stefano Stabellini wrote: > From: Stefano Stabellini <stefano.stabellini@amd.com> > > 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. 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. Jan
On Thu, 26 Jan 2023, Jan Beulich wrote: > On 25.01.2023 21:57, Stefano Stabellini wrote: > > From: Stefano Stabellini <stefano.stabellini@amd.com> > > > > 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.
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@amd.com> >>> >>> 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. Jan
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@amd.com> > >>> > >>> 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.)
On 26.01.2023 19:54, Stefano Stabellini wrote: > 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. I disagree. Looking back at the sheet, it says "rule already followed by the community in most cases" which I assume was based on there being only very few violations that are presently reported. Now we've found the frame_table[] issue, I'm inclined to say that the statement was put there by mistake (due to that oversight). Furthermore I notice that I had put a reference to 18.1 there. And indeed I (continue to) think that the two can only sensibly be accepted together. One might consider 18.3 in the same group actually (I have a similar note there referring to 18.1), but luckily we look to not have a lot of issues there, so accepting that ahead of time was hopefully okay. Jan
On Fri, 27 Jan 2023, Jan Beulich wrote: > On 26.01.2023 19:54, Stefano Stabellini wrote: > > 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. > > I disagree. OK. I'll resend this patch, removing 18.2. I'll mark it appropriately in the sheet as well. > Looking back at the sheet, it says "rule already followed by > the community in most cases" which I assume was based on there being > only very few violations that are presently reported. Now we've found > the frame_table[] issue, I'm inclined to say that the statement was put > there by mistake (due to that oversight). cppcheck is unable to find violations; we know cppcheck has limitations and that's OK. Eclair is excellent and finds violations (including the frame_table[] issue you mentioned), but currently it doesn't read configs from xen.git and we cannot run a test to see if adding a couple of deviations for 2 macros removes most of the violations. If we want to use Eclair as a reference (could be a good idea) then I think we need a better integration. I'll talk to Roberto and see if we can arrange something better. I am writing this with the assumption that if I could show that, as an example, adding 2 deviations reduces the Eclair violations down to less than 10, then we could adopt the rule. Do you think that would be acceptable in your opinion, as a process?
On 27.01.2023 19:33, Stefano Stabellini wrote: > On Fri, 27 Jan 2023, Jan Beulich wrote: >> On 26.01.2023 19:54, Stefano Stabellini wrote: >> Looking back at the sheet, it says "rule already followed by >> the community in most cases" which I assume was based on there being >> only very few violations that are presently reported. Now we've found >> the frame_table[] issue, I'm inclined to say that the statement was put >> there by mistake (due to that oversight). > > cppcheck is unable to find violations; we know cppcheck has limitations > and that's OK. > > Eclair is excellent and finds violations (including the frame_table[] > issue you mentioned), but currently it doesn't read configs from xen.git > and we cannot run a test to see if adding a couple of deviations for 2 > macros removes most of the violations. If we want to use Eclair as a > reference (could be a good idea) then I think we need a better > integration. I'll talk to Roberto and see if we can arrange something > better. > > I am writing this with the assumption that if I could show that, as an > example, adding 2 deviations reduces the Eclair violations down to less > than 10, then we could adopt the rule. Do you think that would be > acceptable in your opinion, as a process? Hmm, to be quite honest: Not sure. Having noticed the oversight of the frame_table[] issue makes me wonder how much else may be missed in this same area (18.1, 18.2, and 18.3). Jan
> On 30 Jan 2023, at 07:33, Jan Beulich <jbeulich@suse.com> wrote: > > On 27.01.2023 19:33, Stefano Stabellini wrote: >> On Fri, 27 Jan 2023, Jan Beulich wrote: >>> On 26.01.2023 19:54, Stefano Stabellini wrote: >>> Looking back at the sheet, it says "rule already followed by >>> the community in most cases" which I assume was based on there being >>> only very few violations that are presently reported. Now we've found >>> the frame_table[] issue, I'm inclined to say that the statement was put >>> there by mistake (due to that oversight). >> >> cppcheck is unable to find violations; we know cppcheck has limitations >> and that's OK. >> >> Eclair is excellent and finds violations (including the frame_table[] >> issue you mentioned), but currently it doesn't read configs from xen.git >> and we cannot run a test to see if adding a couple of deviations for 2 >> macros removes most of the violations. If we want to use Eclair as a >> reference (could be a good idea) then I think we need a better >> integration. I'll talk to Roberto and see if we can arrange something >> better. >> >> I am writing this with the assumption that if I could show that, as an >> example, adding 2 deviations reduces the Eclair violations down to less >> than 10, then we could adopt the rule. Do you think that would be >> acceptable in your opinion, as a process? > > Hmm, to be quite honest: Not sure. Having noticed the oversight of the > frame_table[] issue makes me wonder how much else may be missed in this > same area (18.1, 18.2, and 18.3). Hi Jan, I think I recall the frame_table[] issue but I was looking into the eclair reports to understand it better and I was unable to find it, do you recall where the tool was complaining for the 18.2 related to the frame_table[]? Any notes or link is appreciated, maybe we could speak with Roberto to understand It better, because I checked with Coverity and I was unable to link findings of 18.2 with the symbol frame_table[] (however I might be a bit lost in all the macros). Thank you. Cheers, Luca > > Jan
On 30.01.2023 10:32, Luca Fancellu wrote: > > >> On 30 Jan 2023, at 07:33, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 27.01.2023 19:33, Stefano Stabellini wrote: >>> On Fri, 27 Jan 2023, Jan Beulich wrote: >>>> On 26.01.2023 19:54, Stefano Stabellini wrote: >>>> Looking back at the sheet, it says "rule already followed by >>>> the community in most cases" which I assume was based on there being >>>> only very few violations that are presently reported. Now we've found >>>> the frame_table[] issue, I'm inclined to say that the statement was put >>>> there by mistake (due to that oversight). >>> >>> cppcheck is unable to find violations; we know cppcheck has limitations >>> and that's OK. >>> >>> Eclair is excellent and finds violations (including the frame_table[] >>> issue you mentioned), but currently it doesn't read configs from xen.git >>> and we cannot run a test to see if adding a couple of deviations for 2 >>> macros removes most of the violations. If we want to use Eclair as a >>> reference (could be a good idea) then I think we need a better >>> integration. I'll talk to Roberto and see if we can arrange something >>> better. >>> >>> I am writing this with the assumption that if I could show that, as an >>> example, adding 2 deviations reduces the Eclair violations down to less >>> than 10, then we could adopt the rule. Do you think that would be >>> acceptable in your opinion, as a process? >> >> Hmm, to be quite honest: Not sure. Having noticed the oversight of the >> frame_table[] issue makes me wonder how much else may be missed in this >> same area (18.1, 18.2, and 18.3). > > Hi Jan, > > I think I recall the frame_table[] issue but I was looking into the eclair reports to > understand it better and I was unable to find it, do you recall where the tool was > complaining for the 18.2 related to the frame_table[]? I think you're meaning to ask Stefano instead? I have no pointers into what Eclair may or may not have reported at any point in time. Jan > Any notes or link is appreciated, maybe we could speak with Roberto to understand > It better, because I checked with Coverity and I was unable to link findings of 18.2 with > the symbol frame_table[] (however I might be a bit lost in all the macros). > > Thank you. > > Cheers, > Luca > > >> >> Jan > >
On Mon, 30 Jan 2023, Luca Fancellu wrote: > > On 30 Jan 2023, at 07:33, Jan Beulich <jbeulich@suse.com> wrote: > > > > On 27.01.2023 19:33, Stefano Stabellini wrote: > >> On Fri, 27 Jan 2023, Jan Beulich wrote: > >>> On 26.01.2023 19:54, Stefano Stabellini wrote: > >>> Looking back at the sheet, it says "rule already followed by > >>> the community in most cases" which I assume was based on there being > >>> only very few violations that are presently reported. Now we've found > >>> the frame_table[] issue, I'm inclined to say that the statement was put > >>> there by mistake (due to that oversight). > >> > >> cppcheck is unable to find violations; we know cppcheck has limitations > >> and that's OK. > >> > >> Eclair is excellent and finds violations (including the frame_table[] > >> issue you mentioned), but currently it doesn't read configs from xen.git > >> and we cannot run a test to see if adding a couple of deviations for 2 > >> macros removes most of the violations. If we want to use Eclair as a > >> reference (could be a good idea) then I think we need a better > >> integration. I'll talk to Roberto and see if we can arrange something > >> better. > >> > >> I am writing this with the assumption that if I could show that, as an > >> example, adding 2 deviations reduces the Eclair violations down to less > >> than 10, then we could adopt the rule. Do you think that would be > >> acceptable in your opinion, as a process? > > > > Hmm, to be quite honest: Not sure. Having noticed the oversight of the > > frame_table[] issue makes me wonder how much else may be missed in this > > same area (18.1, 18.2, and 18.3). > > Hi Jan, > > I think I recall the frame_table[] issue but I was looking into the eclair reports to > understand it better and I was unable to find it, do you recall where the tool was > complaining for the 18.2 related to the frame_table[]? > Any notes or link is appreciated, maybe we could speak with Roberto to understand > It better, because I checked with Coverity and I was unable to link findings of 18.2 with > the symbol frame_table[] (however I might be a bit lost in all the macros). Eclair could find the following violation which is related to the frametable in arch/x86/mm.c:init_frametable_chunk: L229: memset(start, 0, end - start); https://eclairit.com:8443/job/XEN/lastBuild/Target=X86_64,agent=docker1/eclair/type.-1600986843/moduleName.-84949685/packageName.590865259/fileName.1553859513/ https://eclairit.com:3787/fs/var/lib/jenkins/jobs/XEN/configurations/axis-Target/X86_64/axis-agent/docker1/builds/686/archive/ECLAIR/out/PROJECT.ecd;/sources/xen/arch/x86/mm.c.html#R46776_1 Almost all the other 18.2 violations are due to the "guest_mode" macro and the NEEP_IP/OP HAVE_IO/OP macros in lzo.c.
diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst index dcceab9388..1da79f33c1 100644 --- a/docs/misra/rules.rst +++ b/docs/misra/rules.rst @@ -138,6 +138,16 @@ existing codebase are work-in-progress. - Single-bit named bit fields shall not be of a signed type - + * - `Rule 7.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_07_01.c>`_ + - Required + - Octal constants shall not be used + - + + * - `Rule 7.3 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_07_03.c>`_ + - Required + - The lowercase character l shall not be used in a literal suffix + - + * - `Rule 8.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_08_01.c>`_ - Required - Types shall be explicitly specified @@ -200,6 +210,11 @@ existing codebase are work-in-progress. expression which has potential side effects - + * - `Rule 13.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_13_01_1.c>`_ + - Required + - Initializer lists shall not contain persistent side effects + - + * - `Rule 14.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_14_01.c>`_ - Required - A loop counter shall not have essentially floating type @@ -227,6 +242,16 @@ existing codebase are work-in-progress. static keyword between the [ ] - + * - `Rule 18.2 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_18_02.c>`_ + - Required + - Subtraction between pointers shall only be applied to pointers that address elements of the same array + - + + * - `Rule 18.3 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_18_03.c>`_ + - Required + - The relational operators > >= < and <= shall not be applied to objects of pointer type except where they point into the same object + - + * - `Rule 19.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_19_01.c>`_ - Mandatory - An object shall not be assigned or copied to an overlapping