Re: [PATCH v3] osdep: Make MIN/MAX evaluate arguments only once

[Eric Blake <eblake@redhat.com>]

On 6/2/20 9:07 PM, Richard Henderson wrote:
> On 6/2/20 6:36 PM, Eric Blake wrote:
>> --- a/include/exec/cpu-all.h
>> +++ b/include/exec/cpu-all.h
>> @@ -176,11 +176,9 @@ extern unsigned long reserved_va;
>>    * avoid setting bits at the top of guest addresses that might need
>>    * to be used for tags.
>>    */
>> -#if MIN(TARGET_VIRT_ADDR_SPACE_BITS, TARGET_ABI_BITS) <= 32
>> -# define GUEST_ADDR_MAX_  UINT32_MAX
>> -#else
>> -# define GUEST_ADDR_MAX_  (~0ul)
>> -#endif
>> +#define GUEST_ADDR_MAX_                                                 \
>> +    ((MIN_CONST(TARGET_VIRT_ADDR_SPACE_BITS, TARGET_ABI_BITS) <= 32) ?  \
>> +     UINT32_MAX : ~0ul)
> 
> This new expression is a type promotion to unsigned long...
> 
>>   #define GUEST_ADDR_MAX    (reserved_va ? reserved_va - 1 : GUEST_ADDR_MAX_)
> 
> ... which is probably ok, since it would be done here anyway.
> But I did wonder why the change.

Because:

#if MIN(...)

now fails to compile (you can't have { in a preprocessor expression), and:

#if MIN_CONST(...)

fails to compile (__builtin_constant_p() is not a preprocessor macro, so 
it warns that it is being treated as 0).  The only fix is to move the 
MIN() out of the #if and into the #define.

> 
>> +/*
>> + * Two variations of MIN/MAX macros. The first is for runtime use, and
>> + * evaluates arguments only once (so it is safe even with side
>> + * effects), but will not work in constant contexts (such as array
>> + * size declarations).  The second is for compile-time use, where
>> + * evaluating arguments twice is safe because the result is going to
>> + * be constant anyway.
>> + */
>> +#undef MIN
>> +#define MIN(a, b)                                       \
>> +    ({                                                  \
>> +        typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
>> +        _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())
> 
> Is it possible to use qemu_build_not_reached?

Possibly.

/me goes and recompiles; touching osdep.h recompiles the world...

No, it blows up hard, because qemu_build_not_reached() is not embeddable 
in an expression:

In file included from /usr/lib64/glib-2.0/include/glibconfig.h:9,
                  from /usr/include/glib-2.0/glib/gtypes.h:32,
                  from /usr/include/glib-2.0/glib/galloca.h:32,
                  from /usr/include/glib-2.0/glib.h:30,
                  from /home/eblake/qemu/include/glib-compat.h:32,
                  from /home/eblake/qemu/include/qemu/osdep.h:126,
                  from /home/eblake/qemu/qemu-io-cmds.c:11:
/home/eblake/qemu/qemu-io-cmds.c: In function ‘create_iovec’:
/usr/include/glib-2.0/glib/gmacros.h:854:23: error: expected expression 
before ‘
do’
   854 | #define G_STMT_START  do
       |                       ^~
/usr/include/glib-2.0/glib/gtestutils.h:168:41: note: in expansion of 
macro ‘G_STMT_START’
   168 | #define g_assert_not_reached()          G_STMT_START { 
g_assertion_message_expr (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, 
NULL); } G_STMT_END
       |                                         ^~~~~~~~~~~~
/home/eblake/qemu/include/qemu/compiler.h:243:35: note: in expansion of 
macro ‘g_assert_not_reached’
   243 | #define qemu_build_not_reached()  g_assert_not_reached()
       |                                   ^~~~~~~~~~~~~~~~~~~~
/home/eblake/qemu/include/qemu/osdep.h:257:9: note: in expansion of 
macro ‘qemu_build_not_reached’
   257 |         qemu_build_not_reached())
       |         ^~~~~~~~~~~~~~~~~~~~~~
/home/eblake/qemu/include/block/block.h:136:34: note: in expansion of 
macro ‘MIN_CONST’
   136 | #define BDRV_REQUEST_MAX_SECTORS MIN_CONST(SIZE_MAX >> 
BDRV_SECTOR_BITS, \
       |                                  ^~~~~~~~~
/home/eblake/qemu/include/block/block.h:138:33: note: in expansion of 
macro ‘BDRV_REQUEST_MAX_SECTORS’
   138 | #define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << 
BDRV_SECTOR_BITS)
       |                                 ^~~~~~~~~~~~~~~~~~~~~~~~
/home/eblake/qemu/qemu-io-cmds.c:500:19: note: in expansion of macro 
‘BDRV_REQUEST_MAX_BYTES’
   500 |         if (len > BDRV_REQUEST_MAX_BYTES) {
       |                   ^~~~~~~~~~~~~~~~~~~~~~
/home/eblake/qemu/qemu-io-cmds.c:500:41: error: expected statement 
before ‘)’ token
   500 |         if (len > BDRV_REQUEST_MAX_BYTES) {
       |                                         ^


> 
> I'd prefer we generate a compile-time error than a runtime trap (or nothing,
> depending on compiler flags controlling __builtin_unreachable).

What we have DOES produce a compile-time error.  If either expression to 
MIN_CONST() is not actually const, the fact that __builtin_unreachable() 
returns void causes a compilation failure because a value is expected.

With __builtin_unreachable(), there is no error when MIN_CONST is used 
correctly, and when used incorrectly, the commit message mentions the error:


Use of MIN_CONST when MIN is needed:

/home/eblake/qemu/qemu-img.c: In function ‘is_allocated_sectors’:
/home/eblake/qemu/qemu-img.c:1225:15: error: void value not ignored as 
it ought to be
  1225 |             i = MIN_CONST(i, n);
       |               ^


> 
>> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
>> index 42ce1dfcff77..d77add79b218 100644
>> --- a/accel/tcg/translate-all.c
>> +++ b/accel/tcg/translate-all.c
>> @@ -2565,9 +2565,9 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
>>       /* This function should never be called with addresses outside the
>>          guest address space.  If this assert fires, it probably indicates
>>          a missing call to h2g_valid.  */
>> -#if TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS
>> -    assert(start < ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
>> -#endif
>> +    if (TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS) {
>> +        assert(start < ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
>> +    }
> 
> IIRC the ifdef is required for clang warnings vs the shift.
> Have you tested that?

I have not yet tested with clang.  We'll see if the CLI bots get to that 
before I do...  But if clang isn't happy, I may have to introduce yet a 
third macro, MIN_PP, safe for use in preprocessor statements.

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