hw/usb/hcd-xhci.h | 2 +- include/block/block.h | 4 ++-- include/exec/cpu-all.h | 8 +++----- include/exec/cpu-defs.h | 7 ++++++- include/qemu/osdep.h | 43 +++++++++++++++++++++++++++++++-------- accel/tcg/translate-all.c | 6 +++--- migration/qemu-file.c | 2 +- 7 files changed, 51 insertions(+), 21 deletions(-)
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
proactively preventing such abuse is easier than falling prey to an
unintended case down the road. At any rate, here's the conversation
that sparked the current patch:
https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05718.html
Update the MIN/MAX macros to only evaluate their argument once at
runtime; this uses typeof(1 ? (a) : (b)) to ensure that we are
promoting the temporaries to the same type as the final comparison (we
have to trigger type promotion, as typeof(bitfield) won't compile; and
we can't use typeof((a) + (b)) or even typeof((a) + 0), as some of our
uses of MAX are on void* pointers where such addition is undefined).
However, we are unable to work around gcc refusing to compile ({}) in
a constant context, even when only used in the dead branch of a
__builtin_choose_expr(), so we have to provide a second macro pair
MIN_CONST and MAX_CONST for use when both arguments are known to be
compile-time constants and where the result must also be usable in
constant contexts.
Fix the resulting callsites that compute a compile-time constant min
or max to use the new macros. cpu-defs.h is interesting, as
CPU_TLB_DYN_MAX_BITS is sometimes used as a constant and sometimes
dynamic. Furthermore, use of either macro in #if now fails.
gcc makes it easy to know which macro to use, thanks to the following
compiler errors (even if they are cryptic):
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’
92 | char array[MIN(1, 2)] = "";
| ^~~
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);
| ^
Use of MIN in the preprocessor:
In file included from /home/eblake/qemu/accel/tcg/translate-all.c:20:
/home/eblake/qemu/accel/tcg/translate-all.c: In function ‘page_check_range’:
/home/eblake/qemu/include/qemu/osdep.h:249:6: error: token "{" is not valid in preprocessor expressions
249 | ({ \
| ^
Signed-off-by: Eric Blake <eblake@redhat.com>
---
I was visiting an old branch, and thought this one is still worth salvaging.
v2 was: https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg00727.html
v3: avoid __auto_type [Richard], document other approaches that fail
[Dave], rebase to master
---
hw/usb/hcd-xhci.h | 2 +-
include/block/block.h | 4 ++--
include/exec/cpu-all.h | 8 +++-----
include/exec/cpu-defs.h | 7 ++++++-
include/qemu/osdep.h | 43 +++++++++++++++++++++++++++++++--------
accel/tcg/translate-all.c | 6 +++---
migration/qemu-file.c | 2 +-
7 files changed, 51 insertions(+), 21 deletions(-)
diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
index 2fad4df2a704..946af51fc25d 100644
--- a/hw/usb/hcd-xhci.h
+++ b/hw/usb/hcd-xhci.h
@@ -214,7 +214,7 @@ struct XHCIState {
uint32_t dcbaap_high;
uint32_t config;
- USBPort uports[MAX(MAXPORTS_2, MAXPORTS_3)];
+ USBPort uports[MAX_CONST(MAXPORTS_2, MAXPORTS_3)];
XHCIPort ports[MAXPORTS];
XHCISlot slots[MAXSLOTS];
uint32_t numports;
diff --git a/include/block/block.h b/include/block/block.h
index 25e299605e19..e8fc8149967f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -133,8 +133,8 @@ typedef struct HDGeometry {
#define BDRV_SECTOR_BITS 9
#define BDRV_SECTOR_SIZE (1ULL << BDRV_SECTOR_BITS)
-#define BDRV_REQUEST_MAX_SECTORS MIN(SIZE_MAX >> BDRV_SECTOR_BITS, \
- INT_MAX >> BDRV_SECTOR_BITS)
+#define BDRV_REQUEST_MAX_SECTORS MIN_CONST(SIZE_MAX >> BDRV_SECTOR_BITS, \
+ INT_MAX >> BDRV_SECTOR_BITS)
#define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
/*
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index d14374bdd499..291454fac12e 100644
--- 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)
#define GUEST_ADDR_MAX (reserved_va ? reserved_va - 1 : GUEST_ADDR_MAX_)
#else
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index 8c44abefa22a..918563233797 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -102,8 +102,13 @@ typedef uint64_t target_ulong;
* Skylake's Level-2 STLB has 16 1G entries.
* Also, make sure we do not size the TLB past the guest's address space.
*/
-# define CPU_TLB_DYN_MAX_BITS \
+# ifdef TARGET_PAGE_BITS_VARY
+# define CPU_TLB_DYN_MAX_BITS \
MIN(22, TARGET_VIRT_ADDR_SPACE_BITS - TARGET_PAGE_BITS)
+# else
+# define CPU_TLB_DYN_MAX_BITS \
+ MIN_CONST(22, TARGET_VIRT_ADDR_SPACE_BITS - TARGET_PAGE_BITS)
+# endif
# endif
typedef struct CPUTLBEntry {
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index ff7c17b85735..1f002f035279 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -236,18 +236,45 @@ extern int daemon(int, int);
#define SIZE_MAX ((size_t)-1)
#endif
-#ifndef MIN
-#define MIN(a, b) (((a) < (b)) ? (a) : (b))
-#endif
-#ifndef MAX
-#define MAX(a, b) (((a) > (b)) ? (a) : (b))
-#endif
+/*
+ * 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())
+#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())
/* 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) \
+ ({ \
+ typeof(1 ? (a) : (b)) _a = (a), _b = (b); \
+ _a == 0 ? _b : (_b == 0 || _b > _a) ? _a : _b; \
+ })
#endif
/* Round number down to multiple */
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));
+ }
if (len == 0) {
return 0;
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 1c3a358a140d..be21518c5708 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -31,7 +31,7 @@
#include "qapi/error.h"
#define IO_BUF_SIZE 32768
-#define MAX_IOV_SIZE MIN(IOV_MAX, 64)
+#define MAX_IOV_SIZE MIN_CONST(IOV_MAX, 64)
struct QEMUFile {
const QEMUFileOps *ops;
--
2.27.0
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.
> +/*
> + * 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?
I'd prefer we generate a compile-time error than a runtime trap (or nothing,
depending on compiler flags controlling __builtin_unreachable).
> 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?
r~
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
On 6/2/20 7:29 PM, Eric Blake wrote:
> 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.
Ah, right. Thanks.
>> 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:
Ah, right, because without -O, qemu_build_not_reached expands to
g_assert_not_reached and not to a symbol marked with QEMU_ERROR.
>> 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.
Ah! Well, that's good and certainly sufficient for my needs.
I do now wonder if it wouldn't be clearer to use "(void)0"
instead of __builtin_unreachable, and add a note to the comment just above.
r~
On 6/3/20 11:32 AM, Richard Henderson wrote:
>>> 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.
>
> Ah! Well, that's good and certainly sufficient for my needs.
>
> I do now wonder if it wouldn't be clearer to use "(void)0"
> instead of __builtin_unreachable, and add a note to the comment just above.
Yes, I just tested; using "((void)0)" in place of
"__builtin_unreachable()" has the same effect (no change to valid use,
and still a compiler error on misuse). gcc:
/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);
| ^
clang:
/home/eblake/qemu/qemu-img.c:1225:15: error: assigning to 'int' from
incompatible type 'void'
i = MIN_CONST(i, n);
^ ~~~~~~~~~~~~~~~
Of course, a comment explaining the intent can't hurt either. I'll wait
to see if this gathers any other comments before spinning a v4 with that
change.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 03/06/20 21:26, Eric Blake wrote: > > Yes, I just tested; using "((void)0)" in place of > "__builtin_unreachable()" has the same effect (no change to valid use, > and still a compiler error on misuse). gcc: > > /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); > | ^ > > clang: > > /home/eblake/qemu/qemu-img.c:1225:15: error: assigning to 'int' from > incompatible type 'void' > i = MIN_CONST(i, n); > ^ ~~~~~~~~~~~~~~~ > > > Of course, a comment explaining the intent can't hurt either. I'll wait > to see if this gathers any other comments before spinning a v4 with that > change. Please go ahead and send v4. Thanks, Paolo
On 6/2/20 9:29 PM, Eric Blake wrote:
>>> +++ 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.
I've now run a clang build over the entire tree (using clang 10.0.0 from
Fedora 32, which required other pending patches mentioned on the list to
work around unrelated warnings), the entire tree built without issue.
So at least one version of clang compiled my rewrite of this hunk just
fine, although it does not rule out what older versions might do.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
© 2016 - 2024 Red Hat, Inc.