<div dir="ltr"><a class="gmail_plusreply" id="plusReplyChip-1" href="mailto:clattner@nondot.org" tabindex="-1">+Chris Lattner</a> to make sure Chris sees this response too.<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Apr 14, 2021 at 7:04 PM Kostya Serebryany 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">I would expect this change to cause a large cleanup and render lsan unusable on any non-trivial code base. <div>But indeed, it would be nice to see the data. <br><div><br></div><div>#1 is undesirable as it will make asan/lsan behave differently depending on the opt level. </div><div>We can disable the transformation under asan (which includes lsan), </div><div>but it will be harder to disable it under pure lsan (which is a link-time option). </div><div><br></div><div>#2 might be the right first step to demonstrate the impact on code size and on lsan. </div><div><br></div><div>Suppressions are not a good way to fix this (and generally, suppressions are almost never a good long term answer). </div><div>Creating suppressions will take more work than adding __attribute((used))__, will be harder to maintain,</div><div>and will cause true leaks to be lost. </div><div><br></div><div>__attribute((used))__ is not a complete solution either. <br></div><div>imagine a code base which imports a third_party code base verbatim and can't add attributes there. </div><div><br></div>Philip's suggestion is worth investigating.</div><div><br></div><div><untested proposal></div><div>For globals that are never read and are written to only once, we may have an lsan-specific solution, </div>by calling __lsan_enable/__lsan_disable around the to-be-removed assignment.<div>Globals that are written to multiple times will be harder to handle. </div><div><br></div><div>--kcc <br><div><br></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Apr 14, 2021 at 2:53 PM Vitaly Buka via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" 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 dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 14 Apr 2021 at 09:39, Sterling Augustine via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" 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 dir="ltr"><div>[Continuing discussion from <a href="https://reviews.llvm.org/D69428" 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" 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" 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></blockquote><div><br></div><div>This could be disabled for Asan by default as it very likely runs with lsan.</div><div> </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"><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></blockquote><div><br></div><div>I expect this requires significant cleanup effort, and not just in Google. It's quite a common pattern, but it would be nice to see some real data. </div><div> </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"><div><br></div><div>Other possibilities?:</div></div></blockquote><div><br></div><div>5.  Maybe replace writes into such removed global with no-op callback which can be intercepted by LeakChecker? This will prevent other useful optimizations.</div><div><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>
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></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>
_______________________________________________<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>