| Message ID | 4039623dc4341758f383ec49228c1e55e5862b0f.1693228255.git.simone.ballarin@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C:2012 Directive 4.10 | expand |
+Nicola, Luca On Mon, 28 Aug 2023, Simone Ballarin wrote: > xen/arch/x86/usercopy.c includes itself, so it is not supposed to > comply with Directive 4.10: > "Precautions shall be taken in order to prevent the contents of a > header file being included more than once" > > This patch adds a deviation for the file. > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> > > --- > automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++ > docs/misra/rules.rst | 2 ++ > 2 files changed, 6 insertions(+) > > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl > index 2681a4cff5..a7d4f29b43 100644 > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl > @@ -96,6 +96,10 @@ conform to the directive." > -config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"} > -doc_end > > +-doc_begin="xen/arch/x86/usercopy.c includes itself: it is not supposed to comply with the directive" > +-config=MC3R1.D4.10,reports+={deliberate, "all_area(all_loc(file("^xen/arch/x86/usercopy\\.c$")))"} > +-doc_end > + > # > # Series 5. > # > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst > index 4b1a7b02b6..45e13d0302 100644 > --- a/docs/misra/rules.rst > +++ b/docs/misra/rules.rst > @@ -62,6 +62,8 @@ maintainers if you want to suggest a change. > - Files that are intended to be included more than once do not need to > conform to the directive. Files that explicitly avoid inclusion guards > under specific circumstances do not need to conform the directive. > + xen/arch/x86/usercopy.c includes itself: it is not supposed to comply > + with the directive. We need to find a consistent way to document this kind of deviations in a non-ECLAIR specific way, without adding the complete list of deviations to rules.rst. Can we use safe.json and add an in-code comment at the top of usercopy.c? E.g.: diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c index b8c2d1cc0b..8bb591f472 100644 --- a/xen/arch/x86/usercopy.c +++ b/xen/arch/x86/usercopy.c @@ -1,3 +1,4 @@ +/* SAF-1-safe */ /* * User address space access functions. * Otherwise, maybe we should extend safe.json to also have an extra field with a list of paths. For instance see "files" below: { "version": "1.0", "content": [ { "id": "SAF-0-safe", "analyser": { "eclair": "MC3R1.R8.6", "coverity": "misra_c_2012_rule_8_6_violation" }, "name": "Rule 8.6: linker script defined symbols", "text": "It is safe to declare this symbol because it is defined in the linker script." }, { "id": "SAF-1-safe", "analyser": { "eclair": "MC3R1.D4.10" }, "name": "Dir 4.10: files that include themselves", "text": "Files purposely written to include themselves are not supposed to comply with D4.10.", "files": ["xen/arch/x86/usercopy.c"] }, { "id": "SAF-2-safe", "analyser": {}, "name": "Sentinel", "text": "Next ID to be used" } ] }
On 29.08.2023 00:27, Stefano Stabellini wrote: > On Mon, 28 Aug 2023, Simone Ballarin wrote: >> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >> @@ -96,6 +96,10 @@ conform to the directive." >> -config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"} >> -doc_end >> >> +-doc_begin="xen/arch/x86/usercopy.c includes itself: it is not supposed to comply with the directive" >> +-config=MC3R1.D4.10,reports+={deliberate, "all_area(all_loc(file("^xen/arch/x86/usercopy\\.c$")))"} >> +-doc_end >> + >> # >> # Series 5. >> # >> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst >> index 4b1a7b02b6..45e13d0302 100644 >> --- a/docs/misra/rules.rst >> +++ b/docs/misra/rules.rst >> @@ -62,6 +62,8 @@ maintainers if you want to suggest a change. >> - Files that are intended to be included more than once do not need to >> conform to the directive. Files that explicitly avoid inclusion guards >> under specific circumstances do not need to conform the directive. >> + xen/arch/x86/usercopy.c includes itself: it is not supposed to comply >> + with the directive. > > > We need to find a consistent way to document this kind of deviations in > a non-ECLAIR specific way, without adding the complete list of > deviations to rules.rst. +1 Especially rules.rst should not be modified to add mention of individual exceptions. That's simply not the purpose of the file, at least the way I understand it. > Can we use safe.json and add an in-code comment at the top of > usercopy.c? E.g.: Right, this ought to be the was to go. Question is whether ... > diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c > index b8c2d1cc0b..8bb591f472 100644 > --- a/xen/arch/x86/usercopy.c > +++ b/xen/arch/x86/usercopy.c > @@ -1,3 +1,4 @@ > +/* SAF-1-safe */ ... this (or any other) placement of the comment will actually do (not just for Eclair). Jan > /* > * User address space access functions. > * > > Otherwise, maybe we should extend safe.json to also have an extra field > with a list of paths. For instance see "files" below: > > { > "version": "1.0", > "content": [ > { > "id": "SAF-0-safe", > "analyser": { > "eclair": "MC3R1.R8.6", > "coverity": "misra_c_2012_rule_8_6_violation" > }, > "name": "Rule 8.6: linker script defined symbols", > "text": "It is safe to declare this symbol because it is defined in the linker script." > }, > { > "id": "SAF-1-safe", > "analyser": { > "eclair": "MC3R1.D4.10" > }, > "name": "Dir 4.10: files that include themselves", > "text": "Files purposely written to include themselves are not supposed to comply with D4.10.", > "files": ["xen/arch/x86/usercopy.c"] > }, > { > "id": "SAF-2-safe", > "analyser": {}, > "name": "Sentinel", > "text": "Next ID to be used" > } > ] > }
On 29/08/23 00:27, Stefano Stabellini wrote: > +Nicola, Luca > > On Mon, 28 Aug 2023, Simone Ballarin wrote: >> xen/arch/x86/usercopy.c includes itself, so it is not supposed to >> comply with Directive 4.10: >> "Precautions shall be taken in order to prevent the contents of a >> header file being included more than once" >> >> This patch adds a deviation for the file. >> >> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> >> >> --- >> automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++ >> docs/misra/rules.rst | 2 ++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl >> index 2681a4cff5..a7d4f29b43 100644 >> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >> @@ -96,6 +96,10 @@ conform to the directive." >> -config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"} >> -doc_end >> >> +-doc_begin="xen/arch/x86/usercopy.c includes itself: it is not supposed to comply with the directive" >> +-config=MC3R1.D4.10,reports+={deliberate, "all_area(all_loc(file("^xen/arch/x86/usercopy\\.c$")))"} >> +-doc_end >> + >> # >> # Series 5. >> # >> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst >> index 4b1a7b02b6..45e13d0302 100644 >> --- a/docs/misra/rules.rst >> +++ b/docs/misra/rules.rst >> @@ -62,6 +62,8 @@ maintainers if you want to suggest a change. >> - Files that are intended to be included more than once do not need to >> conform to the directive. Files that explicitly avoid inclusion guards >> under specific circumstances do not need to conform the directive. >> + xen/arch/x86/usercopy.c includes itself: it is not supposed to comply >> + with the directive. > > > We need to find a consistent way to document this kind of deviations in > a non-ECLAIR specific way, without adding the complete list of > deviations to rules.rst. > > Can we use safe.json and add an in-code comment at the top of > usercopy.c? E.g.: > > diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c > index b8c2d1cc0b..8bb591f472 100644 > --- a/xen/arch/x86/usercopy.c > +++ b/xen/arch/x86/usercopy.c > @@ -1,3 +1,4 @@ > +/* SAF-1-safe */ > /* > * User address space access functions. > * > > Otherwise, maybe we should extend safe.json to also have an extra field > with a list of paths. For instance see "files" below > > { > "version": "1.0", > "content": [ > { > "id": "SAF-0-safe", > "analyser": { > "eclair": "MC3R1.R8.6", > "coverity": "misra_c_2012_rule_8_6_violation" > }, > "name": "Rule 8.6: linker script defined symbols", > "text": "It is safe to declare this symbol because it is defined in the linker script." > }, > { > "id": "SAF-1-safe", > "analyser": { > "eclair": "MC3R1.D4.10" > }, > "name": "Dir 4.10: files that include themselves", > "text": "Files purposely written to include themselves are not supposed to comply with D4.10.", > "files": ["xen/arch/x86/usercopy.c"] > }, > { > "id": "SAF-2-safe", > "analyser": {}, > "name": "Sentinel", > "text": "Next ID to be used" > } > ] > } > In general, I prefer the first option for such ad hoc deviation (the comment at the beginning of the file): this way, anyone who touches the file will immediately see the comment and think as its changes will affect the deviation (is it still safe? is it still necessary?). To help the developer more, I think it is better to also add the "name" in the comment, this is my proposal: /* SAF-4-safe Dir 4.10: files that include themselves*/ /* * User address space access functions. * * Copyright 1997 Andi Kleen <ak@muc.de> * Copyright 1997 Linus Torvalds * Copyright 2002 Andi Kleen <ak@suse.de> */
On Wed, 30 Aug 2023, Simone Ballarin wrote: > On 29/08/23 00:27, Stefano Stabellini wrote: > > +Nicola, Luca > > > > On Mon, 28 Aug 2023, Simone Ballarin wrote: > > > xen/arch/x86/usercopy.c includes itself, so it is not supposed to > > > comply with Directive 4.10: > > > "Precautions shall be taken in order to prevent the contents of a > > > header file being included more than once" > > > > > > This patch adds a deviation for the file. > > > > > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> > > > > > > --- > > > automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++ > > > docs/misra/rules.rst | 2 ++ > > > 2 files changed, 6 insertions(+) > > > > > > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl > > > b/automation/eclair_analysis/ECLAIR/deviations.ecl > > > index 2681a4cff5..a7d4f29b43 100644 > > > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl > > > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl > > > @@ -96,6 +96,10 @@ conform to the directive." > > > -config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, > > > no inclusion guards apply and the caller is responsible.*\\*/$, > > > begin-1))"} > > > -doc_end > > > +-doc_begin="xen/arch/x86/usercopy.c includes itself: it is not supposed > > > to comply with the directive" > > > +-config=MC3R1.D4.10,reports+={deliberate, > > > "all_area(all_loc(file("^xen/arch/x86/usercopy\\.c$")))"} > > > +-doc_end > > > + > > > # > > > # Series 5. > > > # > > > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst > > > index 4b1a7b02b6..45e13d0302 100644 > > > --- a/docs/misra/rules.rst > > > +++ b/docs/misra/rules.rst > > > @@ -62,6 +62,8 @@ maintainers if you want to suggest a change. > > > - Files that are intended to be included more than once do not need > > > to > > > conform to the directive. Files that explicitly avoid inclusion > > > guards > > > under specific circumstances do not need to conform the > > > directive. > > > + xen/arch/x86/usercopy.c includes itself: it is not supposed to > > > comply > > > + with the directive. > > > > > > We need to find a consistent way to document this kind of deviations in > > a non-ECLAIR specific way, without adding the complete list of > > deviations to rules.rst. > > > > Can we use safe.json and add an in-code comment at the top of > > usercopy.c? E.g.: > > > > diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c > > index b8c2d1cc0b..8bb591f472 100644 > > --- a/xen/arch/x86/usercopy.c > > +++ b/xen/arch/x86/usercopy.c > > @@ -1,3 +1,4 @@ > > +/* SAF-1-safe */ > > /* > > * User address space access functions. > > * > > > Otherwise, maybe we should extend safe.json to also have an extra field > > with a list of paths. For instance see "files" below > > > { > > "version": "1.0", > > "content": [ > > { > > "id": "SAF-0-safe", > > "analyser": { > > "eclair": "MC3R1.R8.6", > > "coverity": "misra_c_2012_rule_8_6_violation" > > }, > > "name": "Rule 8.6: linker script defined symbols", > > "text": "It is safe to declare this symbol because it is > > defined in the linker script." > > }, > > { > > "id": "SAF-1-safe", > > "analyser": { > > "eclair": "MC3R1.D4.10" > > }, > > "name": "Dir 4.10: files that include themselves", > > "text": "Files purposely written to include themselves are not > > supposed to comply with D4.10.", > > "files": ["xen/arch/x86/usercopy.c"] > > }, > > { > > "id": "SAF-2-safe", > > "analyser": {}, > > "name": "Sentinel", > > "text": "Next ID to be used" > > } > > ] > > } > > > In general, I prefer the first option for such ad hoc deviation (the comment > at the beginning of the file): this way, anyone who touches the file will > immediately see the comment and think as its changes will affect the deviation > (is it still safe? is it still necessary?). > > To help the developer more, I think it is better to also add the "name" in the > comment, this is my proposal: > > /* SAF-4-safe Dir 4.10: files that include themselves*/ Yes, this is fine, it was always intended to be possible to add the name of the deviation or a short comment in the in-code comment > /* > * User address space access functions. > * > * Copyright 1997 Andi Kleen <ak@muc.de> > * Copyright 1997 Linus Torvalds > * Copyright 2002 Andi Kleen <ak@suse.de> > */ > > -- > Simone Ballarin, M.Sc. > > Field Application Engineer, BUGSENG (https://bugseng.com) >
On 31.08.2023 03:56, Stefano Stabellini wrote: > On Wed, 30 Aug 2023, Simone Ballarin wrote: >> On 29/08/23 00:27, Stefano Stabellini wrote: >>> On Mon, 28 Aug 2023, Simone Ballarin wrote: >>> --- a/xen/arch/x86/usercopy.c >>> +++ b/xen/arch/x86/usercopy.c >>> @@ -1,3 +1,4 @@ >>> +/* SAF-1-safe */ >>> /* >>> * User address space access functions. >>> * >>> > Otherwise, maybe we should extend safe.json to also have an extra field >>> with a list of paths. For instance see "files" below > >>> { >>> "version": "1.0", >>> "content": [ >>> { >>> "id": "SAF-0-safe", >>> "analyser": { >>> "eclair": "MC3R1.R8.6", >>> "coverity": "misra_c_2012_rule_8_6_violation" >>> }, >>> "name": "Rule 8.6: linker script defined symbols", >>> "text": "It is safe to declare this symbol because it is >>> defined in the linker script." >>> }, >>> { >>> "id": "SAF-1-safe", >>> "analyser": { >>> "eclair": "MC3R1.D4.10" >>> }, >>> "name": "Dir 4.10: files that include themselves", >>> "text": "Files purposely written to include themselves are not >>> supposed to comply with D4.10.", >>> "files": ["xen/arch/x86/usercopy.c"] >>> }, >>> { >>> "id": "SAF-2-safe", >>> "analyser": {}, >>> "name": "Sentinel", >>> "text": "Next ID to be used" >>> } >>> ] >>> } >>> >> In general, I prefer the first option for such ad hoc deviation (the comment >> at the beginning of the file): this way, anyone who touches the file will >> immediately see the comment and think as its changes will affect the deviation >> (is it still safe? is it still necessary?). >> >> To help the developer more, I think it is better to also add the "name" in the >> comment, this is my proposal: >> >> /* SAF-4-safe Dir 4.10: files that include themselves*/ > > Yes, this is fine, it was always intended to be possible to add the > name of the deviation or a short comment in the in-code comment But then either the directive number wants omitting, or the Misra version needs to also be stated. Jan
> On 28 Aug 2023, at 23:27, Stefano Stabellini <sstabellini@kernel.org> wrote: > > +Nicola, Luca > > On Mon, 28 Aug 2023, Simone Ballarin wrote: >> xen/arch/x86/usercopy.c includes itself, so it is not supposed to >> comply with Directive 4.10: >> "Precautions shall be taken in order to prevent the contents of a >> header file being included more than once" >> >> This patch adds a deviation for the file. >> >> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> >> >> --- >> automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++ >> docs/misra/rules.rst | 2 ++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl >> index 2681a4cff5..a7d4f29b43 100644 >> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >> @@ -96,6 +96,10 @@ conform to the directive." >> -config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"} >> -doc_end >> >> +-doc_begin="xen/arch/x86/usercopy.c includes itself: it is not supposed to comply with the directive" >> +-config=MC3R1.D4.10,reports+={deliberate, "all_area(all_loc(file("^xen/arch/x86/usercopy\\.c$")))"} >> +-doc_end >> + >> # >> # Series 5. >> # >> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst >> index 4b1a7b02b6..45e13d0302 100644 >> --- a/docs/misra/rules.rst >> +++ b/docs/misra/rules.rst >> @@ -62,6 +62,8 @@ maintainers if you want to suggest a change. >> - Files that are intended to be included more than once do not need to >> conform to the directive. Files that explicitly avoid inclusion guards >> under specific circumstances do not need to conform the directive. >> + xen/arch/x86/usercopy.c includes itself: it is not supposed to comply >> + with the directive. > > > We need to find a consistent way to document this kind of deviations in > a non-ECLAIR specific way, without adding the complete list of > deviations to rules.rst. > > Can we use safe.json and add an in-code comment at the top of > usercopy.c? E.g.: > > diff --git a/xen/arch/x86/usercopy.c b/xen/arch/x86/usercopy.c > index b8c2d1cc0b..8bb591f472 100644 > --- a/xen/arch/x86/usercopy.c > +++ b/xen/arch/x86/usercopy.c > @@ -1,3 +1,4 @@ > +/* SAF-1-safe */ > /* > * User address space access functions. > * > > Otherwise, maybe we should extend safe.json to also have an extra field > with a list of paths. For instance see "files" below: > > { > "version": "1.0", > "content": [ > { > "id": "SAF-0-safe", > "analyser": { > "eclair": "MC3R1.R8.6", > "coverity": "misra_c_2012_rule_8_6_violation" > }, > "name": "Rule 8.6: linker script defined symbols", > "text": "It is safe to declare this symbol because it is defined in the linker script." > }, > { > "id": "SAF-1-safe", > "analyser": { > "eclair": "MC3R1.D4.10" > }, > "name": "Dir 4.10: files that include themselves", > "text": "Files purposely written to include themselves are not supposed to comply with D4.10.", > "files": ["xen/arch/x86/usercopy.c"] Why couldn’t we do it without the “files” field? The presence of the tag in the file and the justification (I think) are enough. > }, > { > "id": "SAF-2-safe", > "analyser": {}, > "name": "Sentinel", > "text": "Next ID to be used" > } > ] > }
diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl index 2681a4cff5..a7d4f29b43 100644 --- a/automation/eclair_analysis/ECLAIR/deviations.ecl +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl @@ -96,6 +96,10 @@ conform to the directive." -config=MC3R1.D4.10,reports+={safe, "first_area(text(^ \\* In this case, no inclusion guards apply and the caller is responsible.*\\*/$, begin-1))"} -doc_end +-doc_begin="xen/arch/x86/usercopy.c includes itself: it is not supposed to comply with the directive" +-config=MC3R1.D4.10,reports+={deliberate, "all_area(all_loc(file("^xen/arch/x86/usercopy\\.c$")))"} +-doc_end + # # Series 5. # diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst index 4b1a7b02b6..45e13d0302 100644 --- a/docs/misra/rules.rst +++ b/docs/misra/rules.rst @@ -62,6 +62,8 @@ maintainers if you want to suggest a change. - Files that are intended to be included more than once do not need to conform to the directive. Files that explicitly avoid inclusion guards under specific circumstances do not need to conform the directive. + xen/arch/x86/usercopy.c includes itself: it is not supposed to comply + with the directive. * - `Dir 4.11 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_04_11.c>`_ - Required
xen/arch/x86/usercopy.c includes itself, so it is not supposed to comply with Directive 4.10: "Precautions shall be taken in order to prevent the contents of a header file being included more than once" This patch adds a deviation for the file. Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> --- automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++ docs/misra/rules.rst | 2 ++ 2 files changed, 6 insertions(+)