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

[Philippe Mathieu-Daudé]

On Thu, Jun 25, 2020 at 6:36 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
> 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>

Tested-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(-)