Hi,
I know this was committed some time ago, but I have a
comment about this patch.
On Sat, Jan 5, 2013 at 1:39 AM, Richard Henderson <r...@twiddle.net> wrote:
> The previous formuation with multiple assignments to __typeof(*hptr) falls
> down when hptr is qualified const. E.g. with const struct S *p, p->f is
> also qualified const.
>
> With this formulation, there's no assignment to any local variable.
>
> Signed-off-by: Richard Henderson <r...@twiddle.net>
> ---
> linux-user/qemu.h | 63
> +++++++++++++++++++++++++++++--------------------------
> 1 file changed, 33 insertions(+), 30 deletions(-)
>
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 8a3538c..31a220a 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -287,36 +287,39 @@ static inline int access_ok(int type, abi_ulong addr,
> abi_ulong size)
> (type == VERIFY_READ) ? PAGE_READ : (PAGE_READ |
> PAGE_WRITE)) == 0;
> }
>
> -/* NOTE __get_user and __put_user use host pointers and don't check access.
> */
> -/* These are usually used to access struct data members once the
> - * struct has been locked - usually with lock_user_struct().
> - */
> -#define __put_user(x, hptr)\
> -({ __typeof(*hptr) pu_ = (x);\
> - switch(sizeof(*hptr)) {\
> - case 1: break;\
> - case 2: pu_ = tswap16(pu_); break; \
> - case 4: pu_ = tswap32(pu_); break; \
> - case 8: pu_ = tswap64(pu_); break; \
> - default: abort();\
> - }\
> - memcpy(hptr, &pu_, sizeof(pu_)); \
> - 0;\
> -})
> -
> -#define __get_user(x, hptr) \
> -({ __typeof(*hptr) gu_; \
> - memcpy(&gu_, hptr, sizeof(gu_)); \
> - switch(sizeof(*hptr)) {\
> - case 1: break; \
> - case 2: gu_ = tswap16(gu_); break; \
> - case 4: gu_ = tswap32(gu_); break; \
> - case 8: gu_ = tswap64(gu_); break; \
> - default: abort();\
> - }\
> - (x) = gu_; \
> - 0;\
> -})
> +/* NOTE __get_user and __put_user use host pointers and don't check access.
> + These are usually used to access struct data members once the struct has
> + been locked - usually with lock_user_struct. */
> +
> +/* Tricky points:
> + - Use __builtin_choose_expr to avoid type promotion from ?:,
> + - Invalid sizes result in a compile time error stemming from
> + the fact that abort has no parameters.
> + - It's easier to use the endian-specific unaligned load/store
> + functions than host-endian unaligned load/store plus tswapN. */
> +
> +#define __put_user_e(x, hptr, e) \
> + (__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)), 0)
> +
> +#define __get_user_e(x, hptr, e) \
> + ((x) = \
> + __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), 0)
For 8- and 16-bit quantities the load is explicitly unsigned
through the use of ldub and lduw. But for 32-bit, ldl_[bl]e_p
return an int, so if x is a 64-bit variable sign-extension will
happen. I'm not sure this is desirable, for instance when
using get_user_u32 which makes one think the result is an
unsigned 32-bit value. Shouldn't ldul*_p functions be added
and used in __get_user_e?
Note I found this in private code, but wonder if some public
code isn't affected by this.
Thanks,
Laurent
> +
> +#ifdef TARGET_WORDS_BIGENDIAN
> +# 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 */
> /* These are usually used to access an atomic data type, such as an int,
> --
> 1.7.11.7
>
>
