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~
