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

[Dr. David Alan Gilbert]

* Eric Blake (address@hidden) wrote:
> 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.

Ah, I didn't understand the context in your message;  you say 'even in
the dead branch of a __builtin_choose_expr()' but the following works
for me (on f29 and rhel7):

#include <stdio.h>

# define QEMU_TYPEOF(a) typeof(a)

#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;                   \
    }))


int main(int argc, char *argv[])
{
    int anarray[DMIN(5, 10)];
    int a=5;
    int b=6;
    fprintf(stderr, "sizeof(anarray)=%zd DMIN(a,b)=%d\n", sizeof(anarray), 
DMIN(a++,b++));
    fprintf(stderr, "a=%d b=%d\n", a,b);
}

Dave

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



--
Dr. David Alan Gilbert / address@hidden / Manchester, UK