From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:52510) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QpiFn-000625-KC for qemu-devel@nongnu.org; Sat, 06 Aug 2011 10:53:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QpiFm-0001Vs-6K for qemu-devel@nongnu.org; Sat, 06 Aug 2011 10:53:31 -0400 Received: from mail-qw0-f45.google.com ([209.85.216.45]:54237) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QpiFm-0001VC-3p for qemu-devel@nongnu.org; Sat, 06 Aug 2011 10:53:30 -0400 Received: by qwj8 with SMTP id 8so678091qwj.4 for ; Sat, 06 Aug 2011 07:53:29 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: From: Artyom Tarasenko Date: Sat, 6 Aug 2011 16:53:08 +0200 Message-ID: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] another TCG branch weirdness List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: qemu-devel On Sat, Aug 6, 2011 at 2:09 PM, Blue Swirl wrote: > On Fri, Aug 5, 2011 at 10:21 PM, Artyom Tarasenko w= rote: >> On Fri, Aug 5, 2011 at 10:32 PM, Blue Swirl wrote= : >>> On Fri, Aug 5, 2011 at 4:36 PM, Artyom Tarasenko = wrote: >>>> Host x86_64, guest sparc64. Found a case where a branch instruction >>>> (brz,pn =A0 %o0) unexpectedly jumps to an unexpected address. I.e. >>>> branch shouldn't be taken at all, but even if it were it should have >>>> been to 0x13e26e4 and not to 0x5. >>>> >>>> Was about to write that the generated OP for brz,pn usually looks >>>> different, when realized that in fact it was even generated for this >>>> very address just before, but with another branch in the delay slot. >>>> The bug looks familiar, Blue, isn't it? :) >>> >>> Sorry, does not ring a bell. >> >> I meant c27e275 where you fixed unconditional branch in a delay slot. >> (One of my first bug reports). >> Now it looks pretty similar for the conditional branches. >> >>>> IN: >>>> 0x00000000013e26c0: =A0brz,pn =A0 %o0, 0x13e26e4 >>>> 0x00000000013e26c4: =A0brlez,pn =A0 %o1, 0x13e26e4 >>>> >>>> OP: >>>> =A0---- 0x13e26c0 >>>> =A0ld_i64 tmp6,regwptr,$0x0 >>>> =A0movi_i64 cond,$0x0 >>>> =A0movi_i64 tmp8,$0x0 >>>> =A0brcond_i64 tmp6,tmp8,ne,$0x0 >>>> =A0movi_i64 cond,$0x1 >>>> =A0set_label $0x0 >>>> >>>> ^^^ Ok, that's how brz,pn =A0usually looks like >>>> >>>> =A0---- 0x13e26c4 >>>> =A0ld_i64 tmp7,regwptr,$0x8 >>>> =A0movi_i64 tmp8,$0x0 >>>> =A0brcond_i64 cond,tmp8,eq,$0x1 >>>> =A0movi_i64 npc,$0x13e26e4 >>>> =A0br $0x2 >>>> =A0set_label $0x1 >>>> =A0movi_i64 npc,$0x13e26c8 >>>> =A0set_label $0x2 >>>> =A0movi_i64 cond,$0x0 >>>> =A0movi_i64 tmp8,$0x0 >>>> =A0brcond_i64 tmp7,tmp8,gt,$0x3 >>>> =A0movi_i64 cond,$0x1 >>>> =A0set_label $0x3 >>>> =A0movi_i64 tmp0,$0x0 >>>> =A0brcond_i64 cond,tmp0,eq,$0x4 >>>> =A0movi_i64 npc,$0x13e26e4 >>>> =A0br $0x5 >>>> =A0set_label $0x4 >>>> =A0movi_i64 npc,$0x5 >>>> =A0set_label $0x5 >>>> =A0exit_tb $0x0 >>>> -------------- >>>> IN: >>>> 0x00000000013e26c0: =A0brz,pn =A0 %o0, 0x13e26e4 >>>> >>>> OP: >>>> =A0---- 0x13e26c0 >>>> =A0ld_i64 tmp6,regwptr,$0x0 >>>> =A0movi_i64 cond,$0x0 >>>> =A0movi_i64 tmp8,$0x0 >>>> =A0brcond_i64 tmp6,tmp8,ne,$0x0 >>>> =A0movi_i64 cond,$0x1 >>>> =A0set_label $0x0 >>>> =A0movi_i64 pc,$0x5 >>>> >>>> ^^^ What's that? >>> >>> Probably DYNAMIC_PC + 4. I guess we are hitting this ancient comment >>> in target-sparc/translate.c:1372: >>> /* XXX: potentially incorrect if dynamic npc */ >> >> Yes, I think this too. The following patch passes my tests. Do you >> think it's correct? If yes, I'll make it for the other branches too. > > Looks OK. All these almost identical checks are a worrying: are all > cases covered? Is the logic same when it should be? Perhaps there > should be centralized handling, for example gen_next_pc_branch() > gen_next_pc_delay_slot() etc. with asserts. Sounds reasonable. Also do_branch and do_fbranch handle unconditional cases absolutely identically. Could do something like if (!do_unconditional_branch()) { gen_fcond() // gen_cond() ... > > Also reusing dc->pc etc for in band signaling is not robust as this case = shows. > >> @@ -1384,8 +1399,14 @@ static void do_branch_reg(DisasContext *dc, >> int32_t offset, uint32_t insn, >> =A0 =A0 } else { >> =A0 =A0 =A0 =A0 dc->pc =3D dc->npc; >> =A0 =A0 =A0 =A0 dc->jump_pc[0] =3D target; >> - =A0 =A0 =A0 =A0dc->jump_pc[1] =3D dc->npc + 4; >> - =A0 =A0 =A0 =A0dc->npc =3D JUMP_PC; >> + =A0 =A0 =A0 =A0if (unlikely(dc->npc =3D=3D DYNAMIC_PC)) { >> + =A0 =A0 =A0 =A0 =A0 =A0dc->jump_pc[1] =3D DYNAMIC_PC; >> + =A0 =A0 =A0 =A0 =A0 =A0tcg_gen_addi_tl(cpu_pc, cpu_npc, 4); >> + >> + =A0 =A0 =A0 =A0} else { >> + =A0 =A0 =A0 =A0 =A0 =A0dc->jump_pc[1] =3D dc->npc + 4; >> + =A0 =A0 =A0 =A0 =A0 =A0dc->npc =3D JUMP_PC; >> + =A0 =A0 =A0 =A0} >> ---- >> >> Regards, >> Artyom Tarasenko >> >> solaris/sparc under qemu blog: http://tyom.blogspot.com/ >> > --=20 Regards, Artyom Tarasenko solaris/sparc under qemu blog: http://tyom.blogspot.com/