[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] another TCG branch weirdness
From: |
Artyom Tarasenko |
Subject: |
Re: [Qemu-devel] another TCG branch weirdness |
Date: |
Sat, 6 Aug 2011 00:21:12 +0200 |
On Fri, Aug 5, 2011 at 10:32 PM, Blue Swirl <address@hidden> wrote:
> On Fri, Aug 5, 2011 at 4:36 PM, Artyom Tarasenko <address@hidden> 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/