Menu
▾
▴
[Tcl-bugs] [ tcl-Bugs-3386721 ] leak in tcltest
|
From: SourceForge.net <no...@so...> - 2011-08-10 17:30:05
|
Bugs item #3386721, was opened at 2011-08-05 07:34 Message generated for change (Comment added) made by dgp You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=110894&aid=3386721&group_id=10894 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: development: 8.6b2 Status: Open Resolution: None Priority: 5 Private: No Submitted By: miguel sofer (msofer) Assigned to: Don Porter (dgp) Summary: leak in tcltest Initial Comment: This script package require tcltest 2 namespace import -force ::tcltest::* testintobj set 0 1 interp create slave load {} Tcltest slave causes the following leak: 0: malloc (vg_replace_malloc.c:236) 1: TclpAlloc (tclAlloc.c:705) 2: Tcl_Alloc (tclCkalloc.c:1046) 3: Tcl_NewIntObj (tclObj.c:2410) 4: TestintobjCmd (tclTestObj.c:640) 5: TclNREvalObjv (tclBasic.c:4272) 6: TEBCresume (tclExecute.c:2789) 7: TclNRRunCallbacks (tclBasic.c:4314) 8: TclEvalObjEx (tclBasic.c:5885) 9: Tcl_EvalObjEx (tclBasic.c:5866) The script is the minimal thing I found while investigating a leak in execute.test, every line seems significant. ---------------------------------------------------------------------- >Comment By: Don Porter (dgp) Date: 2011-08-10 13:30 Message: Reverted the nijtmans commit. Comments here are correct. Moving to per-thread storage isn't the right fix. If getting the right fix is easy, by all means go ahead, and if someone's ambitious and lacks other tasks, go ahead and audit Tcltest for all the similar outdated coding that must lurk within (A very very large part of Tcltest has roots back to the days when "multiple interps" just could not happen, and were coded to the truths of the day). That said, I imagine most folks reading this comment have other tasks they could be doing that would be more important in getting 8.6.0 out the door. I will not block 8.6.0 because our testing tools are not as polished as we would like them to be. ---------------------------------------------------------------------- Comment By: Alexandre Ferrieux (ferrieux) Date: 2011-08-10 13:11 Message: OK, so to get back to the original problem, emiliano suggested RBC as an example of per-interp package init. It uses the assocdata hashtable to store its stuff. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2011-08-10 13:08 Message: --enable-symbols=all should not be used. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2011-08-10 12:53 Message: Ok, that's what I'm doing. The script in this Tracker item does not leak. ---------------------------------------------------------------------- Comment By: Alexandre Ferrieux (ferrieux) Date: 2011-08-10 12:53 Message: env CFLAGS=-DPURIFY ./configure && make clean && make tcltest valgrind ./tcltest foo.tcl ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2011-08-10 12:43 Message: I still cannot reproduce the claimed leak. Will someone please reveal the complete recipe? ---------------------------------------------------------------------- Comment By: Alexandre Ferrieux (ferrieux) Date: 2011-08-10 12:16 Message: Jan, you're up the wrong tree :} There are two orthogonal things (1) the fact that the varPtr are never freed actively (2) the fact that two calls to TclTestObj_Init by two interps wreak havoc by overwriting the static array As it turns out, (1) is no problem because valgrind marks such blocks as "still reachable" (and there are a bunch of other such ones : a low priority endeavour), while here we're after the more ominous "definitely lost". And (2) is the real problem: an overwritten last reference to an object makes it definitely lost indeed. Note that using the TSD is not enough. As I said earlier, those data shoule be per-interp, not per-thread: there may be several interps in a single thread, as is notably the case here (a slave). ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2011-08-10 11:33 Message: How about creating an exit handler using Tcl_CreateExitHandler(), which clears the storage? That should do the trick. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2011-08-10 11:29 Message: Actually, that's not strange: The testintobj set 0 1 allocates a Tcl object in the TSD, but no-one throws it away!! Maybe we could create a "testclearall" command which clears all this storage as cleanup command after the test. Is there a way to define a cleanup callback when the thread ends? ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2011-08-10 10:37 Message: nope, leak still present ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2011-08-10 10:33 Message: Committed to trunk. Michael, does this help? Regards, Jan Nijtmans ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2011-08-10 10:30 Message: >Fix: move the varPtr array from static to per-interp storage. Never done >this myself, appreciate a hand. The 'standard' way to deal with this is using TSD (Thread Specific Cata) ---------------------------------------------------------------------- Comment By: Alexandre Ferrieux (ferrieux) Date: 2011-08-10 07:06 Message: Note that tclTest.c also has a few r/w statics. Also tclThreadTest.c has a few ones, though I cannot say they really are a threat (mainThreadID etc), by ignorance of realistic cases of concurrent/multiinterp loads of the Thread package. ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2011-08-10 07:03 Message: assigning to the package expert so that he chimes in ---------------------------------------------------------------------- Comment By: Alexandre Ferrieux (ferrieux) Date: 2011-08-10 06:57 Message: Ah, I realize now that the Init func of a package is supposed to be called once for each interp it is loaded in ... so clearly the fault here is the init proc's, which is not designed for this multiple-interp setup. Fix: move the varPtr array from static to per-interp storage. Never done this myself, appreciate a hand. Also, verify similar behavior in tons of other Tcltest subpackages ? ---------------------------------------------------------------------- Comment By: Alexandre Ferrieux (ferrieux) Date: 2011-08-09 20:03 Message: OK, got it. In this test, there is a static array varPtr[] that holds the working values. When [load .. slave] is called, somehow the TclObjTest_Init() function is called a 2nd time, and that array is reset, overwriting the obj references. Now to understand how the LoadObjCmd code is supposed to prevent that double Init. ---------------------------------------------------------------------- Comment By: miguel sofer (msofer) Date: 2011-08-09 17:58 Message: Still there: mig@ari:~/DEVEL/tcl-core/trunk/unix$ /usr/bin/valgrind --leak-check=yes --num-callers=10 ./tcltest < /tmp/foo ==9650== Memcheck, a memory error detector ==9650== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al. ==9650== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info ==9650== Command: ./tcltest ==9650== ==9650== ==9650== HEAP SUMMARY: ==9650== in use at exit: 1,074 bytes in 26 blocks ==9650== total heap usage: 40,907 allocs, 40,881 frees, 3,898,487 bytes allocated ==9650== ==9650== 48 bytes in 1 blocks are definitely lost in loss record 23 of 26 ==9650== at 0x4C2815C: malloc (vg_replace_malloc.c:236) ==9650== by 0x4BCFA1: TclpAlloc (tclAlloc.c:705) ==9650== by 0x4CED10: Tcl_Alloc (tclCkalloc.c:1046) ==9650== by 0x4546B6: Tcl_NewIntObj (tclObj.c:2410) ==9650== by 0x422B18: TestintobjCmd (tclTestObj.c:640) ==9650== by 0x4C2C00: TclNREvalObjv (tclBasic.c:4269) ==9650== by 0x54B39C: TEBCresume (tclExecute.c:2793) ==9650== by 0x4C2D31: TclNRRunCallbacks (tclBasic.c:4311) ==9650== by 0x4C54E9: TclEvalObjEx (tclBasic.c:5882) ==9650== by 0x4C5479: Tcl_EvalObjEx (tclBasic.c:5863) ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2011-08-09 14:48 Message: No longer leaking in the trunk? ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=110894&aid=3386721&group_id=10894 |