[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