From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39313) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SeSV6-0006gh-Jx for qemu-devel@nongnu.org; Tue, 12 Jun 2012 10:55:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SeSV0-0007ig-53 for qemu-devel@nongnu.org; Tue, 12 Jun 2012 10:55:20 -0400 Received: from mail-gh0-f173.google.com ([209.85.160.173]:45337) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SeSUz-0007i7-Tv for qemu-devel@nongnu.org; Tue, 12 Jun 2012 10:55:14 -0400 Received: by ghrr14 with SMTP id r14so3925681ghr.4 for ; Tue, 12 Jun 2012 07:55:12 -0700 (PDT) Sender: Richard Henderson Message-ID: <4FD7584B.2080908@twiddle.net> Date: Tue, 12 Jun 2012 07:55:07 -0700 From: Richard Henderson MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] MIPS: Correct branch-likely single-stepping List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Maciej W. Rozycki" Cc: qemu-devel@nongnu.org, Aurelien Jarno On 2012-06-07 18:05, Maciej W. Rozycki wrote: > From: Nathan Froyd > > 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~