Re: [PATCH 03/33] Update the definitions of __put_user and __get_user macros

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

On 8/7/23 23:07, Karim Taha wrote:
> From: Warner Losh <imp@bsdimp.com>
> 
> Use __builtin_choose_expr to avoid type promotion from ?:
> in __put_user_e and __get_user_e macros.
> Copied from linux-user/qemu.h, originally by Blue Swirl.
> 
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> Signed-off-by: Karim Taha <kariem.taha2.7@gmail.com>
> ---
>   bsd-user/qemu.h   | 81 ++++++++++++++++++++---------------------------
>   bsd-user/signal.c |  5 +--
>   2 files changed, 35 insertions(+), 51 deletions(-)
> 
> diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> index dfdfa8dd67..c41ebfe937 100644
> --- a/bsd-user/qemu.h
> +++ b/bsd-user/qemu.h
> @@ -307,50 +307,37 @@ static inline bool access_ok(int type, abi_ulong addr, abi_ulong size)
>   #define PRAGMA_REENABLE_PACKED_WARNING
>   #endif
>   
> -#define __put_user(x, hptr)\
> -({\
> -    int size = sizeof(*hptr);\
> -    switch (size) {\
> -    case 1:\
> -        *(uint8_t *)(hptr) = (uint8_t)(typeof(*hptr))(x);\
> -        break;\
> -    case 2:\
> -        *(uint16_t *)(hptr) = tswap16((typeof(*hptr))(x));\
> -        break;\
> -    case 4:\
> -        *(uint32_t *)(hptr) = tswap32((typeof(*hptr))(x));\
> -        break;\
> -    case 8:\
> -        *(uint64_t *)(hptr) = tswap64((typeof(*hptr))(x));\
> -        break;\
> -    default:\
> -        abort();\
> -    } \
> -    0;\
> -})
> +#define __put_user_e(x, hptr, e)                                            \
> +    do {                                                                    \
> +        PRAGMA_DISABLE_PACKED_WARNING;                                      \
> +        (__builtin_choose_expr(sizeof(*(hptr)) == 1, stb_p,                 \
> +        __builtin_choose_expr(sizeof(*(hptr)) == 2, stw_##e##_p,            \
> +        __builtin_choose_expr(sizeof(*(hptr)) == 4, stl_##e##_p,            \
> +        __builtin_choose_expr(sizeof(*(hptr)) == 8, stq_##e##_p, abort))))  \
> +            ((hptr), (x)), (void)0);                                        \
> +        PRAGMA_REENABLE_PACKED_WARNING;                                     \
> +    } while (0)
> +
> +#define __get_user_e(x, hptr, e)                                            \
> +    do {                                                                    \
> +        PRAGMA_DISABLE_PACKED_WARNING;                                      \
> +        ((x) = (typeof(*hptr))(                                             \
> +        __builtin_choose_expr(sizeof(*(hptr)) == 1, ldub_p,                 \
> +        __builtin_choose_expr(sizeof(*(hptr)) == 2, lduw_##e##_p,           \
> +        __builtin_choose_expr(sizeof(*(hptr)) == 4, ldl_##e##_p,            \
> +        __builtin_choose_expr(sizeof(*(hptr)) == 8, ldq_##e##_p, abort))))  \
> +            (hptr)), (void)0);                                              \
> +        PRAGMA_REENABLE_PACKED_WARNING;                                     \
> +    } while (0)

Hmm.  I guess this works.  The typeof cast in __get_user_e being required when sizeof(x) > 
sizeof(*hptr) in order to get the correct extension.

Is it clearer with _Generic?

     (x) = _Generic(*(hptr),
                    int8_t: *(int8_t *)(hptr),
                    uint8_t: *(uint8_t *)(hptr),
                    int16_t: (int16_t)lduw_##e##_p(hptr),
                    uint16_t: lduw_##e##_p(hptr),
                    int32_t: (int32_t)ldl_##e##_p(hptr),
                    uint32_t: (uint32_t)ldl_##e##_p(hptr),
                    int64_t: (int64_t)ldq_##e##_p(hptr),
                    uint64_t: ldq_##e##_p(hptr));

In particular I believe the error message will be much prettier.

Either way,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~