Re: [XEN PATCH] automation/eclair: add deviation for MISRA C:2012 Rule 16.3

[Federico Serafini <federico.serafini@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> > > > > 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.

--
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)