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

From: "Zoltán Kővágó" <dirty.ice.hu@gmail.com>
To: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [RFC PATCH] osdep: Make MIN/MAX evaluate arguments only once
Date: Sat, 5 Jan 2019 01:23:38 +0100	[thread overview]
Message-ID: <4e30c442-6ddc-2988-160e-168dec552966@gmail.com> (raw)
In-Reply-To: <20190104153951.32306-1-eblake@redhat.com>

Hi,

I have a similar patch in my queue[1]

On 2019-01-04 16:39, Eric Blake wrote:
> Use the __auto_type keyword to make sure our min/max macros only
> evaluate their arguments once.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> RFC because __auto_type didn't exist until gcc 4.9, and I don't know
> which clang version introduced it (other than that it went in
> during 2015: https://reviews.llvm.org/D12686).  Our minimum gcc
> version is 4.8, which has typeof; I'm not sure if our minimum clang
> version supports typeof.
> 
> I'm considering adding a snippet to compiler.h that looks like:
> 
>  #if .... // new enough gcc/clang
>  #define QEMU_TYPEOF(a) __auto_type
>  #else
>  #define QEMU_TYPEOF(a) typeof(a)
>  #endif
> 
> at which point we could blindly use QEMU_TYPEOF(a)=(a) anywhere we
> need automatic typing, for the benefit of smaller macro expansion
> [and proper handling of VLA types, although I don't think we use
> those to care about that aspect of __auto_type] in newer compilers,
> while still getting automatic type deduction in older compilers for
> macros that want single evaluation, and where we've localized the
> version checks to one spot instead of everywhere.  But for that to
> work, again, I need to know whether typeof is supported in our
> minimum clang version, and how to properly spell the version check
> for clang on when to prefer __auto_type over typeof (at least I
> know how to spell it for gcc).
> 
> While at it, the comments to MIN_NON_ZERO() state that callers should
> only compare unsigned types; I suspect we don't actually obey that
> rule, but I also think the comment is over-strict - the macro works
> as long as both arguments are non-negative, and when called with a
> mix of signed and unsigned types, as long as the type promotion
> preserves the fact that the value is still non-negative.  But it
> might be interesting to add compile-time checking (or maybe runtime
> asserts) that the macro is indeed only used on non-negative values.
> 
>  include/qemu/osdep.h | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 3bf48bcdec0..b941572b808 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -233,17 +233,31 @@ extern int daemon(int, int);
>  #endif
> 
>  #ifndef MIN
> -#define MIN(a, b) (((a) < (b)) ? (a) : (b))
> +#define MIN(a, b)              \
> +    ({                         \
> +        __auto_type _a = (a);  \
> +        __auto_type _b = (b);  \
> +        _a < _b ? _a : _b;     \
> +    })
>  #endif
>  #ifndef MAX
> -#define MAX(a, b) (((a) > (b)) ? (a) : (b))
> +#define MAX(a, b)              \
> +    ({                         \
> +        __auto_type _a = (a);  \
> +        __auto_type _b = (b);  \
> +        _a > _b ? _a : _b;     \
> +    })
>  #endif
I tried this[2], but apparently random linux headers define their own
MIN/MAX and in case this version won't be used. And the version above
with __auto_type and statement expression doesn't work on bitfields and
when not in functions (for example struct XHCIState has
    USBPort  uports[MAX(MAXPORTS_2, MAXPORTS_3)];
as one of its member). It only works because currently glib/gmacros.h or
sys/param.h defines it's own MIN/MAX which is identical to the old version.

Now that I think about it, instead of undefining the old macro or only
conditionally defining it, maybe the best course of action would be to
rename MIN/MAX to something more unique so it won't clash with random
system headers.

> 
>  /* Minimum function that returns zero only iff both values are zero.
>   * Intended for use with unsigned values only. */
>  #ifndef MIN_NON_ZERO
> -#define MIN_NON_ZERO(a, b) ((a) == 0 ? (b) : \
> -                                ((b) == 0 ? (a) : (MIN(a, b))))
> +#define MIN_NON_ZERO(a, b)                              \
> +    ({                                                  \
> +        __auto_type _a = (a);                           \
> +        __auto_type _b = (b);                           \
> +        _a == 0 ? _b : (_b == 0 || _b > _a) ? _a : _b;  \
> +    })
>  #endif
> 
>  /* Round number down to multiple */
> 

[1]: https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05718.html
[2]: https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg06006.html

Regards,
Zoltan