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

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2849 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 4021 bytes --]