Since commit efc6c07 ("configure: Add a test for the minimum compiler
version"), QEMU explicitely depends on GCC >= 4.8.

(clang >= 3.4 advertizes itself as GCC >= 4.2 compatible and supports
__builtin_expect too)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
[PMD: #error if likely/unlikely already defined]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Supersedes: <20201210134752.780923-4-marcandre.lureau@redhat.com>
---
 include/qemu/compiler.h | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index c76281f3540..ae1aee79c8d 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -43,14 +43,11 @@
 #define tostring(s)	#s
 #endif
 
-#ifndef likely
-#if __GNUC__ < 3
-#define __builtin_expect(x, n) (x)
+#if defined(likely) || defined(unlikely)
+#error building with likely/unlikely is not supported
 #endif
-
 #define likely(x)   __builtin_expect(!!(x), 1)
 #define unlikely(x)   __builtin_expect(!!(x), 0)
-#endif
 
 #ifndef container_of
 #define container_of(ptr, type, member) ({                      \
-- 
2.26.2

On 12/11/20 2:13 PM, Philippe Mathieu-Daudé wrote:
> Since commit efc6c07 ("configure: Add a test for the minimum compiler
> version"), QEMU explicitely depends on GCC >= 4.8.
> 
> (clang >= 3.4 advertizes itself as GCC >= 4.2 compatible and supports
> __builtin_expect too)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> [PMD: #error if likely/unlikely already defined]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Supersedes: <20201210134752.780923-4-marcandre.lureau@redhat.com>
> ---
>  include/qemu/compiler.h | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index c76281f3540..ae1aee79c8d 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -43,14 +43,11 @@
>  #define tostring(s)	#s
>  #endif
>  
> -#ifndef likely
> -#if __GNUC__ < 3
> -#define __builtin_expect(x, n) (x)
> +#if defined(likely) || defined(unlikely)
> +#error building with likely/unlikely is not supported

I'd put the argument of #error as a single string constant delimited by quotes ",
doesn't matter in this case though as there are no problem characters in the string.

>  #endif
> -
>  #define likely(x)   __builtin_expect(!!(x), 1)
>  #define unlikely(x)   __builtin_expect(!!(x), 0)
> -#endif
>  
>  #ifndef container_of
>  #define container_of(ptr, type, member) ({                      \
> 

Reviewed-by: Claudio Fontana <cfontana@suse.de>

On 12/11/20 2:28 PM, Claudio Fontana wrote:
> On 12/11/20 2:13 PM, Philippe Mathieu-Daudé wrote:
>> Since commit efc6c07 ("configure: Add a test for the minimum compiler
>> version"), QEMU explicitely depends on GCC >= 4.8.
>>
>> (clang >= 3.4 advertizes itself as GCC >= 4.2 compatible and supports
>> __builtin_expect too)
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> [PMD: #error if likely/unlikely already defined]
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Supersedes: <20201210134752.780923-4-marcandre.lureau@redhat.com>
>> ---
>>  include/qemu/compiler.h | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>> index c76281f3540..ae1aee79c8d 100644
>> --- a/include/qemu/compiler.h
>> +++ b/include/qemu/compiler.h
>> @@ -43,14 +43,11 @@
>>  #define tostring(s)	#s
>>  #endif
>>  
>> -#ifndef likely
>> -#if __GNUC__ < 3
>> -#define __builtin_expect(x, n) (x)
>> +#if defined(likely) || defined(unlikely)
>> +#error building with likely/unlikely is not supported
> 
> I'd put the argument of #error as a single string constant delimited by quotes ",
> doesn't matter in this case though as there are no problem characters in the string.

This is the style I prefer too, but I this seems the QEMU style...:

$ git grep -E \#\s*error|wc -l
82
$ git grep -E \#\s*error|fgrep \"|wc -l
11

> 
>>  #endif
>> -
>>  #define likely(x)   __builtin_expect(!!(x), 1)
>>  #define unlikely(x)   __builtin_expect(!!(x), 0)
>> -#endif
>>  
>>  #ifndef container_of
>>  #define container_of(ptr, type, member) ({                      \
>>
> 
> Reviewed-by: Claudio Fontana <cfontana@suse.de>
> 

On Fri, 11 Dec 2020 at 13:13, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Since commit efc6c07 ("configure: Add a test for the minimum compiler
> version"), QEMU explicitely depends on GCC >= 4.8.
>
> (clang >= 3.4 advertizes itself as GCC >= 4.2 compatible and supports
> __builtin_expect too)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> [PMD: #error if likely/unlikely already defined]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Supersedes: <20201210134752.780923-4-marcandre.lureau@redhat.com>
> ---
>  include/qemu/compiler.h | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index c76281f3540..ae1aee79c8d 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -43,14 +43,11 @@
>  #define tostring(s)    #s
>  #endif
>
> -#ifndef likely
> -#if __GNUC__ < 3
> -#define __builtin_expect(x, n) (x)
> +#if defined(likely) || defined(unlikely)
> +#error building with likely/unlikely is not supported

When exactly will the system headers have 'likely' defined,
and when would they define it to something other than the
obvious __builtin_expect() definition the way we do?

likely() and unlikely() in my view fall into a category of
macros like "container_of()" which aren't defined by a system
header but which do have a standard known set of semantics.

I think there are two reasonable approaches:
 (1) just define the macro, on the assumption that the
system headers won't have done (because these aren't standard
macros)
 (2) as we do with container_of() currently, wrap our
definitions in #ifndef whatever, so that we assume that
whatever version we might have got from the system is fine

I don't think there's any point in explicitly #error-ing here:
in fact, it makes the diagnostic to the user less useful,
because instead of the compiler complaining about the macro
being defined twice and giving both locations where it was
defined, now it won't tell the user where the other definition
was...

I think my preference would be "just drop the ifdef", but
there isn't much in it really.

thanks
-- PMM

On 12/11/20 2:33 PM, Peter Maydell wrote:
> On Fri, 11 Dec 2020 at 13:13, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Since commit efc6c07 ("configure: Add a test for the minimum compiler
>> version"), QEMU explicitely depends on GCC >= 4.8.
>>
>> (clang >= 3.4 advertizes itself as GCC >= 4.2 compatible and supports
>> __builtin_expect too)
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> [PMD: #error if likely/unlikely already defined]
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Supersedes: <20201210134752.780923-4-marcandre.lureau@redhat.com>
>> ---
>>  include/qemu/compiler.h | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>> index c76281f3540..ae1aee79c8d 100644
>> --- a/include/qemu/compiler.h
>> +++ b/include/qemu/compiler.h
>> @@ -43,14 +43,11 @@
>>  #define tostring(s)    #s
>>  #endif
>>
>> -#ifndef likely
>> -#if __GNUC__ < 3
>> -#define __builtin_expect(x, n) (x)
>> +#if defined(likely) || defined(unlikely)
>> +#error building with likely/unlikely is not supported
> 
> When exactly will the system headers have 'likely' defined,
> and when would they define it to something other than the
> obvious __builtin_expect() definition the way we do?

Since there was a check, I tried to be extra-cautious
(better safe than sorry).

> likely() and unlikely() in my view fall into a category of
> macros like "container_of()" which aren't defined by a system
> header but which do have a standard known set of semantics.
> 
> I think there are two reasonable approaches:
>  (1) just define the macro, on the assumption that the
> system headers won't have done (because these aren't standard
> macros)
>  (2) as we do with container_of() currently, wrap our
> definitions in #ifndef whatever, so that we assume that
> whatever version we might have got from the system is fine
> 
> I don't think there's any point in explicitly #error-ing here:
> in fact, it makes the diagnostic to the user less useful,
> because instead of the compiler complaining about the macro
> being defined twice and giving both locations where it was
> defined, now it won't tell the user where the other definition
> was...

"diagnostic less useful" is a good reason (to invalidate this
patch).

> I think my preference would be "just drop the ifdef", but
> there isn't much in it really.

Yes, let's stick to Marc-André v3.

Thanks for your review!

Phil.

Hi

On Fri, Dec 11, 2020 at 5:41 PM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> On 12/11/20 2:33 PM, Peter Maydell wrote:
> > On Fri, 11 Dec 2020 at 13:13, Philippe Mathieu-Daudé <philmd@redhat.com>
> wrote:
> >>
> >> Since commit efc6c07 ("configure: Add a test for the minimum compiler
> >> version"), QEMU explicitely depends on GCC >= 4.8.
> >>
> >> (clang >= 3.4 advertizes itself as GCC >= 4.2 compatible and supports
> >> __builtin_expect too)
> >>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> [PMD: #error if likely/unlikely already defined]
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >> Supersedes: <20201210134752.780923-4-marcandre.lureau@redhat.com>
> >> ---
> >>  include/qemu/compiler.h | 7 ++-----
> >>  1 file changed, 2 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> >> index c76281f3540..ae1aee79c8d 100644
> >> --- a/include/qemu/compiler.h
> >> +++ b/include/qemu/compiler.h
> >> @@ -43,14 +43,11 @@
> >>  #define tostring(s)    #s
> >>  #endif
> >>
> >> -#ifndef likely
> >> -#if __GNUC__ < 3
> >> -#define __builtin_expect(x, n) (x)
> >> +#if defined(likely) || defined(unlikely)
> >> +#error building with likely/unlikely is not supported
> >
> > When exactly will the system headers have 'likely' defined,
> > and when would they define it to something other than the
> > obvious __builtin_expect() definition the way we do?
>
> Since there was a check, I tried to be extra-cautious
> (better safe than sorry).
>
> > likely() and unlikely() in my view fall into a category of
> > macros like "container_of()" which aren't defined by a system
> > header but which do have a standard known set of semantics.
> >
> > I think there are two reasonable approaches:
> >  (1) just define the macro, on the assumption that the
> > system headers won't have done (because these aren't standard
> > macros)
> >  (2) as we do with container_of() currently, wrap our
> > definitions in #ifndef whatever, so that we assume that
> > whatever version we might have got from the system is fine
> >
> > I don't think there's any point in explicitly #error-ing here:
> > in fact, it makes the diagnostic to the user less useful,
> > because instead of the compiler complaining about the macro
> > being defined twice and giving both locations where it was
> > defined, now it won't tell the user where the other definition
> > was...
>
> "diagnostic less useful" is a good reason (to invalidate this
> patch).
>
> > I think my preference would be "just drop the ifdef", but
> > there isn't much in it really.
>
> Yes, let's stick to Marc-André v3.
>
> Thanks for your review!
>

Ok to r-b v3 then?
thanks



-- 
Marc-André Lureau