| Message ID | 20230808060815.9001-4-kariem.taha2.7@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Implement the stat system calls for FreeBSD. | expand |
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~
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~
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;