On Fri, Aug 5, 2011 at 10:32 PM, Blue Swirl <blauwir...@gmail.com> wrote: > On Fri, Aug 5, 2011 at 4:36 PM, Artyom Tarasenko <atar4q...@gmail.com> wrote: >> Host x86_64, guest sparc64. Found a case where a branch instruction >> (brz,pn %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: brz,pn %o0, 0x13e26e4 >> 0x00000000013e26c4: brlez,pn %o1, 0x13e26e4 >> >> OP: >> ---- 0x13e26c0 >> ld_i64 tmp6,regwptr,$0x0 >> movi_i64 cond,$0x0 >> movi_i64 tmp8,$0x0 >> brcond_i64 tmp6,tmp8,ne,$0x0 >> movi_i64 cond,$0x1 >> set_label $0x0 >> >> ^^^ Ok, that's how brz,pn usually looks like >> >> ---- 0x13e26c4 >> ld_i64 tmp7,regwptr,$0x8 >> movi_i64 tmp8,$0x0 >> brcond_i64 cond,tmp8,eq,$0x1 >> movi_i64 npc,$0x13e26e4 >> br $0x2 >> set_label $0x1 >> movi_i64 npc,$0x13e26c8 >> set_label $0x2 >> movi_i64 cond,$0x0 >> movi_i64 tmp8,$0x0 >> brcond_i64 tmp7,tmp8,gt,$0x3 >> movi_i64 cond,$0x1 >> set_label $0x3 >> movi_i64 tmp0,$0x0 >> brcond_i64 cond,tmp0,eq,$0x4 >> movi_i64 npc,$0x13e26e4 >> br $0x5 >> set_label $0x4 >> movi_i64 npc,$0x5 >> set_label $0x5 >> exit_tb $0x0 >> -------------- >> IN: >> 0x00000000013e26c0: brz,pn %o0, 0x13e26e4 >> >> OP: >> ---- 0x13e26c0 >> ld_i64 tmp6,regwptr,$0x0 >> movi_i64 cond,$0x0 >> movi_i64 tmp8,$0x0 >> brcond_i64 tmp6,tmp8,ne,$0x0 >> movi_i64 cond,$0x1 >> set_label $0x0 >> movi_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. @@ -1384,8 +1399,14 @@ static void do_branch_reg(DisasContext *dc, int32_t offset, uint32_t insn, } else { dc->pc = dc->npc; dc->jump_pc[0] = target; - dc->jump_pc[1] = dc->npc + 4; - dc->npc = JUMP_PC; + if (unlikely(dc->npc == DYNAMIC_PC)) { + dc->jump_pc[1] = DYNAMIC_PC; + tcg_gen_addi_tl(cpu_pc, cpu_npc, 4); + + } else { + dc->jump_pc[1] = dc->npc + 4; + dc->npc = JUMP_PC; + } ---- Regards, Artyom Tarasenko solaris/sparc under qemu blog: http://tyom.blogspot.com/