Re: [PATCH v2 01/42] i386: pcmpestr 64-bit sign extension bug

[Paolo Bonzini <pbonzini@redhat.com>]

On 4/25/22 00:01, Paul Brook wrote:
> The abs1 function in ops_sse.h only works sorrectly when the result fits
> in a signed int. This is fine most of the time because we're only dealing
> with byte sized values.
> 
> However pcmp_elen helper function uses abs1 to calculate the absolute value
> of a cpu register. This incorrectly truncates to 32 bits, and will give
> the wrong anser for the most negative value.
> 
> Fix by open coding the saturation check before taking the absolute value.
> 
> Signed-off-by: Paul Brook <paul@nowt.org>

Queued, thanks.

Paolo

> ---
>   target/i386/ops_sse.h | 20 +++++++++-----------
>   1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
> index e4d74b814a..535440f882 100644
> --- a/target/i386/ops_sse.h
> +++ b/target/i386/ops_sse.h
> @@ -2011,25 +2011,23 @@ SSE_HELPER_Q(helper_pcmpgtq, FCMPGTQ)
>   
>   static inline int pcmp_elen(CPUX86State *env, int reg, uint32_t ctrl)
>   {
> -    int val;
> +    target_long val, limit;
>   
>       /* Presence of REX.W is indicated by a bit higher than 7 set */
>       if (ctrl >> 8) {
> -        val = abs1((int64_t)env->regs[reg]);
> +        val = (target_long)env->regs[reg];
>       } else {
> -        val = abs1((int32_t)env->regs[reg]);
> +        val = (int32_t)env->regs[reg];
>       }
> -
>       if (ctrl & 1) {
> -        if (val > 8) {
> -            return 8;
> -        }
> +        limit = 8;
>       } else {
> -        if (val > 16) {
> -            return 16;
> -        }
> +        limit = 16;
>       }
> -    return val;
> +    if ((val > limit) || (val < -limit)) {
> +        return limit;
> +    }
> +    return abs1(val);
>   }
>   
>   static inline int pcmp_ilen(Reg *r, uint8_t ctrl)