Re: [XEN PATCH v2] automation/eclair: add deviations for MISRA C:2012 Rule 8.3

[Julien Grall <julien@xxxxxxx>]

Hi,

On 26/10/2023 15:04, Federico Serafini wrote:
> On 26/10/23 15:54, Julien Grall wrote:
> > Hi,
> >
> > On 26/10/2023 13:13, Federico Serafini wrote:
> > > On 26/10/23 12:25, Julien Grall wrote:
> > > > Hi,
> > > >
> > > > On 26/10/2023 11:04, Federico Serafini wrote:
> > > > > Update ECLAIR configuration to deviate Rule 8.3 ("All declarations of
> > > > > an object or function shall use the same names and type qualifiers")
> > > > > for the following functions: guest_walk_tables_[0-9]+_levels().
> > > > > Update file docs/misra/deviations.rst accordingly.
> > > > > No functional change.
> > > > >
> > > > > Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> > > > > ---
> > > > > Changes in v2:
> > > > >    - removed set_px_pminfo() from the scope of the deviation;
> > > > >    - fixed tag of the commit.
> > > > > ---
> > > > >   automation/eclair_analysis/ECLAIR/deviations.ecl | 4 ++++
> > > > >   docs/misra/deviations.rst                        | 6 ++++++
> > > > >   2 files changed, 10 insertions(+)
> > > > >
> > > > > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> > > > > b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > > > > index d8170106b4..b99dfdafd6 100644
> > > > > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> > > > > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > > > > @@ -204,6 +204,10 @@ const-qualified."
> > > > > -config=MC3R1.R8.3,reports+={deliberate,"any_area(any_loc(file(adopted_mpparse_r8_3)))&&any_area(any_loc(file(^xen/arch/x86/include/asm/mpspec\\.h$)))"}
> > > > >   -doc_end
> > > > > +-doc_begin="For functions guest_walk_tables_[0-9]+_levels(), 
> > > > > parameter names of definitions deliberately differ from the ones 
> > > > > used in the corresponding declarations."
> > > > > +-config=MC3R1.R8.3,declarations={deliberate,"^guest_walk_tables_[0-9]+_levels\\(const
> > > > >  struct vcpu\\*, struct p2m_domain\\*, unsigned long, walk_t\\*, uint32_t, gfn_t, mfn_t, 
> > > > > void\\*\\)$"}
> > > > > +-doc_end
> > > > > +
> > > > >   -doc_begin="The following variables are compiled in multiple 
> > > > > translation units
> > > > >   belonging to different executables and therefore are safe."
> > > > >   -config=MC3R1.R8.6,declarations+={safe, 
> > > > > "name(current_stack_pointer||bsearch||sort)"}
> > > > > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
> > > > > index 8511a18925..9423b5cd6b 100644
> > > > > --- a/docs/misra/deviations.rst
> > > > > +++ b/docs/misra/deviations.rst
> > > > > @@ -121,6 +121,12 @@ Deviations related to MISRA C:2012 Rules:
> > > > >            - xen/common/unxz.c
> > > > >            - xen/common/unzstd.c
> > > > > +   * - R8.3
> > > > > +     - In some cases, parameter names used in the function definition
> > > > > +       deliberately differ from the ones used in the corresponding 
> > > > > declaration.
> > > > It would be helpful to provide a bit more reasoning in your commit 
> > > > message why this was desired. At least for Arm and common code, I 
> > > > would not want anyone to do that because it adds more confusion.
> > > > > +     - Tagged as `deliberate` for ECLAIR. Such functions are:
> > > > > +         - guest_walk_tables_[0-9]+_levels()
> > > > I think you want to be a bit mores specific. Other arch may have 
> > > > such function in the function and we don't want to deviate them from 
> > > > the start.
> > > > Cheers,
> > > Alright, thanks for the observation.
> > Actually, I cannot find the original discussion. Do you have link? I 
> > am interested to read the reasoning and how many maintainers expressed 
> > there view.
> > Cheers,
> The discussion started here:
> https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg00122.html
>
> Then, I asked for further suggestions:
> https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg00855.html
Thanks! So only Jan really provided feedback here. I don't think this is 
a good idea to deviate in this case. If we really want to keep in sync 
and use 'walk' for the name, then we could add a comment after. 
Something like:
uint32_t walk /* pfec */

Cheers,

--
Julien Grall