[Qemu-devel] another TCG branch weirdness

[Qemu-devel] another TCG branch weirdness

[Artyom Tarasenko]

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? :)

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?

 movi_i64 tmp0,$0x0
 brcond_i64 cond,tmp0,eq,$0x1
 movi_i64 npc,$0x13e26e4
 br $0x2
 set_label $0x1
 movi_i64 npc,$0x9
 set_label $0x2
 exit_tb $0x0


 33062: Instruction Access MMU Miss (v=0064) pc=0000000000000005
npc=0000000000000009 SP=000000000c3d2d81
...
Current Register Window:
%o0-3: 0000000002483d00 0000000000000018 0000000000000028 00000000000232bd
            ^^^^^^ not zero


-- 
Regards,
Artyom Tarasenko

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

Re: [Qemu-devel] another TCG branch weirdness

[Blue Swirl]

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.

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

>  movi_i64 tmp0,$0x0
>  brcond_i64 cond,tmp0,eq,$0x1
>  movi_i64 npc,$0x13e26e4
>  br $0x2
>  set_label $0x1
>  movi_i64 npc,$0x9
>  set_label $0x2
>  exit_tb $0x0
>
>
>  33062: Instruction Access MMU Miss (v=0064) pc=0000000000000005
> npc=0000000000000009 SP=000000000c3d2d81
> ...
> Current Register Window:
> %o0-3: 0000000002483d00 0000000000000018 0000000000000028 00000000000232bd
>            ^^^^^^ not zero
>
>
> --
> Regards,
> Artyom Tarasenko
>
> solaris/sparc under qemu blog: http://tyom.blogspot.com/
>

Re: [Qemu-devel] another TCG branch weirdness

[Artyom Tarasenko]

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.

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

Re: [Qemu-devel] another TCG branch weirdness

[Blue Swirl]

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.

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

Re: [Qemu-devel] another TCG branch weirdness

[Artyom Tarasenko]

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/