<div dir="ltr"><a class="gmail_plusreply" id="plusReplyChip-0" href="mailto:clattner@nondot.org" tabindex="-1">+Chris Lattner</a> for the last two updates as well. I really don't think this optimization is getting the overall win that it's thought to and breaking roughly every kind of leak checker mechanism I've seen.<br><div><br></div><div>-eric</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Apr 20, 2021 at 12:13 PM Sterling Augustine 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 dir="ltr">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.<div><br></div><div>clang itself gets about 0.4% smaller (from 145217124 to 144631078)</div><div>lld gets about 1.85% smaller (from 101129181 to 99243810 bytes)</div><div><br></div><div>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.</div><div><br></div><div>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. </div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Apr 19, 2021 at 4:52 PM Sterling Augustine <<a href="mailto:saugustine@google.com" target="_blank">saugustine@google.com</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 dir="ltr">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.<div><br></div><div>Although the code could definitely be improved, the comments that it is brittle or fragile seems overstated. Google uses this code without issue against against many millions of lines of code every day, in tens-of-thousands of targets, and has since 2012. It may be ugly, but it does work. We are open to improving it, if it is only the ugliness that is of concern.</div><div><br></div><div>I am working on quantifying the benefit the change gives, but I won't have results until tomorrow.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Apr 19, 2021 at 5:24 AM James Y Knight <<a href="mailto:jyknight@google.com" target="_blank">jyknight@google.com</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 dir="auto"><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">This can apparently happen simply by having the code which reads from a global variable happen to be unused or optimized away in the current compilation. Users<span style="font-family:sans-serif"> would be effectively randomly annotating variables to work around the compiler, not for any reason apparent in the code they wrote.</span></div><div dir="auto"><span style="font-family:sans-serif"><br></span></div><div dir="auto"><span style="font-family:sans-serif">I don't know what the right answer should be -- I've not looked into it at all. But I don't think it's correct to just dismiss this as not-a-problem, even if the current solution is not a good solution.</span></div><br><br><div class="gmail_quote" dir="auto"><div dir="ltr" class="gmail_attr">On Mon, Apr 19, 2021, 12:16 AM Chris Lattner via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" rel="noreferrer" target="_blank">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>Hi Sterling,<div><br></div><div>I agree with the others: therIe are better (and more robust ways) to disable leak checkers. This only addresses one narrow case. The code is also super verbose and fragile looking. This is also eliminating an optimization.</div><div><br></div><div>I’d recommend dropping the code.</div><div><br></div><div>-Chris<br><div><br><blockquote type="cite"><div>On Apr 14, 2021, at 9:38 AM, Sterling Augustine via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" rel="noreferrer noreferrer" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:</div><br><div><div dir="ltr"><div>[Continuing discussion from <a href="https://reviews.llvm.org/D69428" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D69428</a>]<br></div><div><br></div><div>Llvm is fairly conservative when eliminating global variables (or fields of such) that may point to dynamically allocated memory. This behavior is entirely to help leak checking tools such as Valgrind, Google's HeapLeakChecker, and LSAN, all of which treat memory that is reachable at exit as "not leaked", even though it will never be freed. Without these global variables to hold the pointer, the leak checkers can't determine that it is actually reachable, and will report a leak. Global variables that dynamically allocate memory but don't clean themselves up are fairly common in the wild, and various leak checkers have long not reported errors.</div><div><br></div><div>This behavior was added all the way back in 2012 in <a href="https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20120625/145646.html" rel="noreferrer noreferrer" target="_blank">https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20120625/145646.html</a>.</div><div><br></div><div><a href="https://reviews.llvm.org/D69428" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D69428</a> removed this behavior, and I subsequently reverted it when many internal Google tests started failing, but I believe many other users who use leak checking will encounter errors when this hits more mainstream releases.<br></div><div><br></div><div>So: What to do?</div><div><br></div><div>Preventing a valid transformation (the global variables are never read and can be eliminated) to help the leak checkers leaves some performance and code size on the table. Just how much is unclear.</div><div><br></div><div>On the other hand, having leak checkers suddenly start reporting failures where they didn't before also seems suboptimal. Cleaning this somewhat common scenario up is surprisingly difficult at the user level.</div><div><br></div><div>Some possibilities:</div><div><br></div><div>1. Only do this at high optimization levels, say -O3. This would give aggressive users all the performance we can, but also make leak checkers report leaks sometimes, but not others.</div><div><br></div><div>2. Hide it behind a flag or configurable option. Users who care can set it as they prefer. Creates more confusing options, different testing matrices and such, but everyone can get the behaviour that they want.</div><div><br></div><div>3. Do it all the time, and users who encounter issues can clean up their code. Users get the most performance they possibly can, but have to clean up code or drop leak checking. Seems a little user hostile.</div><div><br></div><div>Other possibilities?:</div></div>
_______________________________________________<br>LLVM Developers mailing list<br><a href="mailto:llvm-dev@lists.llvm.org" rel="noreferrer noreferrer" target="_blank">llvm-dev@lists.llvm.org</a><br><a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br></div></blockquote></div><br></div></div>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" rel="noreferrer noreferrer" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer noreferrer noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div>
</blockquote></div>
</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>