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

[Richard Henderson]

On 6/4/20 2:52 PM, Eric Blake wrote:
> I'm not aware of any immediate bugs in qemu where a second runtime
> evalution of the arguments to MIN() or MAX() causes a problem, but
> proactively preventing such abuse is easier than falling prey to an
> unintended case down the road.  At any rate, here's the conversation
> that sparked the current patch:
> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05718.html
> 
> Update the MIN/MAX macros to only evaluate their argument once at
> runtime; this uses typeof(1 ? (a) : (b)) to ensure that we are
> promoting the temporaries to the same type as the final comparison (we
> have to trigger type promotion, as typeof(bitfield) won't compile; and
> we can't use typeof((a) + (b)) or even typeof((a) + 0), as some of our
> uses of MAX are on void* pointers where such addition is undefined).
> 
> However, we are unable to work around gcc refusing to compile ({}) in
> a constant context (such as the array length of a static variable),
> even when only used in the dead branch of a __builtin_choose_expr(),
> so we have to provide a second macro pair MIN_CONST and MAX_CONST for
> use when both arguments are known to be compile-time constants and
> where the result must also be usable as a constant; this second form
> evaluates arguments multiple times but that doesn't matter for
> constants.  By using a void expression as the expansion if a
> non-constant is presented to this second form, we can enlist the
> compiler to ensure the double evaluation is not attempted on
> non-constants.
> 
> Alas, as both macros now rely on compiler intrinsics, they are no
> longer usable in preprocessor #if conditions; those will just have to
> be open-coded or the logic rewritten into #define or runtime 'if'
> conditions (but where the compiler dead-code-elimination will probably
> still apply).
> 
> I tested that both gcc 10.1.1 and clang 10.0.0 produce errors for all
> forms of macro mis-use.  As the errors can sometimes be cryptic, I'm
> demonstrating the gcc output:
> 
> Use of MIN when MIN_CONST is needed:
> 
> In file included from /home/eblake/qemu/qemu-img.c:25:
> /home/eblake/qemu/include/qemu/osdep.h:249:5: error: braced-group within 
> expression allowed only inside a function
>   249 |     ({                                                  \
>       |     ^
> /home/eblake/qemu/qemu-img.c:92:12: note: in expansion of macro ‘MIN’
>    92 | char array[MIN(1, 2)] = "";
>       |            ^~~
> 
> 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);
>       |               ^
> 
> Use of MIN in the preprocessor:
> 
> In file included from /home/eblake/qemu/accel/tcg/translate-all.c:20:
> /home/eblake/qemu/accel/tcg/translate-all.c: In function ‘page_check_range’:
> /home/eblake/qemu/include/qemu/osdep.h:249:6: error: token "{" is not valid 
> in preprocessor expressions
>   249 |     ({                                                  \
>       |      ^
> 
> Fix the resulting callsites that used #if or computed a compile-time
> constant min or max to use the new macros.  cpu-defs.h is interesting,
> as CPU_TLB_DYN_MAX_BITS is sometimes used as a constant and sometimes
> dynamic.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~