<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Apr 22, 2021, 7:28 PM Chris Lattner <<a href="mailto:clattner@nondot.org" target="_blank" rel="noreferrer">clattner@nondot.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
> On Apr 19, 2021, at 5:24 AM, James Y Knight <<a href="mailto:jyknight@google.com" rel="noreferrer noreferrer" target="_blank">jyknight@google.com</a>> wrote:<br>
> <br>
> There is no problem (no leaks) in the code that users wrote, so adding code annotations (sanitizer suppression file, or attributes) is not a good solution to this issue. The problem is that this optimization introduces a "leak" (from the point of view of the leak checker), which wasn't there before. And in practice, this seems to cause a large number of false positives.<br>
<br>
I think that “from the point of view of the leak checker” is the key thing there.<br><br>
Code that this triggers *is* leaking memory</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"></blockquote></div></div><div dir="auto"><br></div><div dir="auto">No, you've got it exactly backwards. "From the point of view of the leak checker", there is a leak, but in actually, there is not.</div><div dir="auto"><br></div><div dir="auto">I'm<span style="font-family:sans-serif"> afraid you're still arguing from mistaken assumptions. As I've already mentioned, reachable memory at program exit is not a leak. </span><span style="font-family:sans-serif">That's the definition of "leak" which is always used by leak checkers. (This is not anything new, it's been how leak checkers work for decades, and how they must work.)</span></div><div dir="auto"><br></div><div dir="auto"><span style="font-family:sans-serif">Therefore, C++ code that allocates memory and assigns it to a global is not a leak, and it's _still_ not a leak even if it so happens in some instantiation of the program that all of the users of the global have been removed by the optimizer.</span><br></div><div dir="auto"><span style="font-family:sans-serif"><br></span></div><div dir="auto"><font face="sans-serif">The code is correct and it's not leaking memory, but with this change, the leak checker is unable to determine that. </font></div><div dir="auto"><span style="font-family:sans-serif"><br></span></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">It was just silenced because the leak was spuriously reachable from a global.  Global variables aren’t a preferred way to silence leak detectors, they have other ways to do so :)</blockquote></div></div><div dir="auto"><br></div><div dir="auto">Memory reachable from a global is not a spurious reachability, it is actual reachability. And, the purpose of assigning a value to a global variable in the source code isn't to silence the leak checker, it is to make the object accessible to other code. (People writing code normally aren't and shouldn't be thinking about leak checking.)</div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> On Apr 19, 2021, at 4:52 PM, Sterling Augustine <<a href="mailto:saugustine@google.com" rel="noreferrer noreferrer" target="_blank">saugustine@google.com</a>> wrote:<br>
> <br>
> There may be other ways to disable leak checkers, but they put a burden on users that was not there before. Further, users who try leak checking with and without optimization will get different answers. The bug report will read: "clang at -O2 makes my program leak". And, as James notes, whether or not you need to suppress the leak depends on whether or not the optimizer does away with the variable. Subtle changes to the code that have nothing to do with memory allocation will appear to add or fix leaks. That is not something I would care to explain or document.<br>
<br>
I can see that concern, but this isn’t a battle that we can win: optimizations in general can expose leaks.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">The word "expose" is invalid here -- that implies that the code is buggy but that the leak checker was previously unable to detect the bug, and now does. But that is not the case at hand. You maybe could say, instead "<span style="font-family:sans-serif">I can see that concern, but this isn’t a battle that we can win: optimizations in general can cause random false positives in the leak checker." (But, in practice it was pretty much "won" for the last 9 years.)</span></div><div dir="auto"><font face="sans-serif"><br></font></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
IMO, If someone doesn’t want the global to be removed, they should mark it volatile.  If they do want it removable, then they can use leak detector features to silence the warning.</blockquote></div></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> On Apr 20, 2021, at 9:12 AM, Sterling Augustine <<a href="mailto:saugustine@google.com" rel="noreferrer noreferrer" target="_blank">saugustine@google.com</a>> wrote:<br>
> <br>
> In order to understand how much benefit this change gives to code size, I built clang and related files with and without the patch, both with CMAKE_BUILD_TYPE=Release.<br>
> <br>
> clang itself gets about 0.4% smaller (from 145217124 to 144631078)<br>
> lld gets about 1.85% smaller (from 101129181 to 99243810 bytes)<br>
> <br>
> Spot checking a few other random targets, in general, it seems that the benefits depend heavily on the coding style, but roughly, bigger the binary, the less benefit this brings.<br>
> <br>
> I suspect that a more aggressive pass as described by Philip Reames could capture a significant part of the benefit without sacrificing functionality. But that would require a more detailed study to be sure. <br>
<br>
A ~2% reduction in code size is a huge win.  I agree with your comment about it being different with different coding styles.  I suspect that this is the sort of thing that will pay particularly for high abstraction code bases.<br>
<br>
I don’t see why we would punish general code to make “code that is leaking where formerly not detected, and where users don’t want to mark the root as volatile”.  This seems really unprincipled to me, and a slippery slope we can’t go down.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">In the way you have restated the issue here, there is no benefit to the current behavior, but that's only because of the mistaken assumptions. You have redefined "leak", and are assuming that the problem is buggy software, whose users are upset that valid bugs are found which were not previously found. But that's simply not the case we're dealing with. The code is correct (non-leaking), and it's a regression in leak checker functionality if we start forcing users to add manual annotations as a workaround.</div><div dir="auto"><br></div><div dir="auto">I don't know what the right thing to do here is. But I'm quite sure we cannot arrive at a good decision until everyone can at least get on the same page about what the purpose of a leak checker is. I would hope that there's a path that makes everyone satisfied, but if not, the disagreement needs to be based on relative priority of use cases and engineering trade-offs, not whether the problem EXISTS.</div></div>