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)
-#define __get_user(x, hptr) \
-({\
- int size = sizeof(*hptr);\
- switch (size) {\
- case 1:\
- x = (typeof(*hptr))*(uint8_t *)(hptr);\
- break;\
- case 2:\
- x = (typeof(*hptr))tswap16(*(uint16_t *)(hptr));\
- break;\
- case 4:\
- x = (typeof(*hptr))tswap32(*(uint32_t *)(hptr));\
- break;\
- case 8:\
- x = (typeof(*hptr))tswap64(*(uint64_t *)(hptr));\
- break;\
- default:\
- x = 0;\
- abort();\
- } \
- 0;\
-})
+
+#if TARGET_BIG_ENDIAN
+# define __put_user(x, hptr) __put_user_e(x, hptr, be)
+# define __get_user(x, hptr) __get_user_e(x, hptr, be)
+#else
+# define __put_user(x, hptr) __put_user_e(x, hptr, le)
+# define __get_user(x, hptr) __get_user_e(x, hptr, le)
+#endif
/*
* put_user()/get_user() take a guest address and check access
@@ -363,10 +350,10 @@ static inline bool access_ok(int type, abi_ulong addr, abi_ulong size)
({ \
abi_ulong __gaddr = (gaddr); \
target_type *__hptr; \
- abi_long __ret; \
+ abi_long __ret = 0; \
__hptr = lock_user(VERIFY_WRITE, __gaddr, sizeof(target_type), 0); \
if (__hptr) { \
- __ret = __put_user((x), __hptr); \
+ __put_user((x), __hptr); \
unlock_user(__hptr, __gaddr, sizeof(target_type)); \
} else \
__ret = -TARGET_EFAULT; \
@@ -377,10 +364,10 @@ static inline bool access_ok(int type, abi_ulong addr, abi_ulong size)
({ \
abi_ulong __gaddr = (gaddr); \
target_type *__hptr; \
- abi_long __ret; \
+ abi_long __ret = 0; \
__hptr = lock_user(VERIFY_READ, __gaddr, sizeof(target_type), 1); \
if (__hptr) { \
- __ret = __get_user((x), __hptr); \
+ __get_user((x), __hptr); \
unlock_user(__hptr, __gaddr, 0); \
} else { \
(x) = 0; \
diff --git a/bsd-user/signal.c b/bsd-user/signal.c
index f4e078ee1d..4db85a3485 100644
--- a/bsd-user/signal.c
+++ b/bsd-user/signal.c
@@ -787,10 +787,7 @@ static int reset_signal_mask(target_ucontext_t *ucontext)
TaskState *ts = (TaskState *)thread_cpu->opaque;
for (i = 0; i < TARGET_NSIG_WORDS; i++) {
- if (__get_user(target_set.__bits[i],
- &ucontext->uc_sigmask.__bits[i])) {
- return -TARGET_EFAULT;
- }
+ __get_user(target_set.__bits[i], &ucontext->uc_sigmask.__bits[i]);
}
target_to_host_sigset_internal(&blocked, &target_set);
ts->signal_mask = blocked;
--
2.40.0On 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~
On Tue, Aug 8, 2023 at 3:03 PM Richard Henderson <
richard.henderson@linaro.org> wrote:
> 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.
>
This code was copied 100% from the current linux-user :) It was also copied
before I needed
to do _Generic for a different project for something as well, so I didn't
think to switch over
from the old-school, gcc-specific hack to the modern clean form.
> 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.
>
Indeed. That looks cleaner. I'll see if I can test that against the latest
bsd-user upstream.
Warner
> Either way,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
>
> r~
>
On 8/8/23 18:39, Warner Losh wrote:
> > +#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.
>
>
> This code was copied 100% from the current linux-user :)
Ha ha indeed! I should have known.
> 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.
>
>
> Indeed. That looks cleaner. I'll see if I can test that against the latest bsd-user upstream.
I'll see if we can share this code via common-user.
r~
On Tue, Aug 8, 2023 at 7:47 PM Richard Henderson <
richard.henderson@linaro.org> wrote:
> On 8/8/23 18:39, Warner Losh wrote:
> > > +#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.
> >
> >
> > This code was copied 100% from the current linux-user :)
>
> Ha ha indeed! I should have known.
>
Yea... It's old-school crazy, and I've done a lot of that in my day, but not
here :)
> > 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.
> >
> >
> > Indeed. That looks cleaner. I'll see if I can test that against the
> latest bsd-user upstream.
>
> I'll see if we can share this code via common-user.
>
It seems to work, though I still need the pragmas for clang 16 (see another
patch for that).
I tried to implement both get and put in terms of this, but found that it
broke the blitz
branch. So why don't we land this as is for bsd-user and then one of us can
try to
put it into common-user so as to not create too many waves for our GSoC
student Kariim.
There's already enough to fix in this series... Sound fair?
Warner
On 8/8/23 19:41, Warner Losh wrote: > I tried to implement both get and put in terms of this, but found that it broke the blitz > branch. So why don't we land this as is for bsd-user and then one of us can try to > put it into common-user so as to not create too many waves for our GSoC student Kariim. > There's already enough to fix in this series... Sound fair? Yes, that's fine. r~
© 2016 - 2026 Red Hat, Inc.