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.
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/
>
