[Qemu-devel] [PATCH 0/2] PPC: Check for temporary leakage

[Qemu-devel] [PATCH 0/2] PPC: Check for temporary leakage

[Alexander Graf]

While running qemu-system-ppc64 on my 32bit system I ran into cases where
TCG just stopped because it was running out of temporaries to allocate from.

Obviously this meant there were instructions not freeing their temporaries
properly, so I went ahead and implemented the same mechanism ARM uses to find
temporary leakage.

While at it, I also fixed all leaks I found.

Alexander Graf (2):
  PPC: Fix TCG chunks that don't free their temps
  PPC: Fail on leaking temporaries

 target-ppc/translate.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/2] PPC: Fix TCG chunks that don't free their temps

[Alexander Graf]

We want to make sure that every instruction cleans up after itself and
clears every temporary it allocated.

While checking whether this is already the case, I came across a few
cases where it isn't. This patch fixes every translation I found that
doesn't free their allocated temporaries.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 target-ppc/translate.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 31f32df..02cd18e 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -1177,6 +1177,7 @@ static inline void gen_op_arith_subf(DisasContext *ctx, TCGv ret, TCGv arg1,
             }
             tcg_gen_xor_tl(t1, arg2, inv1);         /* add without carry */
             tcg_gen_add_tl(t0, t0, inv1);
+            tcg_temp_free(inv1);
             tcg_gen_xor_tl(cpu_ca, t0, t1);         /* bits changes w/ carry */
             tcg_temp_free(t1);
             tcg_gen_shri_tl(cpu_ca, cpu_ca, 32);    /* extract bit 32 */
@@ -3714,6 +3715,9 @@ static inline void gen_bcond(DisasContext *ctx, int type)
         gen_update_nip(ctx, ctx->nip);
         tcg_gen_exit_tb(0);
     }
+    if (type == BCOND_LR || type == BCOND_CTR) {
+        tcg_temp_free(target);
+    }
 }
 
 static void gen_bc(DisasContext *ctx)
@@ -4156,6 +4160,7 @@ static void gen_mtmsr(DisasContext *ctx)
         tcg_gen_mov_tl(msr, cpu_gpr[rS(ctx->opcode)]);
 #endif
         gen_helper_store_msr(cpu_env, msr);
+        tcg_temp_free(msr);
         /* Must stop the translation as machine state (may have) changed */
         /* Note that mtmsr is not always defined as context-synchronizing */
         gen_stop_exception(ctx);
@@ -6290,6 +6295,7 @@ static void gen_tlbsx_booke206(DisasContext *ctx)
 
     tcg_gen_add_tl(t0, t0, cpu_gpr[rB(ctx->opcode)]);
     gen_helper_booke206_tlbsx(cpu_env, t0);
+    tcg_temp_free(t0);
 #endif
 }
 
@@ -6323,6 +6329,7 @@ static void gen_tlbivax_booke206(DisasContext *ctx)
     gen_addr_reg_index(ctx, t0);
 
     gen_helper_booke206_tlbivax(cpu_env, t0);
+    tcg_temp_free(t0);
 #endif
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/2] PPC: Fail on leaking temporaries

[Alexander Graf]

When QEMU gets compiled with --enable-debug-tcg we can check for temporary
leakage. Implement the necessary target code for this and fail emulation
when we hit a leakage.

This hopefully ensures that we don't get new leaks.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 target-ppc/translate.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 02cd18e..759133c 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -10412,6 +10412,7 @@ static inline void gen_intermediate_code_internal(PowerPCCPU *cpu,
         max_insns = CF_COUNT_MASK;
 
     gen_tb_start();
+    tcg_clear_temp_count();
     /* Set env in case of segfault during code fetch */
     while (ctx.exception == POWERPC_EXCP_NONE
             && tcg_ctx.gen_opc_ptr < gen_opc_end) {
@@ -10511,6 +10512,12 @@ static inline void gen_intermediate_code_internal(PowerPCCPU *cpu,
              */
             break;
         }
+        if (tcg_check_temp_count()) {
+            fprintf(stderr, "Opcode %02x %02x %02x (%08x) leaked temporaries\n",
+                    opc1(ctx.opcode), opc2(ctx.opcode), opc3(ctx.opcode),
+                    ctx.opcode);
+            exit(1);
+        }
     }
     if (tb->cflags & CF_LAST_IO)
         gen_io_end();
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2/2] PPC: Fail on leaking temporaries

[Peter Maydell]

On 19 January 2014 16:32, Alexander Graf <agraf@suse.de> wrote:
> When QEMU gets compiled with --enable-debug-tcg we can check for temporary
> leakage. Implement the necessary target code for this and fail emulation
> when we hit a leakage.
>
> This hopefully ensures that we don't get new leaks.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  target-ppc/translate.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 02cd18e..759133c 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -10412,6 +10412,7 @@ static inline void gen_intermediate_code_internal(PowerPCCPU *cpu,
>          max_insns = CF_COUNT_MASK;
>
>      gen_tb_start();
> +    tcg_clear_temp_count();
>      /* Set env in case of segfault during code fetch */
>      while (ctx.exception == POWERPC_EXCP_NONE
>              && tcg_ctx.gen_opc_ptr < gen_opc_end) {
> @@ -10511,6 +10512,12 @@ static inline void gen_intermediate_code_internal(PowerPCCPU *cpu,
>               */
>              break;
>          }
> +        if (tcg_check_temp_count()) {
> +            fprintf(stderr, "Opcode %02x %02x %02x (%08x) leaked temporaries\n",
> +                    opc1(ctx.opcode), opc2(ctx.opcode), opc3(ctx.opcode),
> +                    ctx.opcode);
> +            exit(1);

Exiting is pretty harsh; ARM just warns and continues. In my
experience most of the TCG temp leaks happen on paths
where the decoder has done some setup, then discovered
later that the instruction should throw an exception and
the exception generating code path exits the decoder function
early without freeing the TCG temp. Since we always finish
the TB immediately in this case, it's never possible to actually
run out of TCG temporaries. So I felt that continuing was better
than gratuitously stopping the guest from running in these cases,
since it's hard to be certain you've caught them all unless you
care to run the decoder through the complete set of instructions
from 0x00000000 to 0xffffffff. (That is actually possible in less
than geological time if you write a special purpose test harness.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] PPC: Fail on leaking temporaries

[Alexander Graf]

On 19.01.2014, at 17:51, Peter Maydell <peter.maydell@linaro.org> wrote:

> On 19 January 2014 16:32, Alexander Graf <agraf@suse.de> wrote:
>> When QEMU gets compiled with --enable-debug-tcg we can check for temporary
>> leakage. Implement the necessary target code for this and fail emulation
>> when we hit a leakage.
>> 
>> This hopefully ensures that we don't get new leaks.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> target-ppc/translate.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>> 
>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>> index 02cd18e..759133c 100644
>> --- a/target-ppc/translate.c
>> +++ b/target-ppc/translate.c
>> @@ -10412,6 +10412,7 @@ static inline void gen_intermediate_code_internal(PowerPCCPU *cpu,
>>         max_insns = CF_COUNT_MASK;
>> 
>>     gen_tb_start();
>> +    tcg_clear_temp_count();
>>     /* Set env in case of segfault during code fetch */
>>     while (ctx.exception == POWERPC_EXCP_NONE
>>             && tcg_ctx.gen_opc_ptr < gen_opc_end) {
>> @@ -10511,6 +10512,12 @@ static inline void gen_intermediate_code_internal(PowerPCCPU *cpu,
>>              */
>>             break;
>>         }
>> +        if (tcg_check_temp_count()) {
>> +            fprintf(stderr, "Opcode %02x %02x %02x (%08x) leaked temporaries\n",
>> +                    opc1(ctx.opcode), opc2(ctx.opcode), opc3(ctx.opcode),
>> +                    ctx.opcode);
>> +            exit(1);
> 
> Exiting is pretty harsh; ARM just warns and continues. In my
> experience most of the TCG temp leaks happen on paths
> where the decoder has done some setup, then discovered
> later that the instruction should throw an exception and
> the exception generating code path exits the decoder function
> early without freeing the TCG temp. Since we always finish
> the TB immediately in this case, it's never possible to actually
> run out of TCG temporaries. So I felt that continuing was better
> than gratuitously stopping the guest from running in these cases,
> since it's hard to be certain you've caught them all unless you
> care to run the decoder through the complete set of instructions
> from 0x00000000 to 0xffffffff. (That is actually possible in less
> than geological time if you write a special purpose test harness.)

Well, the check only ever happens when QEMU gets compiled with --enable-debug-tcg, so I figured it's easier for me to catch new problems or problems with unit tests if we get a harsh abort :).


Alex

Re: [Qemu-devel] [PATCH 2/2] PPC: Fail on leaking temporaries

[Peter Maydell]

On 19 January 2014 20:15, Alexander Graf <agraf@suse.de> wrote:
> On 19.01.2014, at 17:51, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Exiting is pretty harsh; ARM just warns and continues.

> Well, the check only ever happens when QEMU gets compiled
> with --enable-debug-tcg, so I figured it's easier for me to catch
> new problems or problems with unit tests if we get a harsh abort :).

Well, you're the one that gets to field the bug reports for PPC so
it's your call :-)

Longer term I was wondering if we should define the concept
of a 'scope object' for TCG temporaries, so you create a scope
object and then we have versions of tcg_temp_new_*() that
take a scope object to effectively define the lifetime of that
temp. Destroying the scope object frees every TCG temp in it.
Then we could just have the target frontends create a scope
for each instruction, and they wouldn't need to worry about
manually freeing TCG temporaries within it at all. That seems
better than the current approach where every frontend rolls
its own auto-free mechanism, and would render this sort of
"check for bugs in manual temp freeing" unnecessary too.

(We could also make the tcg_gen_brcond* functions do a
"free all temps in all scope objects" and then we'd catch
use-of-temp-after-branch bugs, especially if we also got
TCG to assert on use of a dead temporary rather than only
later when it was doing regalloc on it...)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] PPC: Fail on leaking temporaries

[Alexander Graf]

On 19.01.2014, at 21:52, Peter Maydell <peter.maydell@linaro.org> wrote:

> On 19 January 2014 20:15, Alexander Graf <agraf@suse.de> wrote:
>> On 19.01.2014, at 17:51, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Exiting is pretty harsh; ARM just warns and continues.
> 
>> Well, the check only ever happens when QEMU gets compiled
>> with --enable-debug-tcg, so I figured it's easier for me to catch
>> new problems or problems with unit tests if we get a harsh abort :).
> 
> Well, you're the one that gets to field the bug reports for PPC so
> it's your call :-)
> 
> Longer term I was wondering if we should define the concept
> of a 'scope object' for TCG temporaries, so you create a scope
> object and then we have versions of tcg_temp_new_*() that
> take a scope object to effectively define the lifetime of that
> temp. Destroying the scope object frees every TCG temp in it.
> Then we could just have the target frontends create a scope
> for each instruction, and they wouldn't need to worry about
> manually freeing TCG temporaries within it at all. That seems
> better than the current approach where every frontend rolls
> its own auto-free mechanism, and would render this sort of
> "check for bugs in manual temp freeing" unnecessary too.
> 
> (We could also make the tcg_gen_brcond* functions do a
> "free all temps in all scope objects" and then we'd catch
> use-of-temp-after-branch bugs, especially if we also got
> TCG to assert on use of a dead temporary rather than only
> later when it was doing regalloc on it...)

I agree - that would be awesome :). We could even go as far as defining a "fallback scope" that's instruction wide.


Alex

Re: [Qemu-devel] [PATCH 2/2] PPC: Fail on leaking temporaries

[Peter Maydell]

On 19 January 2014 20:55, Alexander Graf <agraf@suse.de> wrote:
> On 19.01.2014, at 21:52, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Longer term I was wondering if we should define the concept
>> of a 'scope object' for TCG temporaries, so you create a scope
>> object and then we have versions of tcg_temp_new_*() that
>> take a scope object to effectively define the lifetime of that
>> temp. Destroying the scope object frees every TCG temp in it.
>> Then we could just have the target frontends create a scope
>> for each instruction, and they wouldn't need to worry about
>> manually freeing TCG temporaries within it at all. That seems
>> better than the current approach where every frontend rolls
>> its own auto-free mechanism, and would render this sort of
>> "check for bugs in manual temp freeing" unnecessary too.
>>
>> (We could also make the tcg_gen_brcond* functions do a
>> "free all temps in all scope objects" and then we'd catch
>> use-of-temp-after-branch bugs, especially if we also got
>> TCG to assert on use of a dead temporary rather than only
>> later when it was doing regalloc on it...)
>
> I agree - that would be awesome :). We could even go as far
> as defining a "fallback scope" that's instruction wide

I was thinking the fallback scope would have to be 'whole
basic block' (both because that makes more sense as a TCG
API and to avoid having to fix every frontend at once), though
you're right that this sits a bit awkwardly with the fact that in
practice the scope you want 99% of the time is going to be
"single guest insn".

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] PPC: Fail on leaking temporaries

[Alexander Graf]

On 19.01.2014, at 22:04, Peter Maydell <peter.maydell@linaro.org> wrote:

> On 19 January 2014 20:55, Alexander Graf <agraf@suse.de> wrote:
>> On 19.01.2014, at 21:52, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Longer term I was wondering if we should define the concept
>>> of a 'scope object' for TCG temporaries, so you create a scope
>>> object and then we have versions of tcg_temp_new_*() that
>>> take a scope object to effectively define the lifetime of that
>>> temp. Destroying the scope object frees every TCG temp in it.
>>> Then we could just have the target frontends create a scope
>>> for each instruction, and they wouldn't need to worry about
>>> manually freeing TCG temporaries within it at all. That seems
>>> better than the current approach where every frontend rolls
>>> its own auto-free mechanism, and would render this sort of
>>> "check for bugs in manual temp freeing" unnecessary too.
>>> 
>>> (We could also make the tcg_gen_brcond* functions do a
>>> "free all temps in all scope objects" and then we'd catch
>>> use-of-temp-after-branch bugs, especially if we also got
>>> TCG to assert on use of a dead temporary rather than only
>>> later when it was doing regalloc on it...)
>> 
>> I agree - that would be awesome :). We could even go as far
>> as defining a "fallback scope" that's instruction wide
> 
> I was thinking the fallback scope would have to be 'whole
> basic block' (both because that makes more sense as a TCG
> API and to avoid having to fix every frontend at once), though
> you're right that this sits a bit awkwardly with the fact that in
> practice the scope you want 99% of the time is going to be
> "single guest insn".

Well, we could have each target define the "default scope" itself. On systems where you know that previous instructions may leak into the next (I don't know of any, but who knows) you could then make the TB the default scope.


Alex

[Qemu-devel] [PATCH 2/2] PPC: Fail on leaking temporaries

[Alexander Graf]

When QEMU gets compiled with --enable-debug-tcg we can check for temporary
leakage. Implement the necessary target code for this and fail emulation
when we hit a leakage.

This hopefully ensures that we don't get new leaks.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 target-ppc/translate.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 3a47b13..e9e7812 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -11767,6 +11767,7 @@ static inline void gen_intermediate_code_internal(PowerPCCPU *cpu,
         max_insns = CF_COUNT_MASK;
 
     gen_tb_start();
+    tcg_clear_temp_count();
     /* Set env in case of segfault during code fetch */
     while (ctx.exception == POWERPC_EXCP_NONE
             && tcg_ctx.gen_opc_ptr < gen_opc_end) {
@@ -11866,6 +11867,12 @@ static inline void gen_intermediate_code_internal(PowerPCCPU *cpu,
              */
             break;
         }
+        if (tcg_check_temp_count()) {
+            fprintf(stderr, "Opcode %02x %02x %02x (%08x) leaked temporaries\n",
+                    opc1(ctx.opcode), opc2(ctx.opcode), opc3(ctx.opcode),
+                    ctx.opcode);
+            exit(1);
+        }
     }
     if (tb->cflags & CF_LAST_IO)
         gen_io_end();
-- 
1.8.1.4