Re: [PATCH 1/5] target/i386: implement special cases for fxtract

[Eduardo Habkost <ehabkost@redhat.com>]

On Thu, May 07, 2020 at 12:43:30AM +0000, Joseph Myers wrote:
> The implementation of the fxtract instruction treats all nonzero
> operands as normal numbers, so yielding incorrect results for invalid
> formats, infinities, NaNs and subnormal and pseudo-denormal operands.
> Implement appropriate handling of all those cases.
> 
> Signed-off-by: Joseph Myers <joseph@codesourcery.com>
> ---
>  target/i386/fpu_helper.c           |  25 +++++-
>  tests/tcg/i386/test-i386-fxtract.c | 120 +++++++++++++++++++++++++++++
>  2 files changed, 144 insertions(+), 1 deletion(-)
>  create mode 100644 tests/tcg/i386/test-i386-fxtract.c
> 
> diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c
> index 792a128a6d..71a696a863 100644
> --- a/target/i386/fpu_helper.c
> +++ b/target/i386/fpu_helper.c
> @@ -767,10 +767,33 @@ void helper_fxtract(CPUX86State *env)
>                             &env->fp_status);
>          fpush(env);
>          ST0 = temp.d;
> +    } else if (floatx80_invalid_encoding(ST0)) {
> +        float_raise(float_flag_invalid, &env->fp_status);
> +        ST0 = floatx80_default_nan(&env->fp_status);
> +        fpush(env);
> +        ST0 = ST1;
> +    } else if (floatx80_is_any_nan(ST0)) {
> +        if (floatx80_is_signaling_nan(ST0, &env->fp_status)) {
> +            float_raise(float_flag_invalid, &env->fp_status);
> +            ST0 = floatx80_silence_nan(ST0, &env->fp_status);
> +        }
> +        fpush(env);
> +        ST0 = ST1;
> +    } else if (floatx80_is_infinity(ST0)) {
> +        fpush(env);
> +        ST0 = ST1;
> +        ST1 = floatx80_infinity;
>      } else {
>          int expdif;
>  
> -        expdif = EXPD(temp) - EXPBIAS;
> +        if (EXPD(temp) == 0) {
> +            int shift = clz64(temp.l.lower);
> +            temp.l.lower <<= shift;

Coverity reports the following.  It looks like a false positive
because floatx80_is_zero() would be true if both EXPD(temp) and
temp.l.lower were zero, but maybe I'm missing something.

________________________________________________________________________________________________________
*** CID 1429970:  Integer handling issues  (BAD_SHIFT)
/target/i386/fpu_helper.c: 922 in helper_fxtract()
916             ST1 = floatx80_infinity;
917         } else {
918             int expdif;
919
920             if (EXPD(temp) == 0) {
921                 int shift = clz64(temp.l.lower);
>>>     CID 1429970:  Integer handling issues  (BAD_SHIFT)
>>>     In expression "temp.l.lower <<= shift", left shifting by more than 63 bits has undefined behavior.  The shift amount, "shift", is 64.
922                 temp.l.lower <<= shift;
923                 expdif = 1 - EXPBIAS - shift;
924                 float_raise(float_flag_input_denormal, &env->fp_status);
925             } else {
926                 expdif = EXPD(temp) - EXPBIAS;
927             }



> +            expdif = 1 - EXPBIAS - shift;
> +            float_raise(float_flag_input_denormal, &env->fp_status);
> +        } else {
> +            expdif = EXPD(temp) - EXPBIAS;
> +        }
>          /* DP exponent bias */
>          ST0 = int32_to_floatx80(expdif, &env->fp_status);
>          fpush(env);

-- 
Eduardo