Re: [Qemu-devel] Confusion regarding temporaries with branch conditional
Richard Hendersonwrites: > 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
On 11/30/2016 10:12 AM, Alex Bennée wrote: > > Richard Hendersonwrites: > >> On 11/30/2016 08:55 AM, Alex Bennée wrote: >>> >>> Nikunj A Dadhania 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~
Re: [Qemu-devel] Confusion regarding temporaries with branch conditional
Richard Hendersonwrites: > On 11/30/2016 08:55 AM, Alex Bennée wrote: >> >> Nikunj A Dadhania 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
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
On 11/30/2016 08:55 AM, Alex Bennée wrote: > > Nikunj A Dadhaniawrites: > >> 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
Nikunj A Dadhaniawrites: > 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
Peter Maydellwrites: > On 30 November 2016 at 07:00, Nikunj A Dadhania > 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
On 30 November 2016 at 07:00, Nikunj A Dadhaniawrote: > > 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