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

[Marc-André Lureau]

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" target="_blank">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