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

Commit Message

Javi Merino Sept. 8, 2023, 4:20 p.m. UTC

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 <[e-mail]>
---
 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

Comments

Jan Beulich Sept. 11, 2023, 7:54 a.m. UTC | #1

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 <[e-mail]>
> ---
>  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 <[e-mail]>

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

Javi Merino Sept. 11, 2023, 9:13 a.m. UTC | #2

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 <[e-mail]>
> > ---
> >  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 <[e-mail]>

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

Andrew Cooper Sept. 11, 2023, 9:48 a.m. UTC | #3

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 <[e-mail]>
>> ---
>>  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.

More importantly, it's the more natural way to structure such a list.

~Andrew

Jan Beulich Sept. 11, 2023, 9:53 a.m. UTC | #4

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 <[e-mail]>
>>> ---
>>>  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 (to amend
the version check), so I'm afraid I don't see such a diff being smaller
(it's uniformly -1/+3 afaict).

> More importantly, it's the more natural way to structure such a list.

I would say multiple views are possible: Naming recent compiler versions
first may also be viewed as beneficial. In the end what counts is how
easy it is to follow the overall construct. And I'm pretty sure less
complex expressions help there.

Jan

Andrew Cooper Sept. 11, 2023, 9:59 a.m. UTC | #5

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 <[e-mail]>
>>>> ---
>>>>  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

~Andrew

Jan Beulich Sept. 11, 2023, 10:33 a.m. UTC | #6

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 <[e-mail]>
>>>>> ---
>>>>>  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

Patch

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
 
 #define GCOV_TAG_FUNCTION_LENGTH        3
diff --git a/xen/common/coverage/gcc_4_9.c b/xen/common/coverage/gcc_4_9.c
deleted file mode 100644
index dcea961936..0000000000
--- a/xen/common/coverage/gcc_4_9.c
+++ /dev/null
@@ -1,33 +0,0 @@ 
-/*
- *  This code provides functions to handle gcc's profiling data format
- *  introduced with gcc 4.7.
- *
- *  For a better understanding, refer to gcc source:
- *  gcc/gcov-io.h
- *  libgcc/libgcov.c
- *
- *  Uses gcc-internal data definitions.
- *
- *  Imported from Linux and modified for Xen by
- *    Wei Liu <[e-mail]>
- */
-
-#include "gcov.h"
-
-#if !(GCC_VERSION >= 40900 && GCC_VERSION < 50000)
-#error "Wrong version of GCC used to compile gcov"
-#endif
-
-#define GCOV_COUNTERS 9
-
-#include "gcc_4_7.c"
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/common/coverage/gcc_5.c b/xen/common/coverage/gcc_5.c
deleted file mode 100644
index 6e0d276f3b..0000000000
--- a/xen/common/coverage/gcc_5.c
+++ /dev/null
@@ -1,33 +0,0 @@ 
-/*
- *  This code provides functions to handle gcc's profiling data format
- *  introduced with gcc 5.
- *
- *  For a better understanding, refer to gcc source:
- *  gcc/gcov-io.h
- *  libgcc/libgcov.c
- *
- *  Uses gcc-internal data definitions.
- *
- *  Imported from Linux and modified for Xen by
- *    Wei Liu <[e-mail]>
- */
-
-#include "gcov.h"
-
-#if GCC_VERSION < 50000 || GCC_VERSION >= 70000
-#error "Wrong version of GCC used to compile gcov"
-#endif
-
-#define GCOV_COUNTERS 10
-
-#include "gcc_4_7.c"
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */
diff --git a/xen/common/coverage/gcc_7.c b/xen/common/coverage/gcc_7.c
deleted file mode 100644
index 3962eb4c76..0000000000
--- a/xen/common/coverage/gcc_7.c
+++ /dev/null
@@ -1,30 +0,0 @@ 
-/*
- *  This code provides functions to handle gcc's profiling data format
- *  introduced with gcc 7.
- *
- *  For a better understanding, refer to gcc source:
- *  gcc/gcov-io.h
- *  libgcc/libgcov.c
- *
- *  Uses gcc-internal data definitions.
- */
-
-#include "gcov.h"
-
-#if GCC_VERSION < 70000
-#error "Wrong version of GCC used to compile gcov"
-#endif
-
-#define GCOV_COUNTERS 9
-
-#include "gcc_4_7.c"
-
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */