| Message ID | 6c96f889ae3fbdf516f0edfe4a5fb207540b6862.1709109662.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [XEN] automation/eclair: extend deviations of MISRA C:2012 Rule 16.3 | expand |
On 28.02.2024 09:53, Federico Serafini wrote: > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl Comments below apply similarly to text added to this file. > --- a/docs/misra/deviations.rst > +++ b/docs/misra/deviations.rst > @@ -291,7 +291,14 @@ Deviations related to MISRA C:2012 Rules: > - Project-wide deviation; tagged as `deliberate` for ECLAIR. > > * - R16.3 > - - Switch clauses ending with continue, goto, return statements are safe. > + - Switch clauses ending with an unconditional flow control statement > + (i.e., continue, goto, or return) are safe. > + - Tagged as `safe` for ECLAIR. With this edit (unmentioned in the description, btw) ... > + * - R16.3 > + - Switch clauses ending with an if-else statement are safe if both > + branches consist of a flow control statement (i.e., continue, break, > + goto, return). ... why is it not also "ending with" here? Also what about either situation ending with a call to a noreturn function? > @@ -307,6 +314,16 @@ Deviations related to MISRA C:2012 Rules: > - Switch clauses ending with failure method \"BUG()\" are safe. > - Tagged as `safe` for ECLAIR. > > + * - R16.3 > + - On X86, switch clauses ending generating an exception through > + \"generate_exception()\" are safe. > + - Tagged as `safe` for ECLAIR. This macro is limited to the emulator, so shouldn't be deviated globally. Furthermore - why does the special case need mentioning here? Shouldn't it be the underlying pattern which is deviated (along the lines of the earlier ones): if ( true ) { ... goto ...; /* Or break / continue / return */ } > + * - R16.3 > + - Switch clauses ending generating a parse error through > + \"PARSE_ERR_RET()\" are safe. > + - Tagged as `safe` for ECLAIR. Again this isn't a global scope macro, so shouldn't be deviated globally. Plus it ends in "return", so ought to be covered by the earlier clause. The fact that the return is in a body of do {} while(0) shouldn't matter at all - that's purely syntactic sugar. Jan
On 28/02/24 10:06, Jan Beulich wrote: > On 28.02.2024 09:53, Federico Serafini wrote: >> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl > > Comments below apply similarly to text added to this file. > >> --- a/docs/misra/deviations.rst >> +++ b/docs/misra/deviations.rst >> @@ -291,7 +291,14 @@ Deviations related to MISRA C:2012 Rules: >> - Project-wide deviation; tagged as `deliberate` for ECLAIR. >> >> * - R16.3 >> - - Switch clauses ending with continue, goto, return statements are safe. >> + - Switch clauses ending with an unconditional flow control statement >> + (i.e., continue, goto, or return) are safe. >> + - Tagged as `safe` for ECLAIR. > > With this edit (unmentioned in the description, btw) ... > >> + * - R16.3 >> + - Switch clauses ending with an if-else statement are safe if both >> + branches consist of a flow control statement (i.e., continue, break, >> + goto, return). > > ... why is it not also "ending with" here? Because the allowed pattern is: if ( cond ) return; /* Or continue / break / goto */ else break; /* Or continue / goto / return */ See below for more information. > > Also what about either situation ending with a call to a noreturn function? This can be added. > >> @@ -307,6 +314,16 @@ Deviations related to MISRA C:2012 Rules: >> - Switch clauses ending with failure method \"BUG()\" are safe. >> - Tagged as `safe` for ECLAIR. >> >> + * - R16.3 >> + - On X86, switch clauses ending generating an exception through >> + \"generate_exception()\" are safe. >> + - Tagged as `safe` for ECLAIR. > > This macro is limited to the emulator, so shouldn't be deviated globally. Noted. > Furthermore - why does the special case need mentioning here? Shouldn't > it be the underlying pattern which is deviated (along the lines of the > earlier ones): > > if ( true ) > { > ... > goto ...; /* Or break / continue / return */ > } This pattern that involves a compound statement for the true branch is not deviated by this configuration. See below for more information. > >> + * - R16.3 >> + - Switch clauses ending generating a parse error through >> + \"PARSE_ERR_RET()\" are safe. >> + - Tagged as `safe` for ECLAIR. > > Again this isn't a global scope macro, so shouldn't be deviated globally. Noted. > Plus it ends in "return", so ought to be covered by the earlier clause. > The fact that the return is in a body of do {} while(0) shouldn't matter > at all - that's purely syntactic sugar. I gather from your comments/questions that you would like to deviate *all* the patterns where an unintentional fall through can not happen. Rule 16.3 is a purely syntactic rule, and, as a consequence, in the current version of ECLAIR additional "allowed pattern" (aka deviations) for that rule need to be described through AST nodes, meaning that all what you consider as syntactic sugar cannot be ignored. A deviation that covers all the pattern you are asking for could be done, but it will result in a complex and quite long expression (not easy to read and justify in front of an assessor). Hence, what I am proposing is to deviate only the the simplest and most readable cases, such as: if ( cond ) return x; else return y; without involving compound statements, fake do-wile and fake if statements but rather deviating the macro inside of which are used (as I did).
On 29.02.2024 09:01, Federico Serafini wrote: > On 28/02/24 10:06, Jan Beulich wrote: >> On 28.02.2024 09:53, Federico Serafini wrote: >>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >> >> Comments below apply similarly to text added to this file. >> >>> --- a/docs/misra/deviations.rst >>> +++ b/docs/misra/deviations.rst >>> @@ -291,7 +291,14 @@ Deviations related to MISRA C:2012 Rules: >>> - Project-wide deviation; tagged as `deliberate` for ECLAIR. >>> >>> * - R16.3 >>> - - Switch clauses ending with continue, goto, return statements are safe. >>> + - Switch clauses ending with an unconditional flow control statement >>> + (i.e., continue, goto, or return) are safe. >>> + - Tagged as `safe` for ECLAIR. >> >> With this edit (unmentioned in the description, btw) ... >> >>> + * - R16.3 >>> + - Switch clauses ending with an if-else statement are safe if both >>> + branches consist of a flow control statement (i.e., continue, break, >>> + goto, return). >> >> ... why is it not also "ending with" here? > > Because the allowed pattern is: > > if ( cond ) > return; /* Or continue / break / goto */ > else > break; /* Or continue / goto / return */ > > See below for more information. > >> >> Also what about either situation ending with a call to a noreturn function? > > This can be added. > >> >>> @@ -307,6 +314,16 @@ Deviations related to MISRA C:2012 Rules: >>> - Switch clauses ending with failure method \"BUG()\" are safe. >>> - Tagged as `safe` for ECLAIR. >>> >>> + * - R16.3 >>> + - On X86, switch clauses ending generating an exception through >>> + \"generate_exception()\" are safe. >>> + - Tagged as `safe` for ECLAIR. >> >> This macro is limited to the emulator, so shouldn't be deviated globally. > > Noted. > >> Furthermore - why does the special case need mentioning here? Shouldn't >> it be the underlying pattern which is deviated (along the lines of the >> earlier ones): >> >> if ( true ) >> { >> ... >> goto ...; /* Or break / continue / return */ >> } > > This pattern that involves a compound statement for the true branch > is not deviated by this configuration. > > See below for more information. > >> >>> + * - R16.3 >>> + - Switch clauses ending generating a parse error through >>> + \"PARSE_ERR_RET()\" are safe. >>> + - Tagged as `safe` for ECLAIR. >> >> Again this isn't a global scope macro, so shouldn't be deviated globally. > > Noted. > >> Plus it ends in "return", so ought to be covered by the earlier clause. >> The fact that the return is in a body of do {} while(0) shouldn't matter >> at all - that's purely syntactic sugar. > > I gather from your comments/questions that you would like to deviate > *all* the patterns where an unintentional fall through can not happen. > > Rule 16.3 is a purely syntactic rule, and, as a consequence, > in the current version of ECLAIR additional "allowed pattern" (aka > deviations) for that rule need to be described through AST nodes, > meaning that all what you consider as syntactic sugar cannot be ignored. > > A deviation that covers all the pattern you are asking for could be > done, but it will result in a complex and quite long expression > (not easy to read and justify in front of an assessor). > > Hence, what I am proposing is to deviate only the the simplest and > most readable cases, such as: > > if ( cond ) > return x; > else > return y; > > without involving compound statements, fake do-wile and fake if > statements but rather deviating the macro inside of which are used > (as I did). I see. Problem is that this isn't sufficient for the code we have, and the seemingly random deviation of certain constructs by name looks to me as pretty undesirable. Jan
On Thu, 29 Feb 2024, Jan Beulich wrote: > On 29.02.2024 09:01, Federico Serafini wrote: > > On 28/02/24 10:06, Jan Beulich wrote: > >> On 28.02.2024 09:53, Federico Serafini wrote: > >>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl > >>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl > >> > >> Comments below apply similarly to text added to this file. > >> > >>> --- a/docs/misra/deviations.rst > >>> +++ b/docs/misra/deviations.rst > >>> @@ -291,7 +291,14 @@ Deviations related to MISRA C:2012 Rules: > >>> - Project-wide deviation; tagged as `deliberate` for ECLAIR. > >>> > >>> * - R16.3 > >>> - - Switch clauses ending with continue, goto, return statements are safe. > >>> + - Switch clauses ending with an unconditional flow control statement > >>> + (i.e., continue, goto, or return) are safe. > >>> + - Tagged as `safe` for ECLAIR. > >> > >> With this edit (unmentioned in the description, btw) ... > >> > >>> + * - R16.3 > >>> + - Switch clauses ending with an if-else statement are safe if both > >>> + branches consist of a flow control statement (i.e., continue, break, > >>> + goto, return). > >> > >> ... why is it not also "ending with" here? > > > > Because the allowed pattern is: > > > > if ( cond ) > > return; /* Or continue / break / goto */ > > else > > break; /* Or continue / goto / return */ > > > > See below for more information. > > > >> > >> Also what about either situation ending with a call to a noreturn function? > > > > This can be added. > > > >> > >>> @@ -307,6 +314,16 @@ Deviations related to MISRA C:2012 Rules: > >>> - Switch clauses ending with failure method \"BUG()\" are safe. > >>> - Tagged as `safe` for ECLAIR. > >>> > >>> + * - R16.3 > >>> + - On X86, switch clauses ending generating an exception through > >>> + \"generate_exception()\" are safe. > >>> + - Tagged as `safe` for ECLAIR. > >> > >> This macro is limited to the emulator, so shouldn't be deviated globally. > > > > Noted. > > > >> Furthermore - why does the special case need mentioning here? Shouldn't > >> it be the underlying pattern which is deviated (along the lines of the > >> earlier ones): > >> > >> if ( true ) > >> { > >> ... > >> goto ...; /* Or break / continue / return */ > >> } > > > > This pattern that involves a compound statement for the true branch > > is not deviated by this configuration. > > > > See below for more information. > > > >> > >>> + * - R16.3 > >>> + - Switch clauses ending generating a parse error through > >>> + \"PARSE_ERR_RET()\" are safe. > >>> + - Tagged as `safe` for ECLAIR. > >> > >> Again this isn't a global scope macro, so shouldn't be deviated globally. > > > > Noted. > > > >> Plus it ends in "return", so ought to be covered by the earlier clause. > >> The fact that the return is in a body of do {} while(0) shouldn't matter > >> at all - that's purely syntactic sugar. > > > > I gather from your comments/questions that you would like to deviate > > *all* the patterns where an unintentional fall through can not happen. > > > > Rule 16.3 is a purely syntactic rule, and, as a consequence, > > in the current version of ECLAIR additional "allowed pattern" (aka > > deviations) for that rule need to be described through AST nodes, > > meaning that all what you consider as syntactic sugar cannot be ignored. > > > > A deviation that covers all the pattern you are asking for could be > > done, but it will result in a complex and quite long expression > > (not easy to read and justify in front of an assessor). > > > > Hence, what I am proposing is to deviate only the the simplest and > > most readable cases, such as: > > > > if ( cond ) > > return x; > > else > > return y; > > > > without involving compound statements, fake do-wile and fake if > > statements but rather deviating the macro inside of which are used > > (as I did). > > I see. Problem is that this isn't sufficient for the code we have, and > the seemingly random deviation of certain constructs by name looks to > me as pretty undesirable. Yeah, I also think it is not ideal. At the same time among all options, it is probably the best way forward (better than in-code comments or better than leaving the violations outstanding). I think we should go for it.
On 01/03/24 00:00, Stefano Stabellini wrote: > On Thu, 29 Feb 2024, Jan Beulich wrote: >> On 29.02.2024 09:01, Federico Serafini wrote: >>> On 28/02/24 10:06, Jan Beulich wrote: >>>> On 28.02.2024 09:53, Federico Serafini wrote: >>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >>>> >>>> Comments below apply similarly to text added to this file. >>>> >>>>> --- a/docs/misra/deviations.rst >>>>> +++ b/docs/misra/deviations.rst >>>>> @@ -291,7 +291,14 @@ Deviations related to MISRA C:2012 Rules: >>>>> - Project-wide deviation; tagged as `deliberate` for ECLAIR. >>>>> >>>>> * - R16.3 >>>>> - - Switch clauses ending with continue, goto, return statements are safe. >>>>> + - Switch clauses ending with an unconditional flow control statement >>>>> + (i.e., continue, goto, or return) are safe. >>>>> + - Tagged as `safe` for ECLAIR. >>>> >>>> With this edit (unmentioned in the description, btw) ... >>>> >>>>> + * - R16.3 >>>>> + - Switch clauses ending with an if-else statement are safe if both >>>>> + branches consist of a flow control statement (i.e., continue, break, >>>>> + goto, return). >>>> >>>> ... why is it not also "ending with" here? >>> >>> Because the allowed pattern is: >>> >>> if ( cond ) >>> return; /* Or continue / break / goto */ >>> else >>> break; /* Or continue / goto / return */ >>> >>> See below for more information. >>> >>>> >>>> Also what about either situation ending with a call to a noreturn function? >>> >>> This can be added. >>> >>>> >>>>> @@ -307,6 +314,16 @@ Deviations related to MISRA C:2012 Rules: >>>>> - Switch clauses ending with failure method \"BUG()\" are safe. >>>>> - Tagged as `safe` for ECLAIR. >>>>> >>>>> + * - R16.3 >>>>> + - On X86, switch clauses ending generating an exception through >>>>> + \"generate_exception()\" are safe. >>>>> + - Tagged as `safe` for ECLAIR. >>>> >>>> This macro is limited to the emulator, so shouldn't be deviated globally. >>> >>> Noted. >>> >>>> Furthermore - why does the special case need mentioning here? Shouldn't >>>> it be the underlying pattern which is deviated (along the lines of the >>>> earlier ones): >>>> >>>> if ( true ) >>>> { >>>> ... >>>> goto ...; /* Or break / continue / return */ >>>> } >>> >>> This pattern that involves a compound statement for the true branch >>> is not deviated by this configuration. >>> >>> See below for more information. >>> >>>> >>>>> + * - R16.3 >>>>> + - Switch clauses ending generating a parse error through >>>>> + \"PARSE_ERR_RET()\" are safe. >>>>> + - Tagged as `safe` for ECLAIR. >>>> >>>> Again this isn't a global scope macro, so shouldn't be deviated globally. >>> >>> Noted. >>> >>>> Plus it ends in "return", so ought to be covered by the earlier clause. >>>> The fact that the return is in a body of do {} while(0) shouldn't matter >>>> at all - that's purely syntactic sugar. >>> >>> I gather from your comments/questions that you would like to deviate >>> *all* the patterns where an unintentional fall through can not happen. >>> >>> Rule 16.3 is a purely syntactic rule, and, as a consequence, >>> in the current version of ECLAIR additional "allowed pattern" (aka >>> deviations) for that rule need to be described through AST nodes, >>> meaning that all what you consider as syntactic sugar cannot be ignored. >>> >>> A deviation that covers all the pattern you are asking for could be >>> done, but it will result in a complex and quite long expression >>> (not easy to read and justify in front of an assessor). >>> >>> Hence, what I am proposing is to deviate only the the simplest and >>> most readable cases, such as: >>> >>> if ( cond ) >>> return x; >>> else >>> return y; >>> >>> without involving compound statements, fake do-wile and fake if >>> statements but rather deviating the macro inside of which are used >>> (as I did). >> >> I see. Problem is that this isn't sufficient for the code we have, and >> the seemingly random deviation of certain constructs by name looks to >> me as pretty undesirable. > > Yeah, I also think it is not ideal. At the same time among all options, > it is probably the best way forward (better than in-code comments or > better than leaving the violations outstanding). > > I think we should go for it. > I'll propose a v2 with an ECLAIR configurations that covers all the patterns you are asking for, so that we can discuss it and understand what the best solution is.
diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl index fd32ff8a9c..28ce251dc1 100644 --- a/automation/eclair_analysis/ECLAIR/deviations.ecl +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl @@ -349,11 +349,14 @@ therefore have the same behavior of a boolean" # Series 16. # --doc_begin="Switch clauses ending with continue, goto, return statements are -safe." +-doc_begin="Switch clauses ending with an unconditional flow control statement (i.e. continue, goto or return) are safe." -config=MC3R1.R16.3,terminals+={safe, "node(continue_stmt||goto_stmt||return_stmt)"} -doc_end +-doc_begin="Switch clauses ending with an if-else statement are safe if both branches consist of a flow control statement (i.e., break, continue, goto, return)." +-config=MC3R1.R16.3,terminals+={safe, "node(if_stmt)&&child(then,node(break_stmt||continue_stmt||goto_stmt||return_stmt))&&child(else,node(break_stmt||continue_stmt||goto_stmt||return_stmt))"} +-doc_end + -doc_begin="Switch clauses ending with a call to a function that does not give the control back (i.e., a function with attribute noreturn) are safe." -config=MC3R1.R16.3,terminals+={safe, "call(property(noreturn))"} @@ -368,6 +371,14 @@ safe." -config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/BUG\\(\\);/))))"} -doc_end +-doc_begin="On X86, switch clauses ending generating an exception through \"generate_exception()\" are safe." +-config=MC3R1.R16.3,reports+={safe, "any_area(any_loc(file(^xen/arch/x86/.*$)&&any_exp(text(/generate_exception(.*?)/))))"} +-doc_end + +-doc_begin="Switch clauses ending generating a parse error through \"PARSE_ERR_RET()\" are safe." +-config=MC3R1.R16.3,reports+={safe, "any_area(any_loc(any_exp(text(/PARSE_ERR_RET(.*?)/))))"} +-doc_end + -doc_begin="Switch clauses not ending with the break statement are safe if an explicit comment indicating the fallthrough intention is present." -config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(^(?s).*/\\* [fF]all ?through.? \\*/.*$,0..1))))"} diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst index 123c78e20a..dbff17f493 100644 --- a/docs/misra/deviations.rst +++ b/docs/misra/deviations.rst @@ -291,7 +291,14 @@ Deviations related to MISRA C:2012 Rules: - Project-wide deviation; tagged as `deliberate` for ECLAIR. * - R16.3 - - Switch clauses ending with continue, goto, return statements are safe. + - Switch clauses ending with an unconditional flow control statement + (i.e., continue, goto, or return) are safe. + - Tagged as `safe` for ECLAIR. + + * - R16.3 + - Switch clauses ending with an if-else statement are safe if both + branches consist of a flow control statement (i.e., continue, break, + goto, return). - Tagged as `safe` for ECLAIR. * - R16.3 @@ -307,6 +314,16 @@ Deviations related to MISRA C:2012 Rules: - Switch clauses ending with failure method \"BUG()\" are safe. - Tagged as `safe` for ECLAIR. + * - R16.3 + - On X86, switch clauses ending generating an exception through + \"generate_exception()\" are safe. + - Tagged as `safe` for ECLAIR. + + * - R16.3 + - Switch clauses ending generating a parse error through + \"PARSE_ERR_RET()\" are safe. + - Tagged as `safe` for ECLAIR. + * - R16.3 - Existing switch clauses not ending with the break statement are safe if an explicit comment indicating the fallthrough intention is present.
Update ECLAIR configuration to deviate more cases where an unintentional fallthrough cannot happen. Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> --- .../eclair_analysis/ECLAIR/deviations.ecl | 15 +++++++++++++-- docs/misra/deviations.rst | 19 ++++++++++++++++++- 2 files changed, 31 insertions(+), 3 deletions(-)