Re: [Qemu-devel] another TCG branch weirdness

[Artyom Tarasenko <atar4qemu@gmail.com>]

On Sat, Aug 6, 2011 at 2:09 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Fri, Aug 5, 2011 at 10:21 PM, Artyom Tarasenko <atar4qemu@gmail.com> wrote:
>> On Fri, Aug 5, 2011 at 10:32 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Fri, Aug 5, 2011 at 4:36 PM, Artyom Tarasenko <atar4qemu@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.
>
> 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,
>>     } 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/
>>
>



-- 
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/