| Message ID | 20221220085100.22848-2-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cppcheck rule 20.7 fixes | expand |
Hi Luca, On 20/12/2022 08:50, Luca Fancellu wrote: > Cppcheck reports violations of rule 20.7 in two macros of > alternative.h. > > The first one is in the __ALT_PTR macro where cppcheck wants to have > parentheses on the second operand of a member access operator, which > is not allowed from the c language syntax, furthermore, the macro > parameter is never used as an expression, hence we can suppress the > finding. Looking at the Misra Rule 20.7 examples [1], this looks similar to GET_MEMBER(). So I don't understand why cppcheck is complaining. > > The second finding is in the __ALTERNATIVE_CFG macro, where cppcheck > wants to have parentheses on the macro arguments, but the macro > parameters are never used as expressions and are used only for text > substitution, so cppcheck is not taking into account the context of > use of the macro arguments and adding parenthesis would break the > code, so we can suppress the finding. This reads like you want to report a bug against cppcheck. > > No error was shown by eclair and coverity for those lines. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > docs/misra/false-positive-cppcheck.json | 14 ++++++++++++++ > xen/arch/arm/include/asm/alternative.h | 2 ++ This file is meant to be close to Linux. From my understanding of the discussion yesterday, we didn't want to add deviation there. > 2 files changed, 16 insertions(+) > > diff --git a/docs/misra/false-positive-cppcheck.json b/docs/misra/false-positive-cppcheck.json > index 5d4da2ce6170..5e7d9377f60b 100644 > --- a/docs/misra/false-positive-cppcheck.json > +++ b/docs/misra/false-positive-cppcheck.json > @@ -3,6 +3,20 @@ > "content": [ > { > "id": "SAF-0-false-positive-cppcheck", > + "violation-id": "misra-c2012-20.7", > + "tool-version": "2.7", > + "name": "R20.7 second operand of member-access operator", > + "text": "The second operand of a member access operator shall be a name of a member of the type pointed to, so in this particular case it is wrong to use parentheses on the macro parameter." > + }, > + { > + "id": "SAF-1-false-positive-cppcheck", > + "violation-id": "misra-c2012-20.7", > + "tool-version": "2.7", > + "name": "R20.7 C macro parameters used only for text substitution", > + "text": "The macro parameters used in this case are not part of an expression, they are used for text substitution." > + }, In both cases the constructs are commonly used in Xen to generate code. So I am a bit concerned to have to add deviation everywhere cppcheck is wrong. More generally, we are still at the beginning of MISRA in Xen and I don't think we should start with adding deviations from bugs in the tools. Instead, we should report those bugs and hopefully by the time Xen is mostly MISRA complaint the tools will not report the false positive. If they are still then we can decide to add deviations. Cheers, [1] https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_20_07.c
On 20.12.2022 10:07, Julien Grall wrote: > On 20/12/2022 08:50, Luca Fancellu wrote: >> --- a/docs/misra/false-positive-cppcheck.json >> +++ b/docs/misra/false-positive-cppcheck.json >> @@ -3,6 +3,20 @@ >> "content": [ >> { >> "id": "SAF-0-false-positive-cppcheck", >> + "violation-id": "misra-c2012-20.7", >> + "tool-version": "2.7", >> + "name": "R20.7 second operand of member-access operator", >> + "text": "The second operand of a member access operator shall be a name of a member of the type pointed to, so in this particular case it is wrong to use parentheses on the macro parameter." >> + }, >> + { >> + "id": "SAF-1-false-positive-cppcheck", >> + "violation-id": "misra-c2012-20.7", >> + "tool-version": "2.7", >> + "name": "R20.7 C macro parameters used only for text substitution", >> + "text": "The macro parameters used in this case are not part of an expression, they are used for text substitution." >> + }, > In both cases the constructs are commonly used in Xen to generate code. > So I am a bit concerned to have to add deviation everywhere cppcheck is > wrong. > > More generally, we are still at the beginning of MISRA in Xen and I > don't think we should start with adding deviations from bugs in the > tools. Instead, we should report those bugs and hopefully by the time > Xen is mostly MISRA complaint the tools will not report the false positive. > > If they are still then we can decide to add deviations. +1 Jan
On Tue, 20 Dec 2022, Julien Grall wrote: > Hi Luca, > > On 20/12/2022 08:50, Luca Fancellu wrote: > > Cppcheck reports violations of rule 20.7 in two macros of > > alternative.h. > > > > The first one is in the __ALT_PTR macro where cppcheck wants to have > > parentheses on the second operand of a member access operator, which > > is not allowed from the c language syntax, furthermore, the macro > > parameter is never used as an expression, hence we can suppress the > > finding. > > Looking at the Misra Rule 20.7 examples [1], this looks similar to > GET_MEMBER(). So I don't understand why cppcheck is complaining. > > > > > The second finding is in the __ALTERNATIVE_CFG macro, where cppcheck > > wants to have parentheses on the macro arguments, but the macro > > parameters are never used as expressions and are used only for text > > substitution, so cppcheck is not taking into account the context of > > use of the macro arguments and adding parenthesis would break the > > code, so we can suppress the finding. > > This reads like you want to report a bug against cppcheck. +1 > > No error was shown by eclair and coverity for those lines. > > > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > > --- > > docs/misra/false-positive-cppcheck.json | 14 ++++++++++++++ > > xen/arch/arm/include/asm/alternative.h | 2 ++ > > This file is meant to be close to Linux. From my understanding of the > discussion yesterday, we didn't want to add deviation there. Yeah. We are still finalizing the list of Linux files in Xen (https://marc.info/?l=xen-devel&m=166859007703945) so we don't have a programmatic way to check if something should be scanned or not. At the moment it is easy to make mistakes. I hope will get it committed soon, then we can blacklist anything listed under docs/misra/external-files.txt (automatically ask cppcheck to avoid scanning files listed in docs/misra/external-files.txt). > > 2 files changed, 16 insertions(+) > > > > diff --git a/docs/misra/false-positive-cppcheck.json > > b/docs/misra/false-positive-cppcheck.json > > index 5d4da2ce6170..5e7d9377f60b 100644 > > --- a/docs/misra/false-positive-cppcheck.json > > +++ b/docs/misra/false-positive-cppcheck.json > > @@ -3,6 +3,20 @@ > > "content": [ > > { > > "id": "SAF-0-false-positive-cppcheck", > > + "violation-id": "misra-c2012-20.7", > > + "tool-version": "2.7", > > + "name": "R20.7 second operand of member-access operator", > > + "text": "The second operand of a member access operator shall > > be a name of a member of the type pointed to, so in this particular case it > > is wrong to use parentheses on the macro parameter." > > + }, > > + { > > + "id": "SAF-1-false-positive-cppcheck", > > + "violation-id": "misra-c2012-20.7", > > + "tool-version": "2.7", > > + "name": "R20.7 C macro parameters used only for text > > substitution", > > + "text": "The macro parameters used in this case are not part of > > an expression, they are used for text substitution." > > + }, > In both cases the constructs are commonly used in Xen to generate code. So I > am a bit concerned to have to add deviation everywhere cppcheck is wrong. > > More generally, we are still at the beginning of MISRA in Xen and I don't > think we should start with adding deviations from bugs in the tools. Instead, > we should report those bugs and hopefully by the time Xen is mostly MISRA > complaint the tools will not report the false positive. > > If they are still then we can decide to add deviations. I also think we should report cppcheck bugs. That said, I think the reason why Luca submitted this series is that we are actually not far from having cppcheck running automatically as part of gitlab-ci to scan staging and new patches. Enabling automatic cppcheck scans is something within our reach, I'd say it is only few weeks away. That's actually going to start giving some concrete benefits for our work on MISRA C. We'll get automatic bug reports and quality reports, which is pretty cool. Aside from the list of Linux files, we also need to deal with false positives otherwise the reports will be too "noisy". If we correctly silence the outstanding false-positive cppcheck reports, we can actually have a clean "zero issues" report by cppcheck on staging with the current list of MISRA C rules supported by docs/misra/rules.rst. And there are only few cppcheck false positives to deal with. Most of them are for Rule 20.7. During the last FuSa call, I actually suggested to skip Rule 20.7 when it comes to scanning for issues (i.e. blacklist Rule 20.7). Luca suggested that before doing that we could at least try to fix/deviate the violations reported by cppcheck (they are far fewer than the ones reported by Eclair). So here we are :-) So in short: - This is not the first of many series flooding xen-devel adding deviations; this is the only series we need to get zero false-positives from cppcheck now - With this series, and with the linux files to skip, we could add cppcheck to gitlab-ci today - Alternatively, we could disable rule 20.7 for now - We could wait for cppcheck to improve and address our bug reports, but it is likely not going to enable us to make progress quickly. If we want to wait, I would rather disable 20.7 until cppcheck improves so that we can still add cppcheck to gitlab-ci soon. I think that would be OK too What do you guys think? Nice automatic 20.7 scans for all patches and for staging, but with the undesirable false-positive comments, or no-20.7 scans but no comments in the tree?
On 22.12.2022 03:01, Stefano Stabellini wrote: > What do you guys think? Nice automatic 20.7 scans for all patches and > for staging, but with the undesirable false-positive comments, or > no-20.7 scans but no comments in the tree? Anything automatic which then also bugs submitters about issues should have as few false positives as possible. Otherwise people may quickly get into the habit of ignoring such reports. Jan
On Thu, 22 Dec 2022, Jan Beulich wrote: > On 22.12.2022 03:01, Stefano Stabellini wrote: > > What do you guys think? Nice automatic 20.7 scans for all patches and > > for staging, but with the undesirable false-positive comments, or > > no-20.7 scans but no comments in the tree? > > Anything automatic which then also bugs submitters about issues should > have as few false positives as possible. Otherwise people may quickly > get into the habit of ignoring such reports. You have a point. That said, it would be easy to spot false positives in new patches once we start from a clean slate, but then if we merge the patch we would also have to add a deviation comment for the false positive. All in all, maybe it is best to skip checking for 20.7 for now in gitlab-ci.
diff --git a/docs/misra/false-positive-cppcheck.json b/docs/misra/false-positive-cppcheck.json index 5d4da2ce6170..5e7d9377f60b 100644 --- a/docs/misra/false-positive-cppcheck.json +++ b/docs/misra/false-positive-cppcheck.json @@ -3,6 +3,20 @@ "content": [ { "id": "SAF-0-false-positive-cppcheck", + "violation-id": "misra-c2012-20.7", + "tool-version": "2.7", + "name": "R20.7 second operand of member-access operator", + "text": "The second operand of a member access operator shall be a name of a member of the type pointed to, so in this particular case it is wrong to use parentheses on the macro parameter." + }, + { + "id": "SAF-1-false-positive-cppcheck", + "violation-id": "misra-c2012-20.7", + "tool-version": "2.7", + "name": "R20.7 C macro parameters used only for text substitution", + "text": "The macro parameters used in this case are not part of an expression, they are used for text substitution." + }, + { + "id": "SAF-2-false-positive-cppcheck", "violation-id": "", "tool-version": "", "name": "Sentinel", diff --git a/xen/arch/arm/include/asm/alternative.h b/xen/arch/arm/include/asm/alternative.h index 1eb4b60fbb3e..9d4dc53bb0c6 100644 --- a/xen/arch/arm/include/asm/alternative.h +++ b/xen/arch/arm/include/asm/alternative.h @@ -20,6 +20,7 @@ struct alt_instr { }; /* Xen: helpers used by common code. */ +/* SAF-0-false-positive-cppcheck R20.7 member-access operator */ #define __ALT_PTR(a,f) ((void *)&(a)->f + (a)->f) #define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset) #define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset) @@ -58,6 +59,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en * * Alternatives with callbacks do not generate replacement instructions. */ +/* SAF-1-false-positive-cppcheck R20.7 argument for text substitution */ #define __ALTERNATIVE_CFG(oldinstr, newinstr, feature, cfg_enabled, cb) \ ".if "__stringify(cfg_enabled)" == 1\n" \ "661:\n\t" \
Cppcheck reports violations of rule 20.7 in two macros of alternative.h. The first one is in the __ALT_PTR macro where cppcheck wants to have parentheses on the second operand of a member access operator, which is not allowed from the c language syntax, furthermore, the macro parameter is never used as an expression, hence we can suppress the finding. The second finding is in the __ALTERNATIVE_CFG macro, where cppcheck wants to have parentheses on the macro arguments, but the macro parameters are never used as expressions and are used only for text substitution, so cppcheck is not taking into account the context of use of the macro arguments and adding parenthesis would break the code, so we can suppress the finding. No error was shown by eclair and coverity for those lines. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- docs/misra/false-positive-cppcheck.json | 14 ++++++++++++++ xen/arch/arm/include/asm/alternative.h | 2 ++ 2 files changed, 16 insertions(+)