Re: [Qemu-devel] [PATCH 0/3] tcg: Support debugging leakage of temporaries

[Aurelien Jarno]

On Wed, Feb 23, 2011 at 03:19:33PM +0000, Peter Maydell wrote:
> This patchset removes the ad-hoc debug code in target-arm for
> identifying cases where we leaked TCG temporary variables, in
> favour of an implementation in tcg itself.
> 
> Generally any temporaries created by a target while it is
> translating an instruction should be freed by the end of that
> instruction; otherwise carefully crafted guest code could cause
> TCG to run out of temporaries and assert.
> 
> Putting the leak-debugging code into TCG proper (a) makes more
> sense as this isn't at all arm-specific (b) makes it more
> comprehensive, as it now covers temporaries created in all ways,
> not just via the new_tmp()/dead_tmp() wrapper functions
> (c) avoids annoying false positives where eg a TCG temp created
> with tcg_const_i32() was passed to dead_tmp().
> 
> The tracking only happens if qemu was configured with
> --enable-debug-tcg. It should be easy to add to other targets if
> desired; it's just a matter of calling tcg_clear_temp_count()
> and tcg_check_temp_count() in the appropriate places.
> 
> Peter Maydell (3):
>   tcg: Add support for debugging leakage of temporaries
>   target-arm: Remove ad-hoc leak checking code
>   target-arm: Use TCG temporary leak debugging facilities
> 
>  target-arm/translate.c |  705 +++++++++++++++++++++++------------------------
>  tcg/tcg.c              |   32 +++
>  tcg/tcg.h              |   17 ++
>  3 files changed, 394 insertions(+), 360 deletions(-)
> 

I am basically fine with this patch series modulo the small comment you
made about patch 1. My only concern is that, given ARM code is quite
sensitive to this kind of issue (there is no real rule if the caller or
the callee should free the temp), that we forget to enable TCG debug to
validate a patch. On the other hand I fully agree that this kind of
check should not be done in "production" use.

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