| Message ID | 20230819012410.1754839-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | docs/misra: add exceptions to rules | expand |
On 19.08.2023 03:24, Stefano Stabellini wrote: > From: Stefano Stabellini <stefano.stabellini@amd.com> > > During the discussions that led to the acceptable of the Rules, we > decided on a few exceptions that were not properly recorded in > rules.rst. Other times, the exceptions were decided later when it came > to enabling a rule in ECLAIR. In a number of cases I'm unaware of such decisions. May be worth splitting the patch into a controversial and an uncontroversial part, such that the latter can go in while we discuss the former. > --- a/docs/misra/rules.rst > +++ b/docs/misra/rules.rst > @@ -59,7 +59,8 @@ maintainers if you want to suggest a change. > - Required > - Precautions shall be taken in order to prevent the contents of a > header file being included more than once > - - > + - Files that are intended to be included more than once do not need to > + conform to the directive (e.g. autogenerated or empty header files) Auto-generated isn't a reason for an exception here. The logic generating the header can very well be adjusted. Same for empty headers - there's no reason they couldn't gain guards. An exception is needed for headers which we deliberately include more than once, in order to have a single central place for attributes, enumerations, and alike. > @@ -106,7 +107,23 @@ maintainers if you want to suggest a change. > * - `Rule 2.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_01_1.c>`_ > - Required > - A project shall not contain unreachable code > - - > + - The following are allowed: > + - Invariantly constant conditions (e.g. while(0) { S; }) When (and why) was this decided? The example given looks exactly like what Misra wants us to not have. > + - Switch with a controlling value incompatible with labeled > + statements What does this mean? > + - Functions that are intended to be never referenced from C > + code, or are referenced in builds not under analysis (e.g. > + 'do_trap_fiq' for the former and 'check_for_unexpected_msi' > + for the latter) I agree with the "not referenced from C", but I don't see why the other kind couldn't be properly addressed. > + - Unreachability caused by the following macros/functions is > + deliberate: BUG, assert_failed, ERROR_EXIT, ERROR_EXIT_DOM, > + PIN_FAIL, __builtin_unreachable, panic, do_unexpected_trap, > + machine_halt, machine_restart, machine_reboot, > + ASSERT_UNREACHABLE Base infrastructure items like BUG() are imo fine to mention here. But specific items shouldn't be; the more we mention here, the more we invite the list to be grown. Note also how you mention items which no longer exist (ERROR_EXIT{,_DOM}, PIN_FAIL). > + - asm-offsets.c, as they are not linked deliberately, because > + they are used to generate definitions for asm modules > + - pure declarations (i.e. declarations without > + initialization) are safe, as they are not executed I don't think "pure declarations" is a term used in the spec. Let's better call it the way it is called elsewhere - declarations without initializer (where, as mentioned before, the term "unreachable code" is questionable anyway). > @@ -167,7 +184,7 @@ maintainers if you want to suggest a change. > * - `Rule 5.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_ > - Required > - A typedef name shall be a unique identifier > - - > + - BOOLEAN, UINT{8,32,64} and INT{8,32,64} are allowed I think this permission needs to be limited as much as possible. > @@ -183,7 +200,10 @@ maintainers if you want to suggest a change. > * - `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 > - - > + - Usage of the following constants is safe, since they are given > + as-is in the inflate algorithm specification and there is > + therefore no risk of them being interpreted as decimal constants: > + ^0(007|37|070|213|236|300|321|330|331|332|333|334|335|337|371)$ This is a very odd set of exceptions, which by stating them here you then grant to be exercised everywhere. Once again I don't think special cases dealing with a single source or sub-component should be globally named. > @@ -239,13 +259,16 @@ maintainers if you want to suggest a change. > - Required > - All declarations of an object or function shall use the same > names and type qualifiers > - - > + - The type ret_t is deliberately used and defined as int or long > + depending on the architecture That's not depending on the architecture, but depending on the type of guest to service. I'd also suggest "may", not "is". Jan
On Mon, 21 Aug 2023, Jan Beulich wrote: > On 19.08.2023 03:24, Stefano Stabellini wrote: > > From: Stefano Stabellini <stefano.stabellini@amd.com> > > > > During the discussions that led to the acceptable of the Rules, we > > decided on a few exceptions that were not properly recorded in > > rules.rst. Other times, the exceptions were decided later when it came > > to enabling a rule in ECLAIR. > > In a number of cases I'm unaware of such decisions. May be worth splitting > the patch into a controversial and an uncontroversial part, such that the > latter can go in while we discuss the former. yes, I'll extract a couple of non-controversial changes and send them out > > --- a/docs/misra/rules.rst > > +++ b/docs/misra/rules.rst > > @@ -59,7 +59,8 @@ maintainers if you want to suggest a change. > > - Required > > - Precautions shall be taken in order to prevent the contents of a > > header file being included more than once > > - - > > + - Files that are intended to be included more than once do not need to > > + conform to the directive (e.g. autogenerated or empty header files) > > Auto-generated isn't a reason for an exception here. The logic generating > the header can very well be adjusted. Same for empty headers - there's no > reason they couldn't gain guards. An exception is needed for headers which > we deliberately include more than once, in order to have a single central > place for attributes, enumerations, and alike. OK > > @@ -106,7 +107,23 @@ maintainers if you want to suggest a change. > > * - `Rule 2.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_01_1.c>`_ > > - Required > > - A project shall not contain unreachable code > > - - > > + - The following are allowed: > > + - Invariantly constant conditions (e.g. while(0) { S; }) > > When (and why) was this decided? The example given looks exactly like what > Misra wants us to not have. This covers things like: if (IS_ENABLED(CONFIG_HVM)) which could resolve in if (0) in certain configurations. I think we gave feedback to Roberto that we wanted to keep this type of things as is. > > + - Switch with a controlling value incompatible with labeled > > + statements > > What does this mean? I am not certain about this one actually. It could be when we have: switch (var) { case 1: something(); break; case 2: something(); break; } and var could be 3 in theory? Nicola, please confirm. > > + - Functions that are intended to be never referenced from C > > + code, or are referenced in builds not under analysis (e.g. > > + 'do_trap_fiq' for the former and 'check_for_unexpected_msi' > > + for the latter) > > I agree with the "not referenced from C", but I don't see why the other > kind couldn't be properly addressed. I think you have a point > > + - Unreachability caused by the following macros/functions is > > + deliberate: BUG, assert_failed, ERROR_EXIT, ERROR_EXIT_DOM, > > + PIN_FAIL, __builtin_unreachable, panic, do_unexpected_trap, > > + machine_halt, machine_restart, machine_reboot, > > + ASSERT_UNREACHABLE > > Base infrastructure items like BUG() are imo fine to mention here. But > specific items shouldn't be; the more we mention here, the more we invite > the list to be grown. Note also how you mention items which no longer > exist (ERROR_EXIT{,_DOM}, PIN_FAIL). The question is whether we want this list to be exhaustive (so we want to mention everything for which we make an exception) or only an example (in which case just BUG is fine.) Let's say we only mention BUG. Where should we keep the exhaustive list? Is it OK if it only lives as an ECLAIR config file under automation/eclair_analysis? There is another very similar question below. BTW I think both options are OK. If we only mention BUG, we are basically saying that as a general rule only BUG is an exception. Then we have a longer more detailed list for ECLAIR because in practice things are always complicated. On the other hand if we have the full list here, then the documentation is more precise, but it looks a bit "strange" to see such a detailed list in this document and also we need to make sure to keep the list up-to-date. > > + - asm-offsets.c, as they are not linked deliberately, because > > + they are used to generate definitions for asm modules > > + - pure declarations (i.e. declarations without > > + initialization) are safe, as they are not executed > > I don't think "pure declarations" is a term used in the spec. Let's better > call it the way it is called elsewhere - declarations without initializer > (where, as mentioned before, the term "unreachable code" is questionable > anyway). OK > > @@ -167,7 +184,7 @@ maintainers if you want to suggest a change. > > * - `Rule 5.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_ > > - Required > > - A typedef name shall be a unique identifier > > - - > > + - BOOLEAN, UINT{8,32,64} and INT{8,32,64} are allowed > > I think this permission needs to be limited as much as possible. Maybe we should take this out completely given that they should be limited to efi and acpi code that is excepted anyway > > @@ -183,7 +200,10 @@ maintainers if you want to suggest a change. > > * - `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 > > - - > > + - Usage of the following constants is safe, since they are given > > + as-is in the inflate algorithm specification and there is > > + therefore no risk of them being interpreted as decimal constants: > > + ^0(007|37|070|213|236|300|321|330|331|332|333|334|335|337|371)$ > > This is a very odd set of exceptions, which by stating them here you then > grant to be exercised everywhere. Once again I don't think special cases > dealing with a single source or sub-component should be globally named. Actually I agree with you there. The problem is where to put them. Right now we have docs/misra/rules.rst with the list of accepted rules and their special interpretations by Xen Project. We also have the ECLAIR configuration under automation/eclair_analysis with a bunch of ECLAIR specific config files. Is it OK if the constants above only live under automation/eclair_analysis and nowhere else? Or should we have another rst document under docs/misra for special cases dealing with a single source? > > @@ -239,13 +259,16 @@ maintainers if you want to suggest a change. > > - Required > > - All declarations of an object or function shall use the same > > names and type qualifiers > > - - > > + - The type ret_t is deliberately used and defined as int or long > > + depending on the architecture > > That's not depending on the architecture, but depending on the type of > guest to service. I'd also suggest "may", not "is". OK
On 22.08.2023 03:40, Stefano Stabellini wrote: > On Mon, 21 Aug 2023, Jan Beulich wrote: >> On 19.08.2023 03:24, Stefano Stabellini wrote: >>> @@ -106,7 +107,23 @@ maintainers if you want to suggest a change. >>> * - `Rule 2.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_01_1.c>`_ >>> - Required >>> - A project shall not contain unreachable code >>> - - >>> + - The following are allowed: >>> + - Invariantly constant conditions (e.g. while(0) { S; }) >> >> When (and why) was this decided? The example given looks exactly like what >> Misra wants us to not have. > > This covers things like: > > if (IS_ENABLED(CONFIG_HVM)) > > which could resolve in if (0) in certain configurations. I think we gave > feedback to Roberto that we wanted to keep this type of things as is. Ah, I see. But then perhaps mention that rather than plain 0 here? See below as to whether a complete list of excepted constructs is wanted. >>> + - Switch with a controlling value incompatible with labeled >>> + statements >> >> What does this mean? > > I am not certain about this one actually. It could be when we have: > > switch (var) { > case 1: > something(); > break; > case 2: > something(); > break; > } > > and var could be 3 in theory? That would be a unhandled value, but no unreachable code. >>> + - Unreachability caused by the following macros/functions is >>> + deliberate: BUG, assert_failed, ERROR_EXIT, ERROR_EXIT_DOM, >>> + PIN_FAIL, __builtin_unreachable, panic, do_unexpected_trap, >>> + machine_halt, machine_restart, machine_reboot, >>> + ASSERT_UNREACHABLE >> >> Base infrastructure items like BUG() are imo fine to mention here. But >> specific items shouldn't be; the more we mention here, the more we invite >> the list to be grown. Note also how you mention items which no longer >> exist (ERROR_EXIT{,_DOM}, PIN_FAIL). > > The question is whether we want this list to be exhaustive (so we want > to mention everything for which we make an exception) or only an example > (in which case just BUG is fine.) > > Let's say we only mention BUG. Where should we keep the exhaustive list? > Is it OK if it only lives as an ECLAIR config file under > automation/eclair_analysis? There is another very similar question > below. First and foremost we need to have a single place where everything is recorded, or where at least a pointer exists to where further details are. As to this being the Eclair config file: Shouldn't any such constructs rather be listed in the deviations file, such that e.g. cppcheck can also benefit? > BTW I think both options are OK. > > If we only mention BUG, we are basically saying that as a general rule > only BUG is an exception. Then we have a longer more detailed list for > ECLAIR because in practice things are always complicated. > > On the other hand if we have the full list here, then the documentation > is more precise, but it looks a bit "strange" to see such a detailed > list in this document and also we need to make sure to keep the list > up-to-date. Thing is: This list shouldn't grow very long anyway, and also better would grow / change much over time. >>> @@ -167,7 +184,7 @@ maintainers if you want to suggest a change. >>> * - `Rule 5.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_ >>> - Required >>> - A typedef name shall be a unique identifier >>> - - >>> + - BOOLEAN, UINT{8,32,64} and INT{8,32,64} are allowed >> >> I think this permission needs to be limited as much as possible. > > Maybe we should take this out completely given that they should be > limited to efi and acpi code that is excepted anyway I would favor that, yes. >>> @@ -183,7 +200,10 @@ maintainers if you want to suggest a change. >>> * - `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 >>> - - >>> + - Usage of the following constants is safe, since they are given >>> + as-is in the inflate algorithm specification and there is >>> + therefore no risk of them being interpreted as decimal constants: >>> + ^0(007|37|070|213|236|300|321|330|331|332|333|334|335|337|371)$ >> >> This is a very odd set of exceptions, which by stating them here you then >> grant to be exercised everywhere. Once again I don't think special cases >> dealing with a single source or sub-component should be globally named. > > Actually I agree with you there. The problem is where to put them. safe.json? > Right now we have docs/misra/rules.rst with the list of accepted rules > and their special interpretations by Xen Project. We also have the > ECLAIR configuration under automation/eclair_analysis with a bunch of > ECLAIR specific config files. > > Is it OK if the constants above only live under > automation/eclair_analysis and nowhere else? Or should we have another > rst document under docs/misra for special cases dealing with a single > source? As per above, I think putting anything in Eclair's config file should only ever be a last resort. Wherever possible we should try to put stuff in files which aren't tool specific. Where necessary special tool settings can then be (machine-)derived from there. Jan
(re-adding xen-devel@) On 22.08.2023 17:09, Nicola Vetrini wrote: > >>>> + - Switch with a controlling value incompatible with labeled >>>> + statements >>> >>> What does this mean? >> >> I am not certain about this one actually. It could be when we have: >> >> switch (var) { >> case 1: >> something(); >> break; >> case 2: >> something(); >> break; >> } >> >> and var could be 3 in theory? >> >> Nicola, please confirm. >> >> > > This one is about case labels that are statically determined not to be > reachable (and hence > saying that the code under that label is unreachable is not inaccurate) > because the > controlling expression of the switch statement can never have such > value. An example below: > > $ cat p.c > int f(void) { > char c; > switch (c) { > case 260: > return 260; > case 4: > return 4; > } > } > > $ eclair_env -enable=MC3.R2.1,B.REPORT.TXT -- gcc -c p.c > violation for rule MC3.R2.1: (required) A project shall not contain > unreachable code. (untagged) > p.c:3.3-3.8: Loc #1 [culprit: `switch' statement has a controlling value > incompatible with labeled statement] > switch (c) { > <~~~~> > p.c:5.14-5.16: Loc #2 [evidence: integer literal is unreachable] > return 260; > <~> > > This is also true for things like > > switch(sizeof(int)) { > case 2: > ... > case 4: > ... > } Ah yes, we certainly have quite a few of those. Not sure how to best describe such for the doc, but what was suggested (still visible at the top) doesn't get this across, I'm afraid, Jan
On Tue, 22 Aug 2023, Jan Beulich wrote: > (re-adding xen-devel@) > > On 22.08.2023 17:09, Nicola Vetrini wrote: > > > >>>> + - Switch with a controlling value incompatible with labeled > >>>> + statements > >>> > >>> What does this mean? > >> > >> I am not certain about this one actually. It could be when we have: > >> > >> switch (var) { > >> case 1: > >> something(); > >> break; > >> case 2: > >> something(); > >> break; > >> } > >> > >> and var could be 3 in theory? > >> > >> Nicola, please confirm. > >> > >> > > > > This one is about case labels that are statically determined not to be > > reachable (and hence > > saying that the code under that label is unreachable is not inaccurate) > > because the > > controlling expression of the switch statement can never have such > > value. An example below: > > > > $ cat p.c > > int f(void) { > > char c; > > switch (c) { > > case 260: > > return 260; > > case 4: > > return 4; > > } > > } > > > > $ eclair_env -enable=MC3.R2.1,B.REPORT.TXT -- gcc -c p.c > > violation for rule MC3.R2.1: (required) A project shall not contain > > unreachable code. (untagged) > > p.c:3.3-3.8: Loc #1 [culprit: `switch' statement has a controlling value > > incompatible with labeled statement] > > switch (c) { > > <~~~~> > > p.c:5.14-5.16: Loc #2 [evidence: integer literal is unreachable] > > return 260; > > <~> > > > > This is also true for things like > > > > switch(sizeof(int)) { > > case 2: > > ... > > case 4: > > ... > > } > > Ah yes, we certainly have quite a few of those. Not sure how to best > describe such for the doc, but what was suggested (still visible at > the top) doesn't get this across, I'm afraid, What about: "switch with a controlling value statically determined not to match one or more case statements" I'll send it as part of a separate new patch to update rules.rst for 2.1
On Tue, 22 Aug 2023, Jan Beulich wrote: > On 22.08.2023 03:40, Stefano Stabellini wrote: > > On Mon, 21 Aug 2023, Jan Beulich wrote: > >> On 19.08.2023 03:24, Stefano Stabellini wrote: > >>> @@ -106,7 +107,23 @@ maintainers if you want to suggest a change. > >>> * - `Rule 2.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_01_1.c>`_ > >>> - Required > >>> - A project shall not contain unreachable code > >>> - - > >>> + - The following are allowed: > >>> + - Invariantly constant conditions (e.g. while(0) { S; }) > >> > >> When (and why) was this decided? The example given looks exactly like what > >> Misra wants us to not have. > > > > This covers things like: > > > > if (IS_ENABLED(CONFIG_HVM)) > > > > which could resolve in if (0) in certain configurations. I think we gave > > feedback to Roberto that we wanted to keep this type of things as is. > > Ah, I see. But then perhaps mention that rather than plain 0 here? Yes, I'll do > See below as to whether a complete list of excepted constructs is > wanted. [...] > >>> + - Unreachability caused by the following macros/functions is > >>> + deliberate: BUG, assert_failed, ERROR_EXIT, ERROR_EXIT_DOM, > >>> + PIN_FAIL, __builtin_unreachable, panic, do_unexpected_trap, > >>> + machine_halt, machine_restart, machine_reboot, > >>> + ASSERT_UNREACHABLE > >> > >> Base infrastructure items like BUG() are imo fine to mention here. But > >> specific items shouldn't be; the more we mention here, the more we invite > >> the list to be grown. Note also how you mention items which no longer > >> exist (ERROR_EXIT{,_DOM}, PIN_FAIL). > > > > The question is whether we want this list to be exhaustive (so we want > > to mention everything for which we make an exception) or only an example > > (in which case just BUG is fine.) > > > > Let's say we only mention BUG. Where should we keep the exhaustive list? > > Is it OK if it only lives as an ECLAIR config file under > > automation/eclair_analysis? There is another very similar question > > below. > > First and foremost we need to have a single place where everything is > recorded, or where at least a pointer exists to where further details > are. As to this being the Eclair config file: Shouldn't any such > constructs rather be listed in the deviations file, such that e.g. > cppcheck can also benefit? Yes, you are right. Basically the choice is whether we want a project-wide deviation, something like a change in how we interpret the rules, or a regular deviation for one macro or one function. After reading your comments, I also think that all the macros above should be covered by safe.json and in-code comments instead. > > BTW I think both options are OK. > > > > If we only mention BUG, we are basically saying that as a general rule > > only BUG is an exception. Then we have a longer more detailed list for > > ECLAIR because in practice things are always complicated. > > > > On the other hand if we have the full list here, then the documentation > > is more precise, but it looks a bit "strange" to see such a detailed > > list in this document and also we need to make sure to keep the list > > up-to-date. > > Thing is: This list shouldn't grow very long anyway, and also better > would grow / change much over time. +1 > >>> @@ -167,7 +184,7 @@ maintainers if you want to suggest a change. > >>> * - `Rule 5.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_ > >>> - Required > >>> - A typedef name shall be a unique identifier > >>> - - > >>> + - BOOLEAN, UINT{8,32,64} and INT{8,32,64} are allowed > >> > >> I think this permission needs to be limited as much as possible. > > > > Maybe we should take this out completely given that they should be > > limited to efi and acpi code that is excepted anyway > > I would favor that, yes. > > >>> @@ -183,7 +200,10 @@ maintainers if you want to suggest a change. > >>> * - `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 > >>> - - > >>> + - Usage of the following constants is safe, since they are given > >>> + as-is in the inflate algorithm specification and there is > >>> + therefore no risk of them being interpreted as decimal constants: > >>> + ^0(007|37|070|213|236|300|321|330|331|332|333|334|335|337|371)$ > >> > >> This is a very odd set of exceptions, which by stating them here you then > >> grant to be exercised everywhere. Once again I don't think special cases > >> dealing with a single source or sub-component should be globally named. > > > > Actually I agree with you there. The problem is where to put them. > > safe.json? Yes you are right > > Right now we have docs/misra/rules.rst with the list of accepted rules > > and their special interpretations by Xen Project. We also have the > > ECLAIR configuration under automation/eclair_analysis with a bunch of > > ECLAIR specific config files. > > > > Is it OK if the constants above only live under > > automation/eclair_analysis and nowhere else? Or should we have another > > rst document under docs/misra for special cases dealing with a single > > source? > > As per above, I think putting anything in Eclair's config file should > only ever be a last resort. Wherever possible we should try to put stuff > in files which aren't tool specific. Where necessary special tool > settings can then be (machine-)derived from there. I agree. I'll send out another patch to update rules.rst. And later as a follow-up I'll see what I can do to update safe.json.
On 23.08.2023 02:19, Stefano Stabellini wrote: > On Tue, 22 Aug 2023, Jan Beulich wrote: >> (re-adding xen-devel@) >> >> On 22.08.2023 17:09, Nicola Vetrini wrote: >>> >>>>>> + - Switch with a controlling value incompatible with labeled >>>>>> + statements >>>>> >>>>> What does this mean? >>>> >>>> I am not certain about this one actually. It could be when we have: >>>> >>>> switch (var) { >>>> case 1: >>>> something(); >>>> break; >>>> case 2: >>>> something(); >>>> break; >>>> } >>>> >>>> and var could be 3 in theory? >>>> >>>> Nicola, please confirm. >>>> >>>> >>> >>> This one is about case labels that are statically determined not to be >>> reachable (and hence >>> saying that the code under that label is unreachable is not inaccurate) >>> because the >>> controlling expression of the switch statement can never have such >>> value. An example below: >>> >>> $ cat p.c >>> int f(void) { >>> char c; >>> switch (c) { >>> case 260: >>> return 260; >>> case 4: >>> return 4; >>> } >>> } >>> >>> $ eclair_env -enable=MC3.R2.1,B.REPORT.TXT -- gcc -c p.c >>> violation for rule MC3.R2.1: (required) A project shall not contain >>> unreachable code. (untagged) >>> p.c:3.3-3.8: Loc #1 [culprit: `switch' statement has a controlling value >>> incompatible with labeled statement] >>> switch (c) { >>> <~~~~> >>> p.c:5.14-5.16: Loc #2 [evidence: integer literal is unreachable] >>> return 260; >>> <~> >>> >>> This is also true for things like >>> >>> switch(sizeof(int)) { >>> case 2: >>> ... >>> case 4: >>> ... >>> } >> >> Ah yes, we certainly have quite a few of those. Not sure how to best >> describe such for the doc, but what was suggested (still visible at >> the top) doesn't get this across, I'm afraid, > > What about: "switch with a controlling value statically determined not > to match one or more case statements" Sounds okay, assuming this is the only case we want to treat specially. Jan
On 23.08.2023 02:23, Stefano Stabellini wrote: > On Tue, 22 Aug 2023, Jan Beulich wrote: >> On 22.08.2023 03:40, Stefano Stabellini wrote: >>> If we only mention BUG, we are basically saying that as a general rule >>> only BUG is an exception. Then we have a longer more detailed list for >>> ECLAIR because in practice things are always complicated. >>> >>> On the other hand if we have the full list here, then the documentation >>> is more precise, but it looks a bit "strange" to see such a detailed >>> list in this document and also we need to make sure to keep the list >>> up-to-date. >> >> Thing is: This list shouldn't grow very long anyway, and also better >> would grow / change much over time. > > +1 I assume you read what was meant (wouldn't), not what was written (would). Jan
diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst index 8f0e4d3f25..ecbb04da96 100644 --- a/docs/misra/rules.rst +++ b/docs/misra/rules.rst @@ -59,7 +59,8 @@ maintainers if you want to suggest a change. - Required - Precautions shall be taken in order to prevent the contents of a header file being included more than once - - + - Files that are intended to be included more than once do not need to + conform to the directive (e.g. autogenerated or empty header files) * - `Dir 4.11 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_04_11.c>`_ - Required @@ -106,7 +107,23 @@ maintainers if you want to suggest a change. * - `Rule 2.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_01_1.c>`_ - Required - A project shall not contain unreachable code - - + - The following are allowed: + - Invariantly constant conditions (e.g. while(0) { S; }) + - Switch with a controlling value incompatible with labeled + statements + - Functions that are intended to be never referenced from C + code, or are referenced in builds not under analysis (e.g. + 'do_trap_fiq' for the former and 'check_for_unexpected_msi' + for the latter) + - Unreachability caused by the following macros/functions is + deliberate: BUG, assert_failed, ERROR_EXIT, ERROR_EXIT_DOM, + PIN_FAIL, __builtin_unreachable, panic, do_unexpected_trap, + machine_halt, machine_restart, machine_reboot, + ASSERT_UNREACHABLE + - asm-offsets.c, as they are not linked deliberately, because + they are used to generate definitions for asm modules + - pure declarations (i.e. declarations without + initialization) are safe, as they are not executed * - `Rule 2.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_02_06.c>`_ - Advisory @@ -117,7 +134,7 @@ maintainers if you want to suggest a change. - Required - The character sequences /* and // shall not be used within a comment - - + - Comments containing hyperlinks inside C-style block comments are safe * - `Rule 3.2 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_03_02.c>`_ - Required @@ -167,7 +184,7 @@ maintainers if you want to suggest a change. * - `Rule 5.6 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_05_06.c>`_ - Required - A typedef name shall be a unique identifier - - + - BOOLEAN, UINT{8,32,64} and INT{8,32,64} are allowed * - `Rule 6.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_06_01.c>`_ - Required @@ -183,7 +200,10 @@ maintainers if you want to suggest a change. * - `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 - - + - Usage of the following constants is safe, since they are given + as-is in the inflate algorithm specification and there is + therefore no risk of them being interpreted as decimal constants: + ^0(007|37|070|213|236|300|321|330|331|332|333|334|335|337|371)$ * - `Rule 7.2 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_07_02.c>`_ - Required @@ -239,13 +259,16 @@ maintainers if you want to suggest a change. - Required - All declarations of an object or function shall use the same names and type qualifiers - - + - The type ret_t is deliberately used and defined as int or long + depending on the architecture * - `Rule 8.4 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_08_04.c>`_ - Required - A compatible declaration shall be visible when an object or function with external linkage is defined - - + - Allowed exceptions: asm-offsets.c (definitions for asm modules + not called from C code), gcov_base.c (definitions only used in + non-release builds) * - `Rule 8.5 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_08_05_2.c>`_ - Required @@ -369,7 +392,9 @@ maintainers if you want to suggest a change. - Required - Expressions resulting from the expansion of macro parameters shall be enclosed in parentheses - - + - Extra parentheses are not required when macro parameters are used + as function arguments, as macro arguments, array indices, lhs in + assignments * - `Rule 20.13 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_20_13.c>`_ - Required