<div class="gmail_extra">On Fri, Jun 29, 2012 at 12:34 AM, Duncan Sands <span dir="ltr"><<a href="mailto:baldrick@free.fr" target="_blank" class="cremed">baldrick@free.fr</a>></span> wrote:<br><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Nick,<br>
<div class="im"><br>
> The attached patch improves the interaction between GlobalOpt and leak checkers.<br>
> One of the things that GlobalOpt will do is delete all the stores to a global<br>
> that isn't loaded. If for some reason we don't mop up the malloc/new which was<br>
> being stored in that global, a leak checker will complain.<br>
<br>
</div>if the malloc cleanup code sees that the malloc result is stored to a global,<br>
then won't that prevent it from removing the malloc?  In short, isn't it<br>
important to remove such dead stores exactly because you then have a chance of<br>
deleting the malloc call?<br>
<br>
Also, isn't the leak checker right to complain, and the fact that it doesn't<br>
complain when the value is stored to the global just a weakness of the leak<br>
checker?  After all, if without the store the memory leaked, then with the<br>
store the memory leaked too, since the global is never read.  So aren't you<br>
just hiding real leaks here?<br>
<br>
This patch seems completely backwards to how it should be to me.<br></blockquote><div><br></div><div>This is actually I think everyone's first reaction... and it's not unreasonable. We've been debating this pretty heatedly ourselves, so let me try to give some background.</div>
<div><br></div><div>First, how the heap leak checking strategy we're interested in works in brief[1] and I suspect greatly simplified, so don't take this as a literal explanation:</div><div><br></div><div>1) It instruments the malloc library and records every allocation in a set of pointers.</div>
<div>2) It removes each freed allocation from the set.</div><div>3) At the end of the program's execution, it removes from the set all "live" pointers.</div><div><br></div><div>The remaining pointers are considered "leaked".</div>
<div><br></div><div>Why is this strategy useful? Why wouldn't you insist that *everything* not freed is leaked? Because of threads.</div><div><br></div><div>In C++ (even the latest C++ standard), there is no reasonable and thread-safe way to free memory associated with global variables on shutdown. This stems from two strange realities of the system: there is no way to safely and correctly join *all* threads in a program, and the destructors (and thus memory deallocation) for global variables are run after main is exited. Because we can't join all threads before we exit main, it is always possible to exit main with a thread still alive and running and using a global variable.</div>
<div><br></div><div>The result is that every global variable shared between threads with a non-trivial destructor is technically thread-hostile, and if the destructor deallocates memory, it is definitely thread-hostile. The solution is to always "leak" global allocations that might be referenced by some thread still running past the end of main. These leaks are categorically different from others -- they are necessarily bounded (they are all named) and don't represent an un-known heap growth in the program.</div>
<div><br></div><div>I think the idea of GC roots is the best way to think about it -- the globals simply form a set of roots of allocations that are still live right to the end of the program, and thus have not yet been "leaked" per-se.</div>
<div><br></div><div><br></div><div>Now, the more interesting part is the part about whether these allocations are *actually* unused, and whether they can still be folded. If globalopt is going to be able to *remove* the allocation, then by all means we should let it. I think in many cases it will do this in the presence of that last store, but Nick would know for sure. If it doesn't, we can certainly teach it to do so.</div>
<div><br></div><div>The other case is the interesting one: when the allocation is itself used (handed into a registry or some other subsystem by the code in the globals initializer), but the actual global variable that holds the pointer is not used. Here, we clearly must keep the allocation (it is used, and can't be simulated by globalopt), but prior to Nick's patch LLVM would delete the global referring to the result. This removes a root that the leak checker uses to determine that in fact, this entity remains live.</div>
<div><br></div><div>The heap leak checking strategy has proven over many years in active use to be extremely high value, even in optimized programs, so we're quite interested in making sure that such a use case is well supported by LLVM. (It happens to be well supported by GCC, and we've worked to keep it that way.)  On the flip side, we've seen thousands of tests fail due to "leaks" that were entirely correct code following this pattern.</div>
<div><br></div><div>That said, if you or others can figure out a way to do heap leak checking that *doesn't* require preserving global allocation roots, that would be fantastic. None of us have been able to come up with anything remotely satisfying that has minimal performance impact, catches as many leaks, and avoids issues with intentional "leaks" through globals for the sake of thread-safety.</div>
<div><br></div><div>-Chandler</div><div><br></div><div>[1]: <a href="http://google-perftools.googlecode.com/svn/trunk/doc/heap_checker.html">http://google-perftools.googlecode.com/svn/trunk/doc/heap_checker.html</a></div>
</div></div>