[Qemu-devel] [PATCH] MIPS: Correct branch-likely single-stepping

[Qemu-devel] [PATCH] MIPS: Correct branch-likely single-stepping

[Maciej W. Rozycki]

From: Nathan Froyd <froydnj@codesourcery.com>

 We have a problem with single-stepping branch-likely instructions.  
Here's Nathan's original note:

"[This] is a problem with single-stepping in QEMU: it manifests as
the program corrupting the register set--specifically the return
address--and going into an infinite loop.  The problem is that we were
not correctly saving state when single-stepping over branch likely
instructions.  In the program, we had this sequence:

  0x8000b328:  bnezl	v0,0x8000b318
  0x8000b32c:  lw	v0,0(s1)	# branch delay slot
  0x8000b330:  lw	ra,28(sp)

The cause of the problem was the QEMU sets a flag in its internal
translation state indicating that we had previously translated a branch
likely instruction.  When we generated the "skip over instruction" for a
not-taken branch, this flag was not correctly cleared for the beginning
of the next translation block.  The result was that we skipped the
instruction at 0x8000b32c (good) *and* the instruction at 0x8000b330
(bad).  $ra therefore never got restored."

 I have verified the problem is still there, here's a relevant raw GDB 
session (addresses are different, but code is essentially the same):

(gdb) continue
Continuing.

Breakpoint 2, 0x8000b460 in __libc_init_array ()
4: /x $ra = 0x8000b460
2: x/i $pc
=> 0x8000b460 <__libc_init_array+124>:  sltu    v0,s0,s2
(gdb) stepi
0x8000b464 in __libc_init_array ()
4: /x $ra = 0x8000b460
2: x/i $pc
=> 0x8000b464 <__libc_init_array+128>:
    bnezl       v0,0x8000b454 <__libc_init_array+112>
   0x8000b468 <__libc_init_array+132>:  lw      v0,0(s1)
(gdb)
0x8000b46c in __libc_init_array ()
4: /x $ra = 0x8000b460
2: x/i $pc
=> 0x8000b46c <__libc_init_array+136>:  lw      ra,28(sp)
(gdb)
0x8000b470 in __libc_init_array ()
4: /x $ra = 0x8000b460
2: x/i $pc
=> 0x8000b470 <__libc_init_array+140>:  lw      s2,24(sp)
(gdb)

-- oops! -- $ra still the same!  Fixed with Nathan's change:

(gdb) continue
Continuing.

Breakpoint 2, 0x8000b460 in __libc_init_array ()
4: /x $ra = 0x8000b460
2: x/i $pc
=> 0x8000b460 <__libc_init_array+124>:  sltu    v0,s0,s2
(gdb) stepi
0x8000b464 in __libc_init_array ()
4: /x $ra = 0x8000b460
2: x/i $pc
=> 0x8000b464 <__libc_init_array+128>:
    bnezl       v0,0x8000b454 <__libc_init_array+112>
   0x8000b468 <__libc_init_array+132>:  lw      v0,0(s1)
(gdb)
0x8000b46c in __libc_init_array ()
4: /x $ra = 0x8000b460
2: x/i $pc
=> 0x8000b46c <__libc_init_array+136>:  lw      ra,28(sp)
(gdb)
0x8000b470 in __libc_init_array ()
4: /x $ra = 0x8000891c
2: x/i $pc
=> 0x8000b470 <__libc_init_array+140>:  lw      s2,24(sp)
(gdb)

Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
---

 Sent on behalf of Nathan, who's since left the company.  Please apply.

  Maciej

qemu-mips-blikely.diff
Index: qemu-git-trunk/target-mips/translate.c
===================================================================
--- qemu-git-trunk.orig/target-mips/translate.c	2012-06-04 05:02:44.015407154 +0100
+++ qemu-git-trunk/target-mips/translate.c	2012-06-04 05:02:45.355612652 +0100
@@ -11699,11 +11699,17 @@ static void decode_opc (CPUMIPSState *en
     /* Handle blikely not taken case */
     if ((ctx->hflags & MIPS_HFLAG_BMASK_BASE) == MIPS_HFLAG_BL) {
         int l1 = gen_new_label();
+        uint32_t saved_hflags;
 
         MIPS_DEBUG("blikely condition (" TARGET_FMT_lx ")", ctx->pc + 4);
         tcg_gen_brcondi_tl(TCG_COND_NE, bcond, 0, l1);
         tcg_gen_movi_i32(hflags, ctx->hflags & ~MIPS_HFLAG_BMASK);
+        /* Fake saving hflags so that gen_goto_tb doesn't overwrite the
+         * hflags we saved above.  */
+        saved_hflags = ctx->saved_hflags;
+        ctx->saved_hflags = ctx->hflags;
         gen_goto_tb(ctx, 1, ctx->pc + 4);
+        ctx->saved_hflags = saved_hflags;
         gen_set_label(l1);
     }
 

Re: [Qemu-devel] [PATCH] MIPS: Correct branch-likely single-stepping

[Richard Henderson]

On 2012-06-07 18:05, Maciej W. Rozycki wrote:
> From: Nathan Froyd <froydnj@codesourcery.com>
> 
>  We have a problem with single-stepping branch-likely instructions.  
> Here's Nathan's original note:
> 
> "[This] is a problem with single-stepping in QEMU: it manifests as
> the program corrupting the register set--specifically the return
> address--and going into an infinite loop.  The problem is that we were
> not correctly saving state when single-stepping over branch likely
> instructions.  In the program, we had this sequence:
> 
>   0x8000b328:  bnezl	v0,0x8000b318
>   0x8000b32c:  lw	v0,0(s1)	# branch delay slot
>   0x8000b330:  lw	ra,28(sp)
> 
> The cause of the problem was the QEMU sets a flag in its internal
> translation state indicating that we had previously translated a branch
> likely instruction.  When we generated the "skip over instruction" for a
> not-taken branch, this flag was not correctly cleared for the beginning
> of the next translation block.  The result was that we skipped the
> instruction at 0x8000b32c (good) *and* the instruction at 0x8000b330
> (bad).  $ra therefore never got restored."
> 
>  I have verified the problem is still there, here's a relevant raw GDB 
> session (addresses are different, but code is essentially the same):
> 
> (gdb) continue
> Continuing.
> 
> Breakpoint 2, 0x8000b460 in __libc_init_array ()
> 4: /x $ra = 0x8000b460
> 2: x/i $pc
> => 0x8000b460 <__libc_init_array+124>:  sltu    v0,s0,s2
> (gdb) stepi
> 0x8000b464 in __libc_init_array ()
> 4: /x $ra = 0x8000b460
> 2: x/i $pc
> => 0x8000b464 <__libc_init_array+128>:
>     bnezl       v0,0x8000b454 <__libc_init_array+112>
>    0x8000b468 <__libc_init_array+132>:  lw      v0,0(s1)
> (gdb)
> 0x8000b46c in __libc_init_array ()
> 4: /x $ra = 0x8000b460
> 2: x/i $pc
> => 0x8000b46c <__libc_init_array+136>:  lw      ra,28(sp)
> (gdb)
> 0x8000b470 in __libc_init_array ()
> 4: /x $ra = 0x8000b460
> 2: x/i $pc
> => 0x8000b470 <__libc_init_array+140>:  lw      s2,24(sp)
> (gdb)
> 
> -- oops! -- $ra still the same!  Fixed with Nathan's change:

What about this comment, from the main body of gen_intermediate_code_internal:

        /* Execute a branch and its delay slot as a single instruction.
           This is what GDB expects and is consistent with what the
           hardware does (e.g. if a delay slot instruction faults, the
           reported PC is the PC of the branch).  */
        if (env->singlestep_enabled && (ctx.hflags & MIPS_HFLAG_BMASK) == 0)
            break;

That suggests we should not have split the lw away from the bnezl in
the first place, and thus the fix should be elsewhere.

Even failing that, this

>          tcg_gen_movi_i32(hflags, ctx->hflags & ~MIPS_HFLAG_BMASK);
> +        /* Fake saving hflags so that gen_goto_tb doesn't overwrite the
> +         * hflags we saved above.  */
> +        saved_hflags = ctx->saved_hflags;
> +        ctx->saved_hflags = ctx->hflags;
>          gen_goto_tb(ctx, 1, ctx->pc + 4);
> +        ctx->saved_hflags = saved_hflags;

seems needlessly convoluted.  Does anyone think that

  /* Save and restore the state we're currently in (inside a branch-likely delay),
     while clearing that state as we exit the current TB.  */
  temp_sh = ctx->saved_hflags;
  temp_h  = ctx->hflags;

  ctx->hflags &= ~MIPS_HFLAG_BMASK;
  gen_goto_tb(...)

  ctx->saved_hflags = temp_sh;
  ctx->hflags = temp_h;

is any clearer?  I.e. let gen_goto_tb do what it so very much wants to do, which
is save hflags to env on the way out.


r~

[Qemu-devel] [PATCH v2] MIPS: Correct branch-likely single-stepping

[Maciej W. Rozycki]

Fix an issue with single-stepping branch likely instructions incorrectly 
nullifying (in the next step) the instruction immediately following the 
delay slot, when the branch is not taken.

The underlying cause of the issue is the MIPS_HFLAG_BL flag is cleared 
when the branch is determined to be not taken and the delay-slot 
instruction nullified, but then mistakenly restored prior to raising the 
single-step exception at the conclusion of the nullified delay-slot 
instruction.

Fix the issue then by making sure the MIPS_HFLAG_BL flag remains cleared 
throughout once the branch-not-taken TCG execution path is taken.

This is illustrated by an excerpt from a GDB session below:

(gdb) continue
Continuing.

Breakpoint 2, 0x8000b460 in __libc_init_array ()
4: /x $ra = 0x8000b460
2: x/i $pc
=> 0x8000b460 <__libc_init_array+124>:  sltu    v0,s0,s2
(gdb) stepi
0x8000b464 in __libc_init_array ()
4: /x $ra = 0x8000b460
2: x/i $pc
=> 0x8000b464 <__libc_init_array+128>:
    bnezl       v0,0x8000b454 <__libc_init_array+112>
   0x8000b468 <__libc_init_array+132>:  lw      v0,0(s1)
(gdb)
0x8000b46c in __libc_init_array ()
4: /x $ra = 0x8000b460
2: x/i $pc
=> 0x8000b46c <__libc_init_array+136>:  lw      ra,28(sp)
(gdb)
0x8000b470 in __libc_init_array ()
4: /x $ra = 0x8000b460
2: x/i $pc
=> 0x8000b470 <__libc_init_array+140>:  lw      s2,24(sp)
(gdb)

-- notice how $ra has not been updated.

Signed-off-by: Nathan Froyd <froydnj@codesourcery.com>
Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
---
Richard,

On Tue, 12 Jun 2012, Richard Henderson wrote:

> What about this comment, from the main body of gen_intermediate_code_internal:
> 
>         /* Execute a branch and its delay slot as a single instruction.
>            This is what GDB expects and is consistent with what the
>            hardware does (e.g. if a delay slot instruction faults, the
>            reported PC is the PC of the branch).  */
>         if (env->singlestep_enabled && (ctx.hflags & MIPS_HFLAG_BMASK) == 0)
>             break;
> 
> That suggests we should not have split the lw away from the bnezl in
> the first place, and thus the fix should be elsewhere.

 The comment stands, the problem is the MIPS_HFLAG_BL flag is mistakenly 
set back by a TCG instruction produced at the end of the TB (comprising 
both the branch and the delay-slot instruction) by `save_cpu_state' called 
by `gen_goto_tb' *only* when single-stepping.  It is the LW instruction at 
0x8000b46c that suffers from this problem, not the delay-slot LW 
instruction at 0x8000b468 (that was included in the branch's TB and 
nullified in this case, before the single-step debug exception was taken).

> Even failing that, this
> 
> >          tcg_gen_movi_i32(hflags, ctx->hflags & ~MIPS_HFLAG_BMASK);
> > +        /* Fake saving hflags so that gen_goto_tb doesn't overwrite the
> > +         * hflags we saved above.  */
> > +        saved_hflags = ctx->saved_hflags;
> > +        ctx->saved_hflags = ctx->hflags;
> >          gen_goto_tb(ctx, 1, ctx->pc + 4);
> > +        ctx->saved_hflags = saved_hflags;
> 
> seems needlessly convoluted.  Does anyone think that
> 
>   /* Save and restore the state we're currently in (inside a branch-likely delay),
>      while clearing that state as we exit the current TB.  */
>   temp_sh = ctx->saved_hflags;
>   temp_h  = ctx->hflags;
> 
>   ctx->hflags &= ~MIPS_HFLAG_BMASK;
>   gen_goto_tb(...)
> 
>   ctx->saved_hflags = temp_sh;
>   ctx->hflags = temp_h;
> 
> is any clearer?  I.e. let gen_goto_tb do what it so very much wants to do, which
> is save hflags to env on the way out.

 I think the confusion here is that `hflags' is only updated 
(`save_cpu_state' called) by `gen_goto_tb' when single-stepping.  The need 
to update `hflags' in all cases is the reason for the `tcg_gen_movi_i32' 
to `hflags' here.

 But this is the only place `hflags' is updated outside `save_cpu_state'.  
So I think for clarity we ought to just call `save_cpu_state' here as 
well, which will update `hflags', and also update `ctx->saved_hflags' 
effectively disabling the otherwise possible subsequent second call from 
`gen_goto_tb' (which would be suboptimal but harmless anyway as 
`ctx->hflags' will now have the correct value at that stage).  This change 
therefore brings your suggestions and my observations together.

 I have verified that this change works as well as the original one for me 
and also does not cause regressions in mips-sde-elf GDB testing, -EB (o32) 
multilib.

 Please apply.

  Maciej

qemu-mips-blikely.diff
Index: qemu-git-trunk/target-mips/translate.c
===================================================================
--- qemu-git-trunk.orig/target-mips/translate.c	2014-12-12 18:06:45.000000000 +0000
+++ qemu-git-trunk/target-mips/translate.c	2014-12-13 01:05:35.857530836 +0000
@@ -18466,11 +18466,25 @@ static void decode_opc(CPUMIPSState *env
     /* Handle blikely not taken case */
     if ((ctx->hflags & MIPS_HFLAG_BMASK_BASE) == MIPS_HFLAG_BL) {
         int l1 = gen_new_label();
+        uint32_t saved_hflags;
+        uint32_t bl_hflags;
 
         MIPS_DEBUG("blikely condition (" TARGET_FMT_lx ")", ctx->pc + 4);
         tcg_gen_brcondi_tl(TCG_COND_NE, bcond, 0, l1);
-        tcg_gen_movi_i32(hflags, ctx->hflags & ~MIPS_HFLAG_BMASK);
+
+        /* Save and restore the state we're currently in (inside
+           a branch-likely delay), while clearing that state
+           as we exit the current TB.  */
+        saved_hflags = ctx->saved_hflags;
+        bl_hflags = ctx->hflags;
+
+        ctx->hflags = bl_hflags & ~MIPS_HFLAG_BMASK;
+        save_cpu_state(ctx, 0);
         gen_goto_tb(ctx, 1, ctx->pc + 4);
+
+        ctx->saved_hflags = saved_hflags;
+        ctx->hflags = bl_hflags;
+
         gen_set_label(l1);
     }