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 aurel...@aurel32.net http://www.aurel32.net
