qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eric Blake
Subject: Re: [PATCH v3] osdep: Make MIN/MAX evaluate arguments only once
Date: Tue, 2 Jun 2020 21:29:36 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0

On 6/2/20 9:07 PM, Richard Henderson wrote:
On 6/2/20 6:36 PM, Eric Blake wrote:
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -176,11 +176,9 @@ extern unsigned long reserved_va;
   * avoid setting bits at the top of guest addresses that might need
   * to be used for tags.
   */
-#if MIN(TARGET_VIRT_ADDR_SPACE_BITS, TARGET_ABI_BITS) <= 32
-# define GUEST_ADDR_MAX_  UINT32_MAX
-#else
-# define GUEST_ADDR_MAX_  (~0ul)
-#endif
+#define GUEST_ADDR_MAX_                                                 \
+    ((MIN_CONST(TARGET_VIRT_ADDR_SPACE_BITS, TARGET_ABI_BITS) <= 32) ?  \
+     UINT32_MAX : ~0ul)

This new expression is a type promotion to unsigned long...

  #define GUEST_ADDR_MAX    (reserved_va ? reserved_va - 1 : GUEST_ADDR_MAX_)

... which is probably ok, since it would be done here anyway.
But I did wonder why the change.

Because:

#if MIN(...)

now fails to compile (you can't have { in a preprocessor expression), and:

#if MIN_CONST(...)

fails to compile (__builtin_constant_p() is not a preprocessor macro, so it warns that it is being treated as 0). The only fix is to move the MIN() out of the #if and into the #define.


+/*
+ * Two variations of MIN/MAX macros. The first is for runtime use, and
+ * evaluates arguments only once (so it is safe even with side
+ * effects), but will not work in constant contexts (such as array
+ * size declarations).  The second is for compile-time use, where
+ * evaluating arguments twice is safe because the result is going to
+ * be constant anyway.
+ */
+#undef MIN
+#define MIN(a, b)                                       \
+    ({                                                  \
+        typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
+        _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())

Is it possible to use qemu_build_not_reached?

Possibly.

/me goes and recompiles; touching osdep.h recompiles the world...

No, it blows up hard, because qemu_build_not_reached() is not embeddable in an expression:

In file included from /usr/lib64/glib-2.0/include/glibconfig.h:9,
                 from /usr/include/glib-2.0/glib/gtypes.h:32,
                 from /usr/include/glib-2.0/glib/galloca.h:32,
                 from /usr/include/glib-2.0/glib.h:30,
                 from /home/eblake/qemu/include/glib-compat.h:32,
                 from /home/eblake/qemu/include/qemu/osdep.h:126,
                 from /home/eblake/qemu/qemu-io-cmds.c:11:
/home/eblake/qemu/qemu-io-cmds.c: In function ‘create_iovec’:
/usr/include/glib-2.0/glib/gmacros.h:854:23: error: expected expression before ‘
do’
  854 | #define G_STMT_START  do
      |                       ^~
/usr/include/glib-2.0/glib/gtestutils.h:168:41: note: in expansion of macro ‘G_STMT_START’ 168 | #define g_assert_not_reached() G_STMT_START { g_assertion_message_expr (G_LOG_DOMAIN, __FILE__, __LINE__, G_STRFUNC, NULL); } G_STMT_END
      |                                         ^~~~~~~~~~~~
/home/eblake/qemu/include/qemu/compiler.h:243:35: note: in expansion of macro ‘g_assert_not_reached’
  243 | #define qemu_build_not_reached()  g_assert_not_reached()
      |                                   ^~~~~~~~~~~~~~~~~~~~
/home/eblake/qemu/include/qemu/osdep.h:257:9: note: in expansion of macro ‘qemu_build_not_reached’
  257 |         qemu_build_not_reached())
      |         ^~~~~~~~~~~~~~~~~~~~~~
/home/eblake/qemu/include/block/block.h:136:34: note: in expansion of macro ‘MIN_CONST’ 136 | #define BDRV_REQUEST_MAX_SECTORS MIN_CONST(SIZE_MAX >> BDRV_SECTOR_BITS, \
      |                                  ^~~~~~~~~
/home/eblake/qemu/include/block/block.h:138:33: note: in expansion of macro ‘BDRV_REQUEST_MAX_SECTORS’ 138 | #define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
      |                                 ^~~~~~~~~~~~~~~~~~~~~~~~
/home/eblake/qemu/qemu-io-cmds.c:500:19: note: in expansion of macro ‘BDRV_REQUEST_MAX_BYTES’
  500 |         if (len > BDRV_REQUEST_MAX_BYTES) {
      |                   ^~~~~~~~~~~~~~~~~~~~~~
/home/eblake/qemu/qemu-io-cmds.c:500:41: error: expected statement before ‘)’ token
  500 |         if (len > BDRV_REQUEST_MAX_BYTES) {
      |                                         ^



I'd prefer we generate a compile-time error than a runtime trap (or nothing,
depending on compiler flags controlling __builtin_unreachable).

What we have DOES produce a compile-time error. If either expression to MIN_CONST() is not actually const, the fact that __builtin_unreachable() returns void causes a compilation failure because a value is expected.

With __builtin_unreachable(), there is no error when MIN_CONST is used correctly, and when used incorrectly, the commit message mentions the error:


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);
      |               ^



diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 42ce1dfcff77..d77add79b218 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2565,9 +2565,9 @@ int page_check_range(target_ulong start, target_ulong 
len, int flags)
      /* This function should never be called with addresses outside the
         guest address space.  If this assert fires, it probably indicates
         a missing call to h2g_valid.  */
-#if TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS
-    assert(start < ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
-#endif
+    if (TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS) {
+        assert(start < ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
+    }

IIRC the ifdef is required for clang warnings vs the shift.
Have you tested that?

I have not yet tested with clang. We'll see if the CLI bots get to that before I do... But if clang isn't happy, I may have to introduce yet a third macro, MIN_PP, safe for use in preprocessor statements.

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]