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 11.09.2023 11:59, Andrew Cooper wrote:
> On 11/09/2023 10:53 am, Jan Beulich wrote:
>> On 11.09.2023 11:48, Andrew Cooper wrote:
>>> On 11/09/2023 8:54 am, 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
>>> Forward order is the one that results in a smaller diff when inserting
>>> new entries.
>> Hmm, even in forward order one prior #elif will need touching
> 
>  #if GCC_VERSION < 40700
>  #define GCOV_COUNTERS 8
>  #elif GCC_VERSION < 40900
>  #define GCOV_COUNTERS 9
>  #elif GCC_VERSION < 50000
>  #define GCOV_COUNTERS 10
>  #elif GCC_VERSION < 70000
>  #define GCOV_COUNTERS 9
> +#elif GCC_VERSION < FOO
> +#define GCOV_COUNTERS BAR
>  #else
>  #error "Wrong version of GCC used to compile gcov"
>  #endif

But that doesn't achieve the same as the other construct: No #error for
gcc earlier than 4.7, and your (presumed; i.e. taking just patch context
without the two added lines) construct would fail to build for gcc 7. We
want an open upper bound but a determined lower one.

Jan