| Message ID | b1d2b64c8117d61ea42cf4e9feae128541eb0b61.1708348799.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN] automation/eclair: add deviation for MISRA C:2012 Rule 16.3 | expand |
On 19.02.2024 14:24, Federico Serafini wrote: > Update ECLAIR configuration to consider safe switch clauses ending > with __{get,put}_user_bad(). > > Update docs/misra/deviations.rst accordingly. > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> As mentioned I'm not happy with this, not the least because of it being unclear why these two would be deviated, when there's no sign of a similar deviation for, say, __bad_atomic_size(). Imo this approach doesn't scale, and that's already leaving aside that the purpose of identically named (pseudo-)helpers could differ between architectures, thus putting under question ... > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl > @@ -368,6 +368,10 @@ safe." > -config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/BUG\\(\\);/))))"} > -doc_end > > +-doc_begin="Switch clauses ending with constructs \"__get_user_bad()\" and \"__put_user_bad()\" are safe: they denote an unreachable program point." > +-config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/__(put|get)_user_bad\\(\\);/))))"} > +-doc_end ... the global scope of such a deviation. While it may not be a good idea, even within an arch such (pseudo-)helpers could be used for multiple distinct purposes. Jan
On 19/02/24 14:43, Jan Beulich wrote: > On 19.02.2024 14:24, Federico Serafini wrote: >> Update ECLAIR configuration to consider safe switch clauses ending >> with __{get,put}_user_bad(). >> >> Update docs/misra/deviations.rst accordingly. >> >> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> > > As mentioned I'm not happy with this, not the least because of it being > unclear why these two would be deviated, when there's no sign of a > similar deviation for, say, __bad_atomic_size(). Imo this approach > doesn't scale, and that's already leaving aside that the purpose of > identically named (pseudo-)helpers could differ between architectures, > thus putting under question ... > >> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >> @@ -368,6 +368,10 @@ safe." >> -config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/BUG\\(\\);/))))"} >> -doc_end >> >> +-doc_begin="Switch clauses ending with constructs \"__get_user_bad()\" and \"__put_user_bad()\" are safe: they denote an unreachable program point." >> +-config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/__(put|get)_user_bad\\(\\);/))))"} >> +-doc_end > > ... the global scope of such a deviation. While it may not be a good idea, > even within an arch such (pseudo-)helpers could be used for multiple > distinct purposes. Would you agree with adding the missing break statement after the uses of __{put,get}_user_bad() (as it is already happening for uses of __bad_atomic_size())?
On 19.02.2024 15:59, Federico Serafini wrote: > On 19/02/24 14:43, Jan Beulich wrote: >> On 19.02.2024 14:24, Federico Serafini wrote: >>> Update ECLAIR configuration to consider safe switch clauses ending >>> with __{get,put}_user_bad(). >>> >>> Update docs/misra/deviations.rst accordingly. >>> >>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> >> >> As mentioned I'm not happy with this, not the least because of it being >> unclear why these two would be deviated, when there's no sign of a >> similar deviation for, say, __bad_atomic_size(). Imo this approach >> doesn't scale, and that's already leaving aside that the purpose of >> identically named (pseudo-)helpers could differ between architectures, >> thus putting under question ... >> >>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >>> @@ -368,6 +368,10 @@ safe." >>> -config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/BUG\\(\\);/))))"} >>> -doc_end >>> >>> +-doc_begin="Switch clauses ending with constructs \"__get_user_bad()\" and \"__put_user_bad()\" are safe: they denote an unreachable program point." >>> +-config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/__(put|get)_user_bad\\(\\);/))))"} >>> +-doc_end >> >> ... the global scope of such a deviation. While it may not be a good idea, >> even within an arch such (pseudo-)helpers could be used for multiple >> distinct purposes. > > Would you agree with adding the missing break statement after > the uses of __{put,get}_user_bad() (as it is already happening for > uses of __bad_atomic_size())? I probably wouldn't mind that (despite being a little pointless). Perhaps declaring them as noreturn would also help? Jan
On 19/02/24 16:06, Jan Beulich wrote: > On 19.02.2024 15:59, Federico Serafini wrote: >> On 19/02/24 14:43, Jan Beulich wrote: >>> On 19.02.2024 14:24, Federico Serafini wrote: >>>> Update ECLAIR configuration to consider safe switch clauses ending >>>> with __{get,put}_user_bad(). >>>> >>>> Update docs/misra/deviations.rst accordingly. >>>> >>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> >>> >>> As mentioned I'm not happy with this, not the least because of it being >>> unclear why these two would be deviated, when there's no sign of a >>> similar deviation for, say, __bad_atomic_size(). Imo this approach >>> doesn't scale, and that's already leaving aside that the purpose of >>> identically named (pseudo-)helpers could differ between architectures, >>> thus putting under question ... >>> >>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >>>> @@ -368,6 +368,10 @@ safe." >>>> -config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/BUG\\(\\);/))))"} >>>> -doc_end >>>> >>>> +-doc_begin="Switch clauses ending with constructs \"__get_user_bad()\" and \"__put_user_bad()\" are safe: they denote an unreachable program point." >>>> +-config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/__(put|get)_user_bad\\(\\);/))))"} >>>> +-doc_end >>> >>> ... the global scope of such a deviation. While it may not be a good idea, >>> even within an arch such (pseudo-)helpers could be used for multiple >>> distinct purposes. >> >> Would you agree with adding the missing break statement after >> the uses of __{put,get}_user_bad() (as it is already happening for >> uses of __bad_atomic_size())? > > I probably wouldn't mind that (despite being a little pointless). > Perhaps declaring them as noreturn would also help? Yes, it will help. Is there any reason to have long as __get_user_bad()'s return value? It would be nicer to declare it as a void function and then add the noreturn attribute.
On 20.02.2024 09:16, Federico Serafini wrote: > On 19/02/24 16:06, Jan Beulich wrote: >> On 19.02.2024 15:59, Federico Serafini wrote: >>> On 19/02/24 14:43, Jan Beulich wrote: >>>> On 19.02.2024 14:24, Federico Serafini wrote: >>>>> Update ECLAIR configuration to consider safe switch clauses ending >>>>> with __{get,put}_user_bad(). >>>>> >>>>> Update docs/misra/deviations.rst accordingly. >>>>> >>>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> >>>> >>>> As mentioned I'm not happy with this, not the least because of it being >>>> unclear why these two would be deviated, when there's no sign of a >>>> similar deviation for, say, __bad_atomic_size(). Imo this approach >>>> doesn't scale, and that's already leaving aside that the purpose of >>>> identically named (pseudo-)helpers could differ between architectures, >>>> thus putting under question ... >>>> >>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >>>>> @@ -368,6 +368,10 @@ safe." >>>>> -config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/BUG\\(\\);/))))"} >>>>> -doc_end >>>>> >>>>> +-doc_begin="Switch clauses ending with constructs \"__get_user_bad()\" and \"__put_user_bad()\" are safe: they denote an unreachable program point." >>>>> +-config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/__(put|get)_user_bad\\(\\);/))))"} >>>>> +-doc_end >>>> >>>> ... the global scope of such a deviation. While it may not be a good idea, >>>> even within an arch such (pseudo-)helpers could be used for multiple >>>> distinct purposes. >>> >>> Would you agree with adding the missing break statement after >>> the uses of __{put,get}_user_bad() (as it is already happening for >>> uses of __bad_atomic_size())? >> >> I probably wouldn't mind that (despite being a little pointless). >> Perhaps declaring them as noreturn would also help? > > Yes, it will help. > Is there any reason to have long as __get_user_bad()'s return value? > It would be nicer to declare it as a void function and then add the > noreturn attribute. That's a historical leftover, which can be changed. Xen originally derived quite a bit of code from Linux. If you go look at Linux 2.6.16, you'll find why it was declared that way. Jan
On 20/02/24 09:31, Jan Beulich wrote: > On 20.02.2024 09:16, Federico Serafini wrote: >> On 19/02/24 16:06, Jan Beulich wrote: >>> On 19.02.2024 15:59, Federico Serafini wrote: >>>> On 19/02/24 14:43, Jan Beulich wrote: >>>>> On 19.02.2024 14:24, Federico Serafini wrote: >>>>>> Update ECLAIR configuration to consider safe switch clauses ending >>>>>> with __{get,put}_user_bad(). >>>>>> >>>>>> Update docs/misra/deviations.rst accordingly. >>>>>> >>>>>> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> >>>>> >>>>> As mentioned I'm not happy with this, not the least because of it being >>>>> unclear why these two would be deviated, when there's no sign of a >>>>> similar deviation for, say, __bad_atomic_size(). Imo this approach >>>>> doesn't scale, and that's already leaving aside that the purpose of >>>>> identically named (pseudo-)helpers could differ between architectures, >>>>> thus putting under question ... >>>>> >>>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >>>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >>>>>> @@ -368,6 +368,10 @@ safe." >>>>>> -config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/BUG\\(\\);/))))"} >>>>>> -doc_end >>>>>> >>>>>> +-doc_begin="Switch clauses ending with constructs \"__get_user_bad()\" and \"__put_user_bad()\" are safe: they denote an unreachable program point." >>>>>> +-config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/__(put|get)_user_bad\\(\\);/))))"} >>>>>> +-doc_end >>>>> >>>>> ... the global scope of such a deviation. While it may not be a good idea, >>>>> even within an arch such (pseudo-)helpers could be used for multiple >>>>> distinct purposes. >>>> >>>> Would you agree with adding the missing break statement after >>>> the uses of __{put,get}_user_bad() (as it is already happening for >>>> uses of __bad_atomic_size())? >>> >>> I probably wouldn't mind that (despite being a little pointless). >>> Perhaps declaring them as noreturn would also help? >> >> Yes, it will help. >> Is there any reason to have long as __get_user_bad()'s return value? >> It would be nicer to declare it as a void function and then add the >> noreturn attribute. > > That's a historical leftover, which can be changed. Xen originally > derived quite a bit of code from Linux. If you go look at Linux 2.6.16, > you'll find why it was declared that way. Thank you, I'll send a v2.
diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl index fd32ff8a9c..831b5d4c67 100644 --- a/automation/eclair_analysis/ECLAIR/deviations.ecl +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl @@ -368,6 +368,10 @@ safe." -config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/BUG\\(\\);/))))"} -doc_end +-doc_begin="Switch clauses ending with constructs \"__get_user_bad()\" and \"__put_user_bad()\" are safe: they denote an unreachable program point." +-config=MC3R1.R16.3,reports+={safe, "any_area(end_loc(any_exp(text(/__(put|get)_user_bad\\(\\);/))))"} +-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..58f4fac18e 100644 --- a/docs/misra/deviations.rst +++ b/docs/misra/deviations.rst @@ -307,6 +307,12 @@ Deviations related to MISRA C:2012 Rules: - Switch clauses ending with failure method \"BUG()\" are safe. - Tagged as `safe` for ECLAIR. + * - R16.3 + - Switch clauses ending with constructs + \"__get_user_bad()\" and \"__put_user_bad()\" are safe: + they denote an unreachable program point. + - 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 consider safe switch clauses ending with __{get,put}_user_bad(). Update docs/misra/deviations.rst accordingly. Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> --- automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++ docs/misra/deviations.rst | 6 ++++++ 2 files changed, 10 insertions(+)