Re: [PATCH] target: i386: Check float overflow about register stack

[Chen Gang <chengang@emindsoft.com.cn>]

On 2020/2/21 下午4:58, Paolo Bonzini wrote:
> On 21/02/20 04:45, chengang@emindsoft.com.cn wrote:
>>  static inline void fpush(CPUX86State *env)
>>  {
>> -    env->fpstt = (env->fpstt - 1) & 7;
>> -    env->fptags[env->fpstt] = 0; /* validate stack entry */
>> +    set_fpstt(env, env->fpstt - 1, false, true);
> 
> On overflow fpstt is ~0, so this does:
> 
>     env->foverflow = true;
>     env->fpstt = 7;
>     env->fptags[7] = 0;      /* validate stack entry */
> 
> Is this correct?  You are going to set ST0 so the register should not be
> marked empty.
> 

Originally, I wanted to add foverflow to mark the stack overflow only,
and kept another things no touch.

But I think what you said above is correct, for me, if fpush/f[i]ld*_STO
are overflow, the env->fpstt, env->fpregs and env->fptags should be kept
no touch, and foverflow is set true, so there is no negative effect.

Welcome your idea.

>>  void helper_fdecstp(CPUX86State *env)
>>  {
>> -    env->fpstt = (env->fpstt - 1) & 7;
>> +    set_fpstt(env, env->fpstt - 1, false, false);
> 
> This is clearing env->foverflow.  But after 8 consecutive fdecstp or
> fincstp the result of FXAM should not change.
> 
>>      env->fpus &= ~0x4700;
>>  }
>>  
>>  void helper_fincstp(CPUX86State *env)
>>  {
>> -    env->fpstt = (env->fpstt + 1) & 7;
>> +    set_fpstt(env, env->fpstt + 1, true, false);
> 
> Same here.
> 

OK. thanks.

Now if foverflow is only for fpush/f[i]ld*_ST0, I guess fincstp/fdecstp
can clear foverflow. The env->fptags are only for fpop, which keep no
touch in fincstp/fdecstp.

> The actual bug is hinted in helper_fxam_ST0:
> 
>     /* XXX: test fptags too */
> 
> I think the correct fix should be something like
> 
> diff --git a/target/i386/fpu_helper.c b/target/i386/fpu_helper.c
> index 99f28f267f..792a128a6d 100644
> --- a/target/i386/fpu_helper.c
> +++ b/target/i386/fpu_helper.c
> @@ -991,7 +991,11 @@ void helper_fxam_ST0(CPUX86State *env)
>          env->fpus |= 0x200; /* C1 <-- 1 */
>      }
> 
> -    /* XXX: test fptags too */
> +    if (env->fptags[env->fpstt]) {
> +        env->fpus |= 0x4100; /* Empty */
> +        return;
> +    }
> +

For fpop overflow, this fix is enough, but for me, we still need
foverflow to check fpush/fld*_ST0 overflow.

Don't you think we need check fpush/f[i]ld*_ST0 overflow?

Thanks