Re: [Qemu-devel] [PATCH v2] osdep: Make MIN/MAX evaluate arguments only once

[Eric Blake]

On 1/7/19 3:49 AM, Dr. David Alan Gilbert wrote:
> * Eric Blake (address@hidden) wrote:
>> Add the macro QEMU_TYPEOF() to access __auto_type in new enough
>> compilers, while falling back to typeof on older compilers (the
>> fallback doesn't handle variable length arrays, but we don't use
>> those; it also expands to more text).
>>
>> Then use that macro to make MIN/MAX only evaluate their argument
>> once; this uses type promotion (by adding to 0) to work around
>> the fact that typeof(bitfield) won't compile.  However, we are
>> unable to work around gcc refusing to compile ({}) in a constant
>> context, even when only used in the dead branch of a
>> __builtin_choose_expr(),


>> +#undef MIN
>> +#define MIN(a, b)                            \
>> +    ({                                       \
>> +        QEMU_TYPEOF((a) + 0) _a = (a) + 0;   \
>> +        QEMU_TYPEOF((b) + 0) _b = (b) + 0;   \
>> +        _a < _b ? _a : _b;                   \
>> +    })
>> +#define MIN_CONST(a, b)                                         \
>> +    __builtin_choose_expr(                                      \
>> +        __builtin_constant_p(a) && __builtin_constant_p(b),     \
>> +        (a) < (b) ? (a) : (b),                                  \
>> +        __builtin_unreachable())
> 
> Why do these need to be separate macros? Can't you just put the 
> non-constant code in what you have as the 'builtin_unreachable' side of
> the choose_expr:
> 
> #define DMIN(a,b) __builtin_choose_expr(                  \
>     __builtin_constant_p(a) && __builtin_constant_p(b),   \
>     (a) < (b) ? (a) : (b),                                \
>     ({                                       \
>         QEMU_TYPEOF((a) + 0) _a = (a) + 0;   \
>         QEMU_TYPEOF((b) + 0) _b = (b) + 0;   \
>         _a < _b ? _a : _b;                   \
>     }))

Because it doesn't work - gcc treats ({}) as a syntax error inside
constant expressions, even in dead code (although 'info gcc' said that
might change in the future, we can't wait for that change).  I also
tried it as documented here:
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg00715.html
hence my mention in the commit message.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature