Re: [Qemu-devel] Tracking unfreed tcg temps

[Aurelien Jarno]

On Tue, Jan 11, 2011 at 06:09:06AM -0600, Peter Maydell wrote:
> The ARM target-arm/translate.c file has some code in it which tries to
> track the number of TCG temporaries allocated during translation of an
> ARM instruction and complain if they are not freed by the end of that
> instruction. So new_tmp() allocates a temp with tcg_temp_new_i32() and
> increments the count; dead_tmp() calls tcg_temp_free() and decrements
> the count. If at the end of translating an instruction the count isn't
> zero we print a warning:
> 
>   fprintf(stderr, "Internal resource leak before %08x\n", dc->pc);
> 
> However there are a lot of code paths which will trigger this warning;
> generally these are for invalid encodings where we only notice that
> the opcode is invalid after having loaded the input operands, so we've
> allocated a temp but the generate-UNDEF-exception exit path doesn't
> free it.
> 
> tcg/README says that failure to free a temporary is not very harmful,
> because it just means more memory usage and slower translation. (On
> the other hand there seems to be a hardcoded TCG_MAX_TEMPS limit on
> the number of temporaries, which suggests that freeing temporaries is
> important and the README is misleading?)

This temporary is only valid for a given TB, so the leak is not going to
take more and more memory. On the other hand, if it is easy to trigger
such leaks with non-priviledged instructions and reach TCG_MAX_TEMPS,
this means that a simple user on a virtual machine can crash it. No risk
of security issue, but at least a DoS.

Note also that our register spill strategy is not really optimized, so
the generated code might be less optimized in such cases.

> So what's the best thing to do with this temporary-tracking code?
> 
> (1) just get rid of it as it is misguided

I think it is something important to make sure temp are correctly freed.
OTOH, it's maybe not a good idea to expose such message to users.

> (2) tweak it so that we don't complain about non-freed temps if this
> is the end of the TB anyway [since the invalid-encoding case will
> always result in ending the TB]

That might be a temporary solution.

> (3) rework all the code which catches invalid encodings so that we can
> identify undefined instructions before we have done any of the
> preparatory loading of operands that is causing the warning to trigger
> 
> [If it is useful to track not-freed-temps like this shouldn't the
> code be in tcg and not ad-hoc in target-arm anyway?]

I guess this is currently only done in target-arm, as it is loading
registers a bit differently than other targets. Other targets, or at
least some of them, tends to have a short par of code between
tcg_temp_new() and tcg_temp_free(), so it's easier to verify manually.
This is also probably related to the way the instruction space is split,
and for sure thumb doesn't help here.

That said, such a check can be useful on other targets, so moving it to
tcg/tcg.c might be a good idea. It can be made conditional on
CONFIG_DEBUG_TCG like many other checks of this kind. This #define is 
enabled by the --enable-debug-tcg configure option.

> This question is motivated by the meego-qemu tree including a set
> of changes which go down path (3), which I'm not sure what to do
> with...
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net