[Qemu-devel] Confusion regarding temporaries with branch conditional

[Qemu-devel] Confusion regarding temporaries with branch conditional

[Nikunj A Dadhania]

Hi,

I was writing one instruction and hit following issue:

[snip]/qemu/tcg/tcg.c:2039: tcg fatal error
qemu-ppc64le: [snip]/qemu/translate-all.c:175: tb_lock: Assertion `!have_tb_lock' failed.
Segmentation fault (core dumped)

Debugging deeper found that its something to do with the variable type:

    TCGv nb = tcg_temp_new();
    tcg_gen_andi_tl(nb, cpu_gpr[rB(ctx->opcode)], 0xFF);
    tcg_gen_brcondi_tl(TCG_COND_EQ, nb, 0, l1);
    [ Do something here]
    gen_set_label(l1);
    tcg_temp_free(nb);

If I change the variable as "local temporary", the code works fine:

    TCGv nb = tcg_temp_local_new();
    tcg_gen_andi_tl(nb, cpu_gpr[rB(ctx->opcode)], 0xFF);
    tcg_gen_brcondi_tl(TCG_COND_EQ, nb, 0, l1);
    [ Do something here]
    gen_set_label(l1);
    tcg_temp_free(nb);

I see lot of code that is using temporaries for similar operations,
example target-ppc/translate.c:gen_check_align(). How is that working,
is this a bug there as well?

Regards,
Nikunj

Re: [Qemu-devel] Confusion regarding temporaries with branch conditional

[Peter Maydell]

On 30 November 2016 at 07:00, Nikunj A Dadhania
<nikunj@linux.vnet.ibm.com> wrote:
>
> Hi,
>
> I was writing one instruction and hit following issue:
>
> [snip]/qemu/tcg/tcg.c:2039: tcg fatal error
> qemu-ppc64le: [snip]/qemu/translate-all.c:175: tb_lock: Assertion `!have_tb_lock' failed.
> Segmentation fault (core dumped)
>
> Debugging deeper found that its something to do with the variable type:
>
>     TCGv nb = tcg_temp_new();
>     tcg_gen_andi_tl(nb, cpu_gpr[rB(ctx->opcode)], 0xFF);
>     tcg_gen_brcondi_tl(TCG_COND_EQ, nb, 0, l1);
>     [ Do something here]
>     gen_set_label(l1);
>     tcg_temp_free(nb);
>
> If I change the variable as "local temporary", the code works fine:
>
>     TCGv nb = tcg_temp_local_new();
>     tcg_gen_andi_tl(nb, cpu_gpr[rB(ctx->opcode)], 0xFF);
>     tcg_gen_brcondi_tl(TCG_COND_EQ, nb, 0, l1);
>     [ Do something here]
>     gen_set_label(l1);
>     tcg_temp_free(nb);
>
> I see lot of code that is using temporaries for similar operations,
> example target-ppc/translate.c:gen_check_align(). How is that working,
> is this a bug there as well?

You don't say what your "do something" code is doing, which
is the critical question for whether you need a plain
temporary or a local temporary. (See tcg/README.)
The plain temporary is only valid to the end of a basic
block, and brcond ends a basic block. So you can free
the temp after the brcond but you can't do anything
else with it. (This is what the PPC gen_check_align() does.)
If you want to use 'nb' in the "do something" code then
it must remain valid over the end of the basic block
and you need a local temporary.

thanks
-- PMM

Re: [Qemu-devel] Confusion regarding temporaries with branch conditional

[Nikunj A Dadhania]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 30 November 2016 at 07:00, Nikunj A Dadhania
> <nikunj@linux.vnet.ibm.com> wrote:
>>
>> Hi,
>>
>> I was writing one instruction and hit following issue:
>>
>> [snip]/qemu/tcg/tcg.c:2039: tcg fatal error
>> qemu-ppc64le: [snip]/qemu/translate-all.c:175: tb_lock: Assertion `!have_tb_lock' failed.
>> Segmentation fault (core dumped)
>>
>> Debugging deeper found that its something to do with the variable type:
>>
>>     TCGv nb = tcg_temp_new();
>>     tcg_gen_andi_tl(nb, cpu_gpr[rB(ctx->opcode)], 0xFF);
>>     tcg_gen_brcondi_tl(TCG_COND_EQ, nb, 0, l1);
>>     [ Do something here]
>>     gen_set_label(l1);
>>     tcg_temp_free(nb);
>>
>> If I change the variable as "local temporary", the code works fine:
>>
>>     TCGv nb = tcg_temp_local_new();
>>     tcg_gen_andi_tl(nb, cpu_gpr[rB(ctx->opcode)], 0xFF);
>>     tcg_gen_brcondi_tl(TCG_COND_EQ, nb, 0, l1);
>>     [ Do something here]
>>     gen_set_label(l1);
>>     tcg_temp_free(nb);
>>
>> I see lot of code that is using temporaries for similar operations,
>> example target-ppc/translate.c:gen_check_align(). How is that working,
>> is this a bug there as well?
>
> You don't say what your "do something" code is doing, which
> is the critical question for whether you need a plain
> temporary or a local temporary. (See tcg/README.)

Lets bring full example here.

    TCGv nb = tcg_temp_new();
    tcg_gen_andi_tl(nb, cpu_gpr[rB(ctx->opcode)], 0xFF);
    tcg_gen_brcondi_tl(TCG_COND_EQ, nb, 0, l1);

    /* do something */
    gen_set_access_type(ctx, ACCESS_INT);
    EA = tcg_temp_new();
    gen_addr_register(ctx, EA);
    tcg_gen_qemu_ld_i64(xtl, EA, ctx->mem_idx, MO_LEQ);
    tcg_gen_addi_tl(EA, EA, 8);
    tcg_gen_qemu_ld_i64(xth, EA, ctx->mem_idx, MO_LEQ);
    opc = tcg_const_i32(ctx->opcode);
    gen_helper_lxvl(cpu_env, opc, nb);      /* <--- That uses nb */
    tcg_temp_free_i32(opc);
    tcg_temp_free(EA);

    gen_set_label(l1);
    tcg_temp_free(nb);

> The plain temporary is only valid to the end of a basic
> block, and brcond ends a basic block. So you can free
> the temp after the brcond but you can't do anything
> else with it.

In the above case, assuming that nb is a plain temporary, case nb != 0
worked fine (by fluke?), i.e. no branch.

While when nb == 0, failed, i.e. branch taken to l1, and just free nb. I
am not using "nb" in this case.

> (This is what the PPC gen_check_align() does.)
> If you want to use 'nb' in the "do something" code then
> it must remain valid over the end of the basic block
> and you need a local temporary.

Understood, I need nb in that code, so I will use local temporary.

Regards
Nikunj

Re: [Qemu-devel] Confusion regarding temporaries with branch conditional

[Richard Henderson]

On 11/29/2016 11:56 PM, Nikunj A Dadhania wrote:
> Lets bring full example here.
> 
>     TCGv nb = tcg_temp_new();
>     tcg_gen_andi_tl(nb, cpu_gpr[rB(ctx->opcode)], 0xFF);
>     tcg_gen_brcondi_tl(TCG_COND_EQ, nb, 0, l1);
> 
>     /* do something */
>     gen_set_access_type(ctx, ACCESS_INT);
>     EA = tcg_temp_new();
>     gen_addr_register(ctx, EA);
>     tcg_gen_qemu_ld_i64(xtl, EA, ctx->mem_idx, MO_LEQ);
>     tcg_gen_addi_tl(EA, EA, 8);
>     tcg_gen_qemu_ld_i64(xth, EA, ctx->mem_idx, MO_LEQ);
>     opc = tcg_const_i32(ctx->opcode);
>     gen_helper_lxvl(cpu_env, opc, nb);      /* <--- That uses nb */
>     tcg_temp_free_i32(opc);
>     tcg_temp_free(EA);
> 
>     gen_set_label(l1);
>     tcg_temp_free(nb);
> 
>> The plain temporary is only valid to the end of a basic
>> block, and brcond ends a basic block. So you can free
>> the temp after the brcond but you can't do anything
>> else with it.
> 
> In the above case, assuming that nb is a plain temporary, case nb != 0
> worked fine (by fluke?), i.e. no branch.
> 
> While when nb == 0, failed, i.e. branch taken to l1, and just free nb. I
> am not using "nb" in this case.

This is also a good example of why you should preferentially avoid branches
within the tcg opcode stream.

In the case of lxvl, I strongly suggest that you push *everything* into the
helper.  In particular:

 (1) Passing the full instruction opcode means you've got to re-parse.
     Why are you not passing a pointer to the XT register like other
     VSX helpers?

 (2) As I read it, this is wrong, since when NB == 0, XT is assigned 0.
     Which you are not doing, having skipped over the helper.

 (3) Use cpu_ldq_data_ra within the helper to perform the memory loads.


r~

Re: [Qemu-devel] Confusion regarding temporaries with branch conditional

[Nikunj A Dadhania]

Richard Henderson <rth@twiddle.net> writes:

> On 11/29/2016 11:56 PM, Nikunj A Dadhania wrote:
>> Lets bring full example here.
>> 
>>     TCGv nb = tcg_temp_new();
>>     tcg_gen_andi_tl(nb, cpu_gpr[rB(ctx->opcode)], 0xFF);
>>     tcg_gen_brcondi_tl(TCG_COND_EQ, nb, 0, l1);
>> 
>>     /* do something */
>>     gen_set_access_type(ctx, ACCESS_INT);
>>     EA = tcg_temp_new();
>>     gen_addr_register(ctx, EA);
>>     tcg_gen_qemu_ld_i64(xtl, EA, ctx->mem_idx, MO_LEQ);
>>     tcg_gen_addi_tl(EA, EA, 8);
>>     tcg_gen_qemu_ld_i64(xth, EA, ctx->mem_idx, MO_LEQ);
>>     opc = tcg_const_i32(ctx->opcode);
>>     gen_helper_lxvl(cpu_env, opc, nb);      /* <--- That uses nb */
>>     tcg_temp_free_i32(opc);
>>     tcg_temp_free(EA);
>> 
>>     gen_set_label(l1);
>>     tcg_temp_free(nb);
>> 
>>> The plain temporary is only valid to the end of a basic
>>> block, and brcond ends a basic block. So you can free
>>> the temp after the brcond but you can't do anything
>>> else with it.
>> 
>> In the above case, assuming that nb is a plain temporary, case nb != 0
>> worked fine (by fluke?), i.e. no branch.
>> 
>> While when nb == 0, failed, i.e. branch taken to l1, and just free nb. I
>> am not using "nb" in this case.
>
> This is also a good example of why you should preferentially avoid branches
> within the tcg opcode stream.
>
> In the case of lxvl, I strongly suggest that you push *everything* into the
> helper.  In particular:
>
>  (1) Passing the full instruction opcode means you've got to re-parse.
>      Why are you not passing a pointer to the XT register like other
>      VSX helpers?

Sure, I can do that. No particular reason though.

>  (2) As I read it, this is wrong, since when NB == 0, XT is assigned 0.
>      Which you are not doing, having skipped over the helper.

I am doing that in my code. Didn't want to complicate the example code
above.

>  (3) Use cpu_ldq_data_ra within the helper to perform the memory
>  loads.

Let me try that.

Regards
Nikunj

Re: [Qemu-devel] Confusion regarding temporaries with branch conditional

[Alex Bennée]

Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:

> Hi,
>
> I was writing one instruction and hit following issue:
>
> [snip]/qemu/tcg/tcg.c:2039: tcg fatal error
> qemu-ppc64le: [snip]/qemu/translate-all.c:175: tb_lock: Assertion `!have_tb_lock' failed.
> Segmentation fault (core dumped)

This is confusing because something is trying to take the tb_lock while
you are in code generation. tb_lock is held for code generation to
ensure serialisation of generation.

>
> Debugging deeper found that its something to do with the variable type:
>
>     TCGv nb = tcg_temp_new();
>     tcg_gen_andi_tl(nb, cpu_gpr[rB(ctx->opcode)], 0xFF);
>     tcg_gen_brcondi_tl(TCG_COND_EQ, nb, 0, l1);
>     [ Do something here]
>     gen_set_label(l1);
>     tcg_temp_free(nb);
>
> If I change the variable as "local temporary", the code works fine:
>
>     TCGv nb = tcg_temp_local_new();
>     tcg_gen_andi_tl(nb, cpu_gpr[rB(ctx->opcode)], 0xFF);
>     tcg_gen_brcondi_tl(TCG_COND_EQ, nb, 0, l1);
>     [ Do something here]
>     gen_set_label(l1);
>     tcg_temp_free(nb);
>
> I see lot of code that is using temporaries for similar operations,
> example target-ppc/translate.c:gen_check_align(). How is that working,
> is this a bug there as well?

Well that is odd. Are you sure there is no side effect that is
attempting to modify run state during generation? I'm thinking of
changing memory maps or other such stuff. A back trace at the assert
would make things clearer.

>
> Regards,
> Nikunj


--
Alex Bennée

Re: [Qemu-devel] Confusion regarding temporaries with branch conditional

[Richard Henderson]

On 11/30/2016 08:55 AM, Alex Bennée wrote:
> 
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
> 
>> Hi,
>>
>> I was writing one instruction and hit following issue:
>>
>> [snip]/qemu/tcg/tcg.c:2039: tcg fatal error
>> qemu-ppc64le: [snip]/qemu/translate-all.c:175: tb_lock: Assertion `!have_tb_lock' failed.
>> Segmentation fault (core dumped)
> 
> This is confusing because something is trying to take the tb_lock while
> you are in code generation. tb_lock is held for code generation to
> ensure serialisation of generation.

Yes, I've seen this myself.  I never got around to reporting the "problem"
properly.  It's a confusing side effect of a SIGSEGV arriving during tcg code
generation.  The signal handler longjmps back with unexpected locks held.

Probably we should simply crash earlier and less confusingly.


r~

Re: [Qemu-devel] Confusion regarding temporaries with branch conditional

[Alex Bennée]

Richard Henderson <rth@twiddle.net> writes:

> On 11/30/2016 08:55 AM, Alex Bennée wrote:
>>
>> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
>>
>>> Hi,
>>>
>>> I was writing one instruction and hit following issue:
>>>
>>> [snip]/qemu/tcg/tcg.c:2039: tcg fatal error
>>> qemu-ppc64le: [snip]/qemu/translate-all.c:175: tb_lock: Assertion `!have_tb_lock' failed.
>>> Segmentation fault (core dumped)
>>
>> This is confusing because something is trying to take the tb_lock while
>> you are in code generation. tb_lock is held for code generation to
>> ensure serialisation of generation.
>
> Yes, I've seen this myself.  I never got around to reporting the "problem"
> properly.  It's a confusing side effect of a SIGSEGV arriving during tcg code
> generation.  The signal handler longjmps back with unexpected locks
> held.

So this is a SEGV which belongs to the translation code rather than the
guest?

There are places in the cpu loop where we exit that should reset the
locks on a restart - see tb_lock_reset() so I'm not quite sure what has
happened here.

>
> Probably we should simply crash earlier and less confusingly.
>
>
> r~


--
Alex Bennée

Re: [Qemu-devel] Confusion regarding temporaries with branch conditional

[Richard Henderson]

On 11/30/2016 10:12 AM, Alex Bennée wrote:
> 
> Richard Henderson <rth@twiddle.net> writes:
> 
>> On 11/30/2016 08:55 AM, Alex Bennée wrote:
>>>
>>> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
>>>
>>>> Hi,
>>>>
>>>> I was writing one instruction and hit following issue:
>>>>
>>>> [snip]/qemu/tcg/tcg.c:2039: tcg fatal error
>>>> qemu-ppc64le: [snip]/qemu/translate-all.c:175: tb_lock: Assertion `!have_tb_lock' failed.
>>>> Segmentation fault (core dumped)
>>>
>>> This is confusing because something is trying to take the tb_lock while
>>> you are in code generation. tb_lock is held for code generation to
>>> ensure serialisation of generation.
>>
>> Yes, I've seen this myself.  I never got around to reporting the "problem"
>> properly.  It's a confusing side effect of a SIGSEGV arriving during tcg code
>> generation.  The signal handler longjmps back with unexpected locks
>> held.
> 
> So this is a SEGV which belongs to the translation code rather than the
> guest?

Yes.

> There are places in the cpu loop where we exit that should reset the
> locks on a restart - see tb_lock_reset() so I'm not quite sure what has
> happened here.

It should be easy enough to force a segv from within the compile step to
reproduce this.


r~