Re: [PULL 25/31] osdep: Make MIN/MAX evaluate arguments only once

[Daniel P . Berrangé]

On Wed, Jun 24, 2020 at 07:13:17AM -0500, Eric Blake wrote:
> On 6/24/20 5:50 AM, Paolo Bonzini wrote:
> > From: Eric Blake <eblake@redhat.com>
> > 
> > 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
> 
> evaluation
> 
> > Update the MIN/MAX macros to only evaluate their argument once at
> > runtime;
> 
> > 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’
> 
> UTF-8 mojibake in the commit message ;(
> 
> 
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > 
> > Message-Id: <20200604215236.2798244-1-eblake@redhat.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> 
> > +#define MIN_CONST(a, b)                                         \
> > +    __builtin_choose_expr(                                      \
> > +        __builtin_constant_p(a) && __builtin_constant_p(b),     \
> > +        (a) < (b) ? (a) : (b),                                  \
> > +        ((void)0))
> 
> This one is correct...
> 
> > +#undef MAX
> > +#define MAX(a, b)                                       \
> > +    ({                                                  \
> > +        typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
> > +        _a > _b ? _a : _b;                              \
> > +    })
> > +#define MAX_CONST(a, b)                                         \
> > +    __builtin_choose_expr(                                      \
> > +        __builtin_constant_p(a) && __builtin_constant_p(b),     \
> > +        (a) > (b) ? (a) : (b),                                  \
> > +        __builtin_unreachable())
> 
> ...but this one should also use ((void)0), to match the commit message.
> 
> > +
> > +/* Minimum function that returns zero only if both values are zero.
> >    * Intended for use with unsigned values only. */
> 
> And checkpatch complains about the formatting here.
> 
> Ah well.  I had all these things fixed in my tree, but failed to post a v5.
> Not worth holding up this pull request in isolation, but if there are any
> other build issues, I'll post a v5 of this patch, otherwise a followup.

FWIW, the current QEMU code defining MIN/MAX was a no-op, since they
were already defined by GLib in /usr/include/glib-2.0/glib/gmacros.h
which we get via glib.h

Now, the GLib impl shared the same theoretical flaw as the old QEMU
impl, but you said it wasn't a real problem right now.

So I'm wondering if the better option would be to remove the MIN/MAX
def from QEMU, and then submit a pull request to GLib to improve
their definition ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|