Re: Rule 10.1 violations in perfc_incra and PERFCOUNTER_ARRAY

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 10 Oct 2023, Nicola Vetrini wrote:
> On 10/10/2023 03:56, Stefano Stabellini wrote:
> > On Fri, 6 Oct 2023, Nicola Vetrini wrote:
> > > Given the following macros in <xen/perfc.h> and
> > > 
> > > #define perfc_incra(x,y)                                                \
> > >     ( (y) <= PERFC_LAST_ ## x - PERFC_ ## x ?                           \
> > >    ++this_cpu(perfcounters)[PERFC_ ## x + (y)] : 0 )
> > > 
> > > and the following violation
> > > 
> > > xen/arch/arm/traps.c:1407.5-1407.32:
> > >   reference to enum constant `PERFC_hypercalls' has essential type `enum
> > > perfcounter'
> > >   and standard type `int'
> > >  <preprocessed xen/arch/arm/traps.c>:11606.29-11606.44: preprocessed
> > > tokens
> > >  <scratch space>:137.1-137.16: expanded from macro `PERFC_'
> > >  xen/include/xen/perfc.h:69.35-69.45: expanded from macro `perfc_incra'
> > > xen/arch/arm/traps.c:1407.5-1407.32:
> > >   `+' addition operator expects a number or a character
> > >  <preprocessed xen/arch/arm/traps.c>:11606.46: preprocessed tokens
> > >  xen/include/xen/perfc.h:69.47: expanded from macro `perfc_incra'
> > > 
> > > the difference between enumerated values is forbidden by the Rule. In the
> > > coding standard's
> > > interpretation, named enums are unordered list of symbols, which can only
> > > be
> > > compared for
> > > equality.
> > > There are a few possible paths forward:
> > > 
> > > 1. use means different from named enums to generate these constants (such
> > > as
> > > #define-s or
> > >    constants integers);
> > 
> > This is a viable option
> > 
> > 
> > > 2. explicitly deviate subtraction of enums, therefore defining an explicit
> > > ordering on
> > >    enumerated values;
> > 
> > I would prefer to avoid a project-wide deviation, because I think in
> > general it is a good rule to have. If we go with a deviation, I think it
> > would be better to deviate PERFCOUNTER or perfc.h specifically. This is
> > a file that changes very rarely. We could make the case that this is
> > safe with GCC (most probably is) and this case was reviewed carefully
> > and doesn't change often (3 changes in the last 10 yeas).
> > 
> > 
> > > 3. use an unnamed enum, effectively considering the enumerated values as
> > > plain
> > > integers.
> > >    This does not violate the Rule.
> > 
> > What do you mean by unname enum?
> 
> e.g.
> 
> enum {
> #include <xen/perfc_defn.h>
>       NUM_PERFCOUNTERS
> };
> 
> instead of
> 
> enum perfcounter {
> #include <xen/perfc_defn.h>
>       NUM_PERFCOUNTERS
> };


I think this should be easy to do in this case. I gave it a quick try
and it seems to still build successfully. It could be the best way
forward for this instance.


However in general I am confused why unnamed enum can do comparisons
between members and named enums cannot. What is the reason? In any case,
I think we should clarify this detail in the notes section of
docs/misra/rules.rst, because I don't think it was clear to anyone that
there is a difference in behavior between named and unnamed enums.