Re: [Qemu-devel] another TCG branch weirdness

[Blue Swirl]

On Fri, Aug 5, 2011 at 10:21 PM, Artyom Tarasenko <address@hidden> wrote:
> 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.

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.

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,
>     } 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/
>