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

[Philippe Mathieu-Daudé]

On 6/25/20 6:26 PM, Eric Blake wrote:
> I'm not aware of any immediate bugs in qemu where a second runtime
> evaluation 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.
> 
> It may be worth improving glib's MIN/MAX definitions to be saner, but
> that is a task for another day.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
>

Nice! (btw why extra line after s-o-b?)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
> 
> v2 was: https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg00727.html
> v3: avoid __auto_type [Richard], document other approaches that fail
> [Dave], rebase to master
> v4: use ((void)0) instead of __builtin_unreachable [Dave], update comments
> v5: fix typo, comment formatting, one more __builtin_unreachable
> ---
>  hw/usb/hcd-xhci.h         |  2 +-
>  include/block/block.h     |  4 +--
>  include/exec/cpu-all.h    |  8 +++---
>  include/exec/cpu-defs.h   |  7 ++++-
>  include/qemu/osdep.h      | 57 ++++++++++++++++++++++++++++++++-------
>  accel/tcg/translate-all.c |  6 ++---
>  migration/qemu-file.c     |  2 +-
>  7 files changed, 63 insertions(+), 23 deletions(-)