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

[Richard Henderson <richard.henderson@linaro.org>]

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~