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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 17 Nov 2023, Federico Serafini wrote:
> On 27/10/23 00:55, Stefano Stabellini wrote:
> > +Roger
> > 
> > See below
> > 
> > On Thu, 26 Oct 2023, Julien Grall wrote:
> > > 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 */
> 
> What do you think about "pfec_walk" as parameter name?

I am OK with that