From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=37872 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pcn7s-00026H-6H for qemu-devel@nongnu.org; Tue, 11 Jan 2011 17:55:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pcn7m-0005rh-Sc for qemu-devel@nongnu.org; Tue, 11 Jan 2011 17:55:40 -0500 Received: from hall.aurel32.net ([88.191.126.93]:53692) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pcn7m-0005rb-Hh for qemu-devel@nongnu.org; Tue, 11 Jan 2011 17:55:34 -0500 Date: Tue, 11 Jan 2011 23:55:33 +0100 From: Aurelien Jarno Subject: Re: [Qemu-devel] Tracking unfreed tcg temps Message-ID: <20110111225533.GC2577@volta.aurel32.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers 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 aurelien@aurel32.net http://www.aurel32.net