Re: [RFC PATCH v4] compiler.h: remove GCC < 3 __builtin_expect fallback

[Peter Maydell]

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