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

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

[Eric Blake]

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

 /* 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 */
-- 
2.20.1

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190104153951.32306-1-eblake@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-quick@centos7 SHOW_ENV=1 J=8
=== TEST SCRIPT END ===

libpmem support   no
libudev           no

WARNING: Use of SDL 1.2 is deprecated and will be removed in
WARNING: future releases. Please switch to using SDL 2.0

NOTE: cross-compilers enabled:  'cc'
  GEN     x86_64-softmmu/config-devices.mak.tmp
---
  CC      block/commit.o
In file included from /tmp/qemu-test/src/block/block-backend.c:13:0:
/tmp/qemu-test/src/block/block-backend.c: In function 'blk_get_max_transfer':
/tmp/qemu-test/src/include/qemu/osdep.h:257:9: error: unknown type name '__auto_type'
         __auto_type _a = (a);                           \
         ^
/tmp/qemu-test/src/block/block-backend.c:1817:12: note: in expansion of macro 'MIN_NON_ZERO'
     return MIN_NON_ZERO(max, INT_MAX);
            ^
/tmp/qemu-test/src/include/qemu/osdep.h:258:9: error: unknown type name '__auto_type'
         __auto_type _b = (b);                           \
         ^
/tmp/qemu-test/src/block/block-backend.c:1817:12: note: in expansion of macro 'MIN_NON_ZERO'
     return MIN_NON_ZERO(max, INT_MAX);
            ^
/tmp/qemu-test/src/block/block-backend.c: At top level:
cc1: error: unrecognized command line option "-Wno-format-truncation" [-Werror]
cc1: all warnings being treated as errors
make: *** [block/block-backend.o] Error 1
make: *** Waiting for unfinished jobs....


The full log is available at
http://patchew.org/logs/20190104153951.32306-1-eblake@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[Zoltán Kővágó]

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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3935 bytes --]

On 1/4/19 6:23 PM, Zoltán Kővágó wrote:
> Hi,
> 
> I have a similar patch in my queue[1]
> 

Sorry for not noticing that thread.


>>  #ifndef MIN
>> -#define MIN(a, b) (((a) < (b)) ? (a) : (b))

The old version is at least named in ALL_CAPS, to warn the user that it
is a macro and may have side effects due to multiple evaluations.

>> +#define MIN(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.

Indeed; changing

#ifndef MIN

to

#undef MIN

forces us to use our version rather than inheriting something, at which
point...

> 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).

Yeah, I ran into that on libvirt as well.  It's a real bummer that
__auto_type can't be used in constant expressions; I don't know if C11
generics can be used to come up with an alternative that still qualifies
as a constant expression.

Quoting your other mail:

>> /home/dirty_ice/qemu/include/qemu/osdep.h:240:5: error: ‘__auto_type’
>> used with a bit-field initializer
>> /home/dirty_ice/qemu/include/qemu/osdep.h:247:35: error: braced-group
>> within expression allowed only inside a function
>> 
>> The first one is fixable with an explicit cast (ugly but works),

Or by writing:

__auto_type _a = (a) + 0;
typeof((a) + 0) _a = (a) + 0;

to force type promotion of the bitfield 'a' (no casts needed, either at
the caller or in the macro).  Since we are doing ?: between a and b, we
end up with integer promotion anyways (that is, MIN(char, char) still
results in int, and that's unchanged regardless of naive or smart
implementation of the macro; the only way to get MIN(char, char) to
result in char is with typeof, although I don't see any strong reasons
why making the macro return an unpromoted value would ever be useful).

>> but the
>> second one is more problematic. It means we can't write stuff like
>> 
>> USBPort  uports[MAX(MAXPORTS_2, MAXPORTS_3)];
>> 
>> when not in a function. So we either need a dumb version of MIN/MAX, or
>> scrape the idea altogether.

I'm fine keeping the name MIN/MAX for the common use, but we'd need a
second pair, maybe named MIN_CONST/MAX_CONST, for use in contexts that
require a constant (there, both arguments are evaluated twice because it
is the naive implementation, but that is harmless because both arguments
are constant and the macro is then usable in places where the smart
MIN/MAX are not).  I don't know if trying to use __builtin_constant_p in
the const version would also be worthwhile.  In fact, if
__builtin_constant_p is powerful enough, perhaps we could use it to
force a single macro to expand to the naive version if both arguments
are constant and the smart version otherwise.  I'll give that a shot.

> 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.

No, if we want smart MIN/MAX, we merely undefine whatever random junk
got inherited from the system.  If we rename anything, it should be the
constant version for use where the smart version is semantically prohibited.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]

On 1/5/19 7:48 AM, Eric Blake wrote:

> I'm fine keeping the name MIN/MAX for the common use, but we'd need a
> second pair, maybe named MIN_CONST/MAX_CONST, for use in contexts that
> require a constant (there, both arguments are evaluated twice because it
> is the naive implementation, but that is harmless because both arguments
> are constant and the macro is then usable in places where the smart
> MIN/MAX are not).  I don't know if trying to use __builtin_constant_p in
> the const version would also be worthwhile.  In fact, if
> __builtin_constant_p is powerful enough, perhaps we could use it to
> force a single macro to expand to the naive version if both arguments
> are constant and the smart version otherwise.  I'll give that a shot.

Alas, even though __builtin_constant_p can let the compiler overlook
SOME cases of non-constant code (as in __builtin_constant_p(0 && foo())
returning 1), it is not powerful enough to ignore the dead branch:

$ cat foo.c
#define MIN(a, b)                                               \
    (__builtin_constant_p(a) && __builtin_constant_p(b) ?       \
     (a) < (b) ? (a) : (b) :                                    \
     ({                                                         \
         __auto_type _a = (a) + 0;                              \
         __auto_type _b = (b) + 0;                              \
         _a < _b ? _a : _b;                                     \
     }))

char x[MIN(1, 2)];

int
main(int argc, char **argv)
{
    return MIN(argc, 0);
}
$ gcc -o foo -Wall foo.c
foo.c:4:6: error: braced-group within expression allowed only inside a
function
      ({                                                         \
      ^
foo.c:10:8: note: in expansion of macro ‘MIN’
 char x[MIN(1, 2)];
        ^~~

If anyone can come up with a workaround for single-macro usage, be my
guest.  Otherwise, I'm going with the two-macro solution, with MIN/MAX
for common use, and MIN_CONST/MAX_CONST for constant-context use.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]