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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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