[for-1.4] linux-user: Restore cast to target type in get_user()

Commit 658f2dc97 accidentally dropped the cast to the target type of
the value loaded by get_user().  The most visible effect of this would
be that the sequence "uint64_t v; get_user_u32(v, addr)" would sign
extend the 32 bit loaded value into v rather than zero extending as
would be expected for a _u32 accessor.  Put the cast back again to
restore the old behaviour.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/qemu.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On Thu, Jan 31, 2013 at 1:50 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Commit 658f2dc97 accidentally dropped the cast to the target type of
> the value loaded by get_user().  The most visible effect of this would
> be that the sequence "uint64_t v; get_user_u32(v, addr)" would sign
> extend the 32 bit loaded value into v rather than zero extending as
> would be expected for a _u32 accessor.  Put the cast back again to
> restore the old behaviour.

This fixes the issue I mentioned a week ago, thanks.

Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com>


Laurent

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/qemu.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 31a220a..b10e957 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -306,12 +306,12 @@ static inline int access_ok(int type, abi_ulong addr, abi_ulong size)
>       ((hptr), (x)), 0)
>
>  #define __get_user_e(x, hptr, e)                                        \
> -  ((x) =                                                                \
> +  ((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), 0)
> +     (hptr)), 0)
>
>  #ifdef TARGET_WORDS_BIGENDIAN
>  # define __put_user(x, hptr)  __put_user_e(x, hptr, be)
> --
> 1.7.9.5
>

On 01/31/2013 04:50 AM, Peter Maydell wrote:
> Commit 658f2dc97 accidentally dropped the cast to the target type of
> the value loaded by get_user().  The most visible effect of this would
> be that the sequence "uint64_t v; get_user_u32(v, addr)" would sign
> extend the 32 bit loaded value into v rather than zero extending as
> would be expected for a _u32 accessor.  Put the cast back again to
> restore the old behaviour.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   linux-user/qemu.h |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>

I now recall that I'd intended to go audit the uses of ld[us][bw]_p
and change the return type to be the "natural" one, which would not
have had this surprise.  But then failed to actually do so.

In the meantime this patch will produce correct results.


r~

On 31 January 2013 16:22, Richard Henderson <rth@twiddle.net> wrote:
> On 01/31/2013 04:50 AM, Peter Maydell wrote:
>> Commit 658f2dc97 accidentally dropped the cast to the target type of
>> the value loaded by get_user().  The most visible effect of this would
>> be that the sequence "uint64_t v; get_user_u32(v, addr)" would sign
>> extend the 32 bit loaded value into v rather than zero extending as
>> would be expected for a _u32 accessor.  Put the cast back again to
>> restore the old behaviour.

> I now recall that I'd intended to go audit the uses of ld[us][bw]_p
> and change the return type to be the "natural" one, which would not
> have had this surprise.  But then failed to actually do so.

That audit would not have caught the actual problem case, which
doesn't involve byte or word loads at all, but 'long' [ie 32 bit]
ones. The byte and word cases in __get_user() work OK because
the ld[us][bw]_p implementations return an appropriately sign
or zero extended value; but there is no signed/unsigned distinction
for ldl_p and so the caller (ie __get_user) must use or cast
via the right type to get the sign/zero extension right.

-- PMM

Ping as a for-1.4 patch with reviews.
patchwork url: http://patchwork.ozlabs.org/patch/217186/

Blue, Anthony: could one of you apply this, please?

thanks
-- PMM

On 31 January 2013 12:50, Peter Maydell <peter.maydell@linaro.org> wrote:
> Commit 658f2dc97 accidentally dropped the cast to the target type of
> the value loaded by get_user().  The most visible effect of this would
> be that the sequence "uint64_t v; get_user_u32(v, addr)" would sign
> extend the 32 bit loaded value into v rather than zero extending as
> would be expected for a _u32 accessor.  Put the cast back again to
> restore the old behaviour.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/qemu.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 31a220a..b10e957 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -306,12 +306,12 @@ static inline int access_ok(int type, abi_ulong addr, abi_ulong size)
>       ((hptr), (x)), 0)
>
>  #define __get_user_e(x, hptr, e)                                        \
> -  ((x) =                                                                \
> +  ((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), 0)
> +     (hptr)), 0)
>
>  #ifdef TARGET_WORDS_BIGENDIAN
>  # define __put_user(x, hptr)  __put_user_e(x, hptr, be)
> --
> 1.7.9.5

Applied.  Thanks.

Regards,

Anthony Liguori