[llvm-dev] Eliminating global memory roots (or not) to help leak checkers

Fri Apr 23 18:35:54 PDT 2021

Follow-up to stage 2 clang size.

The optimization we see with D69428 is all due to `STATISTIC(...)`
variables (there are ~1000).

llvm::NoopStatistic has 3 unused global pointers.  With
https://reviews.llvm.org/D101211 , all the `STATISTIC(...)` contribute
zero size. I don't see D69428 removes any global variable which should
be dropped.

D69428 would drop two global variables which should be retained,
but that is usage issue. I've sent https://reviews.llvm.org/D101217 to fix them.

So at least for Clang, we don't gain any size improvement from D69428.
BTW, it is possible that much of D69428's size gain can be provided by
linker garbage collection(ELF)/dead stripping(Mach-O)/"eliminates
functions and data that are never referenced" (link.exe).
So in general projects, I expect its gain to be tiny.

D69428 and the thread does raise much to improve. e.g. I suspect with
D69428 there are still some `STATISTIC(...)` not optimized out.  There
are definitely optimization/code cleanup/test improvement opportunities there.

>>> > clang itself gets about 0.4% smaller (from 145217124 to 144631078)
>>> > lld gets about 1.85% smaller (from 101129181 to 99243810 bytes)

On 2021-04-23, Sterling Augustine via llvm-dev wrote:
>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).
>
>There is plenty of room for wins here while preserving the functionality.
>
>For those who say the argument is over semantics about whether or not that
>is a leak:
>
>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.
>
>On Fri, Apr 23, 2021 at 8:31 AM Philip Reames via llvm-dev <
>llvm-dev at lists.llvm.org> wrote:
>
>>
>> On 4/23/21 7:18 AM, James Y Knight via llvm-dev wrote:
>>
>>
>>
>> On Thu, Apr 22, 2021, 7:28 PM Chris Lattner <clattner at nondot.org> wrote:
>>
>>>
>>>
>>> > On Apr 19, 2021, at 5:24 AM, James Y Knight <jyknight at google.com>
>>> wrote:
>>> >
>>> > 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.
>>>
>>> I think that “from the point of view of the leak checker” is the key
>>> thing there.
>>>
>>> Code that this triggers *is* leaking memory
>>
>>
>> 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.
>>
>> I'm afraid you're still arguing from mistaken assumptions. As I've
>> already mentioned, reachable memory at program exit is not a leak. 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.)
>>
>> 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.
>>
>> The code is correct and it's not leaking memory, but with this change, the
>> leak checker is unable to determine that.
>>
>> I want to object here.  :)
>>
>> 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.
>>
>> 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.)
>>
>> Separately from the terminology point above, I'll share my own weakly held
>> opinion from reading along with this thread.
>>
>> 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.
>>
>> 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!)
>>
>>
>>
>>
>> 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 :)
>>
>>
>> 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.)
>>
>>
>> > On Apr 19, 2021, at 4:52 PM, Sterling Augustine <saugustine at google.com>
>>> wrote:
>>> >
>>> > 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.
>>>
>>> I can see that concern, but this isn’t a battle that we can win:
>>> optimizations in general can expose leaks.
>>>
>>
>> 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 "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.)
>>
>> 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.
>>
>>
>> > On Apr 20, 2021, at 9:12 AM, Sterling Augustine <saugustine at google.com>
>>> wrote:
>>> >
>>> > 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.
>>> >
>>> > clang itself gets about 0.4% smaller (from 145217124 to 144631078)
>>> > lld gets about 1.85% smaller (from 101129181 to 99243810 bytes)
>>> > 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.
>>> >
>>> > 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.
>>>
>>> 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.
>>>
>>> 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.
>>>
>>
>> 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.
>>
>> 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.
>>
>> _______________________________________________
>> LLVM Developers mailing listllvm-dev at lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>

>_______________________________________________
>LLVM Developers mailing list
>llvm-dev at lists.llvm.org
>https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev