Re: [Qemu-devel] another TCG branch weirdness

[Artyom Tarasenko]

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/