| Message ID | alpine.DEB.1.10.1206072353260.23962@tp.orcam.me.uk |
|---|---|
| State | New |
| Headers | show |
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~
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); }