Patchwork MIPS: Correct branch-likely single-stepping

login
register
mail settings
Submitter Maciej W. Rozycki
Date June 8, 2012, 1:05 a.m.
Message ID <alpine.DEB.1.10.1206072353260.23962@tp.orcam.me.uk>
Download mbox | patch
Permalink /patch/163703/
State New
Headers show

Comments

Maciej W. Rozycki - June 8, 2012, 1:05 a.m.
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
Richard Henderson - June 12, 2012, 2:55 p.m.
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~

Patch

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);
     }