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