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

[Paolo Bonzini]

On 21/02/20 04:45, address@hidden 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.

>  static inline void fpop(CPUX86State *env)
>  {
> -    env->fptags[env->fpstt] = 1; /* invalidate stack entry */
> -    env->fpstt = (env->fpstt + 1) & 7;
> +    set_fpstt(env, env->fpstt + 1, true, true);

While here:

    env->foverflow = true;
    env->fptags[7] = 1;
    env->fpstt = 0;

>  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.

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;
+    }
+
     expdif = EXPD(temp);
     if (expdif == MAXEXPD) {
         if (MANTD(temp) == 0x8000000000000000ULL) {

Paolo