Re: [XEN PATCH v2 1/2] coverage: simplify the logic of choosing the number of gcov counters depending on the gcc version

[Javi Merino <javi.merino@xxxxxxxxx>]

On Mon, Sep 11, 2023 at 09:54:53AM +0200, Jan Beulich wrote:
> On 08.09.2023 18:20, Javi Merino wrote:
> > The current structure of choosing the correct file based on the
> > compiler version makes us make 33 line files just to define a
> > constant.  The changes after gcc 4.7 are minimal, just the number of
> > counters.
> > 
> > Fold the changes in gcc_4_9.c, gcc_5.c and gcc_7.c into gcc_4_7.c to
> > remove a lot of the boilerplate and keep the logic of choosing the
> > GCOV_COUNTER in gcc_4_7.c.
> > 
> > Signed-off-by: Javi Merino <javi.merino@xxxxxxxxx>
> > ---
> >  xen/common/coverage/Makefile  |  6 +-----
> >  xen/common/coverage/gcc_4_7.c | 17 +++++++++--------
> >  xen/common/coverage/gcc_4_9.c | 33 ---------------------------------
> >  xen/common/coverage/gcc_5.c   | 33 ---------------------------------
> >  xen/common/coverage/gcc_7.c   | 30 ------------------------------
> >  5 files changed, 10 insertions(+), 109 deletions(-)
> >  delete mode 100644 xen/common/coverage/gcc_4_9.c
> >  delete mode 100644 xen/common/coverage/gcc_5.c
> >  delete mode 100644 xen/common/coverage/gcc_7.c
> > 
> > diff --git a/xen/common/coverage/Makefile b/xen/common/coverage/Makefile
> > index 63f98c71d6..d729afc9c7 100644
> > --- a/xen/common/coverage/Makefile
> > +++ b/xen/common/coverage/Makefile
> > @@ -1,11 +1,7 @@
> >  obj-y += coverage.o
> >  ifneq ($(CONFIG_CC_IS_CLANG),y)
> >  obj-y += gcov_base.o gcov.o
> > -obj-y += $(call cc-ifversion,-lt,0407, \
> > -           gcc_3_4.o, $(call cc-ifversion,-lt,0409, \
> > -           gcc_4_7.o, $(call cc-ifversion,-lt,0500, \
> > -           gcc_4_9.o, $(call cc-ifversion,-lt,0700, \
> > -           gcc_5.o, gcc_7.o))))
> > +obj-y += $(call cc-ifversion,-lt,0407, gcc_3_4.o, gcc_4_7.o)
> >  else
> >  obj-y += llvm.o
> >  endif
> > diff --git a/xen/common/coverage/gcc_4_7.c b/xen/common/coverage/gcc_4_7.c
> > index 25b4a8bcdc..ddbc9f4bb0 100644
> > --- a/xen/common/coverage/gcc_4_7.c
> > +++ b/xen/common/coverage/gcc_4_7.c
> > @@ -18,15 +18,16 @@
> >  
> >  #include "gcov.h"
> >  
> > -/*
> > - * GCOV_COUNTERS will be defined if this file is included by other
> > - * source files.
> > - */
> > -#ifndef GCOV_COUNTERS
> > -# if !(GCC_VERSION >= 40700 && GCC_VERSION < 40900)
> > -#  error "Wrong version of GCC used to compile gcov"
> > -# endif
> > +#if (GCC_VERSION >= 40700 && GCC_VERSION < 40900)
> >  #define GCOV_COUNTERS 8
> > +#elif (GCC_VERSION >= 40900 && GCC_VERSION < 50000)
> > +#define GCOV_COUNTERS 9
> > +#elif GCC_VERSION >= 50000 && GCC_VERSION < 70000
> > +#define GCOV_COUNTERS 10
> > +#elif GCC_VERSION >= 70000
> > +#define GCOV_COUNTERS 9
> > +#else
> > +#error "Wrong version of GCC used to compile gcov"
> >  #endif
> 
> How about inverse order:
> 
> #if GCC_VERSION >= 70000
> #define GCOV_COUNTERS 9
> #elif GCC_VERSION >= 50000
> #define GCOV_COUNTERS 10
> #elif GCC_VERSION >= 40900
> #define GCOV_COUNTERS 9
> #elif GCC_VERSION >= 40700
> #define GCOV_COUNTERS 8
> #else
> #error "Wrong version of GCC used to compile gcov"
> #endif
> 
> Otherwise a nit and a question: Parentheses would want using consistently.

True, the parenthesis are unnecessary and inconsistent in the patch.

> And wouldn't it make sense to combine the two ranges resulting in 9 being
> chosen? (Imo in the alternative layout that's less desirable.)
> 
> Since the adjustment would be easy to make, I'd be fine doing so while
> committing, and then
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Happy for you to do the changes.  Or I can do it and fix the next
patch as well.

Cheers,
Javi

> As an unrelated remark: gcc_3_4.c is clearly outdated as well, simply by
> its name. Imo it would have wanted to be gcc_4_1.c the latest as of commit
> 03f22f0070f3 ("README: adjust gcc version requirement"), i.e. for over 10
> years.
> 
> Jan