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

[Richard Henderson <rth@twiddle.net>]

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~