[Qemu-devel] [PATCH 2/2] PPC: Fail on leaking temporaries
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
---
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
Re: [Qemu-devel] [PATCH 2/2] PPC: Fail on leaking temporaries
On 19.01.2014, at 22:04, Peter Maydell wrote: > On 19 January 2014 20:55, Alexander Graf wrote: >> On 19.01.2014, at 21:52, Peter Maydell 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
Re: [Qemu-devel] [PATCH 2/2] PPC: Fail on leaking temporaries
On 19 January 2014 20:55, Alexander Graf wrote: > On 19.01.2014, at 21:52, Peter Maydell 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
On 19.01.2014, at 21:52, Peter Maydell wrote: > On 19 January 2014 20:15, Alexander Graf wrote: >> On 19.01.2014, at 17:51, Peter Maydell 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
On 19 January 2014 20:15, Alexander Graf wrote: > On 19.01.2014, at 17:51, Peter Maydell 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
On 19.01.2014, at 17:51, Peter Maydell wrote:
> On 19 January 2014 16:32, Alexander Graf 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
>> ---
>> 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 0x to 0x. (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
On 19 January 2014 16:32, Alexander Graf 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
> ---
> 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 0x to 0x. (That is actually possible in less
than geological time if you write a special purpose test harness.)
thanks
-- PMM
[Qemu-devel] [PATCH 2/2] PPC: Fail on leaking temporaries
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
---
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
