On 2012-06-07 18:05, Maciej W. Rozycki wrote:
> From: Nathan Froyd <froy...@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~
