<div dir="ltr">I want to point out that it doesn't need to be either 1. Good leak checking and the current lack of optimizations. Or 2. Bad leak checking and somewhat better optimization. (incidentally, 1.85% in clang was the _best_ improvement I saw, not the expected case).<div><br></div><div>There is plenty of room for wins here while preserving the functionality.</div><div><br></div><div>For those who say the argument is over semantics about whether or not that is a leak: </div><div><br></div><div>I would need to double check, but given 1. All three leak checkers work this way. 2. Those checkers work well with other compilers. It seems to me that other compilers behave this way too.<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Apr 23, 2021 at 8:31 AM Philip Reames via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<p><br>
</p>
<div>On 4/23/21 7:18 AM, James Y Knight via
llvm-dev wrote:<br>
</div>
<blockquote type="cite">
<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" rel="noreferrer" target="_blank">clattner@nondot.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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>
</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. <br>
</font></div>
</div>
</blockquote>
<p>I want to object here. :)</p>
<p>A program with dynamic allocation which has not been reclaimed by
program termination does have a leak. It simply happens to be a
leak that we've chosen by convention to not treat as interesting.
This is a reasonable convention because standard process tear
mechanisms will deallocate it for most classes of memory. The
program could be converted to one which actually doesn't leak by
using static destructors to free the pointed to object. <br>
</p>
<p>Just to be clear, this objection is purely on terminology. I
think it's important to distinguish between programs which leak
(e.g. don't reclaim all memory), and programs which simply aren't
interesting from a leak detection standpoint (because the memory
is about to be reclaimed anyways.)</p>
<p>Separately from the terminology point above, I'll share my own
weakly held opinion from reading along with this thread.</p>
<p>I have generally found the arguments against optimizing away
globals to avoid leak reports unconvincing. The results on the
optimization benefit are clearly worthwhile. If forced to chose
at this moment, I'd trade the optimization impact for the leak
detection usage complexity. To me, it is critical to note that
there are multiple source level changes possible to address the
(true) leaks reported, several of which have already been
suggested in this thread. It's also important to note that we
have other optimizations already in tree which require the same
type of source change. <br>
</p>
<p>I would suggest that if the advocates for leak suppression in the
compiler continue to want to argue this point that the burden of
work needs to shift. In particular, I would really like to see
some proactive efforts to either a) assess the optimization
potential tradeoff of an SROA-ish approach, or b) proposals for
making the desired preservation well defined in IR. (i.e. a set
of rules which describe which optimizations are legal - the
current code does not do this!)<br>
</p>
<p><br>
</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="auto">
<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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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>
<br>
<fieldset></fieldset>
<pre>_______________________________________________
LLVM Developers mailing list
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a>
</pre>
</blockquote>
</div>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>