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

[Sterling Augustine via llvm-dev]

[Continuing discussion from https://reviews.llvm.org/D69428]

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.

This behavior was added all the way back in 2012 in https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20120625/145646.html.

https://reviews.llvm.org/D69428 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.

So: What to do?

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.

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.

Some possibilities:

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.

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.

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.

Other possibilities?:

[David Blaikie via llvm-dev]

What was the original motivation for making the change - given the
gains are unclear? Might help frame what the right path forward is.

If the goal is to experiment to see if making this change is
sufficiently valuable to either regress heap leak checkers or create a
divergence where this is configurable/appears only in certain modes -
then I'd say having a flag (maybe even an internal/cc1 only flag) is
the right first step, enabling data to be gathered to help inform the
design discussion.

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

[Mehdi AMINI via llvm-dev]

Hi,

On Wed, Apr 14, 2021 at 10:02 AM David Blaikie via llvm-dev <llvm...@lists.llvm.org> wrote:

What was the original motivation for making the change - given the
gains are unclear? Might help frame what the right path forward is.

If the goal is to experiment to see if making this change is
sufficiently valuable to either regress heap leak checkers or create a
divergence where this is configurable/appears only in certain modes -
then I'd say having a flag (maybe even an internal/cc1 only flag) is
the right first step, enabling data to be gathered to help inform the
design discussion.

On Wed, Apr 14, 2021 at 9:39 AM Sterling Augustine via llvm-dev
<llvm...@lists.llvm.org> wrote:
>
> [Continuing discussion from https://reviews.llvm.org/D69428]
>
> 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.
>
> This behavior was added all the way back in 2012 in https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20120625/145646.html.
>
> https://reviews.llvm.org/D69428 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.
>
> So: What to do?
>
> 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.
>
> 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.
>
> Some possibilities:
>
> 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.
>
> 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.
>
> 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.

It seems to me that to some extent this change may expose more leaks which aren't necessarily false positives, which is a net benefit. And in cases that are indeed false positives, shouldn't the users just add `volatile` on these pointers (or `__attribute__(__used__)`, but compared to volatile it does not guarantee that the stores won't be elided I think)?

So I'm curious to know how widely affected the users of the leak detection tools will be? Because this is still quite a problem of course if the amount of false positive leaks exposed is so important that most users would then just not use the tool instead of adding the volatile annotation to these "intentional leaks". I don't know if you have some data about this?

Best,

--

Mehdi

[Nuno Lopes via llvm-dev]

Most (all?) leak checkers support suppression files. Isn’t that sufficient for your use case?

Marking your leak roots with __attribute((used))__ is also an alternative.

 

I understand that leaking memory on purpose happens because it’s expensive to clean it up. But reachable memory may well be a true leak. So flagging it as such is useful. None of us has data about the % of reachable memory that is a true leak, so it’s not possible to argue what’s user friendly/hostile.

 

Programs that leak memory on purpose are often sophisticated. And sophisticated devs can handle a little bit of extra effort to hide those smarts I think.

 

Nuno

 

P.S.: The original patch went in almost a decade ago when the ecosystem was a bit less developed. It was always meant to be temporary.

[Mehdi AMINI via llvm-dev]

James points to me that the more universally accepted definition of leaks is not "memory allocated that isn't deallocated" as I naively thought, but that the memory isn't reachable at program termination.

Under this definition, it seems like we ought to be more conservative in the optimizer instead, so I retract what I wrote previously :)

[Vitaly Buka via llvm-dev]

On Wed, 14 Apr 2021 at 09:39, Sterling Augustine via llvm-dev <llvm...@lists.llvm.org> wrote:

[Continuing discussion from https://reviews.llvm.org/D69428]

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.

This behavior was added all the way back in 2012 in https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20120625/145646.html.

https://reviews.llvm.org/D69428 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.

So: What to do?

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.

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.

Some possibilities:

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.

This could be disabled for Asan by default as it very likely runs with lsan.

 

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.

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.

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. 

 

Other possibilities?:

5.  Maybe replace writes into such removed global with no-op callback which can be intercepted by LeakChecker? This will prevent other useful optimizations.

[Fangrui Song via llvm-dev]

The motivation in https://reviews.llvm.org/D69428 was a function pointer
example. A function pointer should not point to an allocated object, so
ignoring it for root-set semantics is totally fine.

D69428 extended the function pointer cases to non-function-pointer
cases, which can be problematic.

On 2021-04-14, Nuno Lopes via llvm-dev wrote:
>Most (all?) leak checkers support suppression files. Isn’t that sufficient for your use case?

>Marking your leak roots with __attribute((used))__ is also an alternative.
>

>
>I understand that leaking memory on purpose happens because it’s expensive to clean it up. But reachable memory may well be a true leak. So flagging it as such is useful. None of us has data about the % of reachable memory that is a true leak, so it’s not possible to argue what’s user friendly/hostile.

As is, many code patterns in various projects can be affected by the
aggressive optimization. They may use a global pointer referencing an
allocated object as a replacement for a global variable with a
non-trivial destructor ([[clang::no_destroy]] :). Dynamic destruction is
not ordered across translation units, this can lead to all sorts of
static finalization order fiasco problems. If there are threads not
joined at exit time, some threads may access objects which have been
destructed.

In addition, the leak checker may be registered as an atexit callback
instead of running after all destructors have run.
If the leak checker is registered by atexit, normally it runs before
destructors. Even if you have sophisticated destructors which deallocate
objects properly, if you ignore them as roots, the checker will report
false positives.

>Programs that leak memory on purpose are often sophisticated. And sophisticated devs can handle a little bit of extra effort to hide those smarts I think.
>
>
>
>Nuno
>
>
>
>P.S.: The original patch went in almost a decade ago when the ecosystem was a bit less developed. It was always meant to be temporary.
>
>
>
>
>
>From: Sterling Augustine
>Sent: 14 April 2021 17:39
>To: llvm-dev <llvm...@lists.llvm.org>
>Subject: [llvm-dev] Eliminating global memory roots (or not) to help leak checkers
>
>
>
>[Continuing discussion from https://reviews.llvm.org/D69428]
>
>
>
>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.
>
>
>
>This behavior was added all the way back in 2012 in https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20120625/145646.html.
>
>
>
>https://reviews.llvm.org/D69428 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.
>
>
>
>So: What to do?
>
>
>
>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.
>
>
>
>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.
>
>
>
>Some possibilities:
>
>
>
>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.
>
>
>
>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.
>
>
>
>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.
>
>
>
>Other possibilities?:
>

>_______________________________________________

[Philip Reames via llvm-dev]

Don't really have an opinion on the question as asked, but want to point out an alternate framing.   I will comment that the code being removed looks more than a bit fragile. 

From a very quick skim of the original code, it appears to be focused on DCE of globals.  If we wanted to keep the "leak detector safe" semantic, but allow more aggressive optimization, we could re-imagine this as global SROA.  We could deconstruct the struct/array/etc and keep only the fields which could potentially be allocation roots.  We could also write an optimization which leverages the knowledge of the allocation root being otherwise unused to eliminate mallocs which are stored into them.

I haven't fully thought that through, but it seems like we have quite a bit of room to optimize better without changing our handling for the leak detectors. 

Philip

[Kostya Serebryany via llvm-dev]

I would expect this change to cause a large cleanup and render lsan unusable on any non-trivial code base. 

But indeed, it would be nice to see the data. 

#1 is undesirable as it will make asan/lsan behave differently depending on the opt level. 

We can disable the transformation under asan (which includes lsan), 

but it will be harder to disable it under pure lsan (which is a link-time option). 

#2 might be the right first step to demonstrate the impact on code size and on lsan. 

Suppressions are not a good way to fix this (and generally, suppressions are almost never a good long term answer). 

Creating suppressions will take more work than adding __attribute((used))__, will be harder to maintain,

and will cause true leaks to be lost. 

__attribute((used))__ is not a complete solution either. 

imagine a code base which imports a third_party code base verbatim and can't add attributes there. 

Philip's suggestion is worth investigating.

<untested proposal>

For globals that are never read and are written to only once, we may have an lsan-specific solution, 

by calling __lsan_enable/__lsan_disable around the to-be-removed assignment.

Globals that are written to multiple times will be harder to handle. 

--kcc 

[Chris Lattner via llvm-dev]

Hi Sterling,

I agree with the others: there 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.

I’d recommend dropping the code.

-Chris

[James Y Knight via llvm-dev]

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.

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 would be effectively randomly annotating variables to work around the compiler, not for any reason apparent in the code they wrote.

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.

On Mon, Apr 19, 2021, 12:16 AM Chris Lattner via llvm-dev <llvm...@lists.llvm.org> wrote:

Hi Sterling,

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.

[Sterling Augustine via llvm-dev]

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.

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.

I am working on quantifying the benefit the change gives, but I won't have results until tomorrow.

[Sterling Augustine via llvm-dev]

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. 

[Eric Christopher via llvm-dev]

+Chris Lattner 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.

-eric

[Chris Lattner via llvm-dev]

> On Apr 19, 2021, at 5:24 AM, James Y Knight <jykn...@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, 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 :)

> On Apr 19, 2021, at 4:52 PM, Sterling Augustine <saugu...@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.

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 <saugu...@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.

-Chris

[pawel k. via llvm-dev]

Hello,

Seems like long and interesting thread. I would love to learn what problem were facing here. Maybe im ignorant enough to try to tackle it. 

Famous quote: I could make great violin player, i never tried it.

Haha __attribute(immortal) on global variable.

Stupid man from poland would say its volatile keyword abuse but maybe minor and what do i know.

Can someone briefly describe key problematic code, what component fails and how do we want to workaround it? I could study pros and cons and maybe come up with set of proposals.

Best regards,

Pawel Kunio

[David Blaikie via llvm-dev]

On Thu, Apr 22, 2021 at 4:28 PM Chris Lattner via llvm-dev
<llvm...@lists.llvm.org> wrote:
>
>
>
> > On Apr 19, 2021, at 5:24 AM, James Y Knight <jykn...@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, 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 :)

"spuriously" reachable is quite questionable here. This way of writing
code is to make the allocation quite intentionally reachable from a
global.

For instance, one of the ways to disable global destruction: (
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
)

"If all else fails, you can create an object dynamically and never
delete it by using a function-local static pointer or reference (e.g.,
static const auto& impl = *new T(args...);)."

(all else fails in a large enough codebase rather often... )

> > On Apr 19, 2021, at 4:52 PM, Sterling Augustine <saugu...@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.
>
> 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 <saugu...@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.

I don't think it's as general or open ended as that - it's a fairly
specific carveout that's been used pretty broadly I think.

So - any interest in a flag to control this behavior, if that's what
it comes to? It's one that a fair bit of software relies on.

- Dave

[Eric Christopher via llvm-dev]

+Chris Lattner to make sure Chris sees this response too.

[Sterling Augustine via llvm-dev]

Have any of the proponents of the patch tried lsan against a reasonable large code base both with and with out it?

[Fangrui Song via llvm-dev]

On 2021-04-20, Eric Christopher via llvm-dev wrote:
>+Chris Lattner <clat...@nondot.org> 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.
>
>-eric

A sanitizer maintainer has contributed a LeakSanitizer patch to ensure
we don't regress (https://reviews.llvm.org/D100906).
(The test can test the false positive under valgrind which would be
introduced by the original GlobalOpt patch (D69428) as well.)

Just few days ago, I used this pattern to suppress a leak in the in-tree
project LLDB. (https://reviews.llvm.org/D100806).

To bring back some potentially lost optimization, we can try making the
GlobalOpt heuristic smarter, e.g. recognize function pointer types
(as I suggested in the very first few comments on D69428).
Then there is also Philip's suggestion to improve global variable SROA.

A simple code removal does not pull its weight and can cause large
friction to various downstream projects.

[Evgeny Leviant via llvm-dev]

My 2 cents:

 

1. 2% of code size improvement is alone a strong argument to revert r160529. In embedded world it sometimes determines whether image fits the ROM or not.

 

2. Code/data size reduction often correlates with performance improvements, due to smaller number of cache refills. I suspect data segment was reduced somewhat more than those 2%.

 

3. I don’t use LSAN and the whole idea that “what is reachable is not a leak” isn’t applicable to my use cases quite well. There are other ways to find leaking memory which do not prevent compiler optimizations. I suspect many people also don’t use LSAN, why should they suffer?

 

4. As it was already mentioned patch introduced by r160529 is of substandard quality and is not properly covered by test cases. This is yet another argument for removal.

 

5. I still can’t understand why LSAN instrumentation can’t prevent globals from being removed, delegating this to globalopt instead. Can someone explain this to me, please?

 

 

From: Sterling Augustine via llvm-dev
Sent: 23 апреля 2021 г. 3:44
To: Chris Lattner
Cc: llvm-dev
Subject: [EXTERNAL] Re: [llvm-dev] Eliminating global memory roots (or not) to help leak checkers

 

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.  If you suspect potential phishing or spam email, report it to Repor...@accesssoftek.com

[James Y Knight via llvm-dev]

On Thu, Apr 22, 2021, 7:28 PM Chris Lattner <clat...@nondot.org> wrote:

> On Apr 19, 2021, at 5:24 AM, James Y Knight <jykn...@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. 

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 <saugu...@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 <saugu...@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.

[pawel k. via llvm-dev]

Hello,

If its about runtime leak checking rather than compile time we might be prying the already open door here. Aka i might have something important to say.

As of code sample at what url can i see the code that is or isnt a leak or can somebody tell where its allocated where its freed and as i understand its stored in global var ptr,  right?

Best regards,

Pawel Kunio

[pawel k. via llvm-dev]

For runtime leak checker rather than compile time, my solution doesnt give false positives thus fewer supressions needed for complete well written albeit possibly leaky app. We might need them for testsuite incomplete code if its scanned with it but that would feel silly.

Hint: i saw it somewhere and liked it. We might think of version with bit ugly syntax or pretty one but needing some compiler support for adding lineno and srcfilename parameters to alloc fun call.

If its compile time checker, apologies and sorry for intrusion.

Best regards,

Pawel Kunio

pt., 23.04.2021, 16:19 użytkownik James Y Knight via llvm-dev <llvm...@lists.llvm.org> napisał:

[Philip Reames via llvm-dev]

On 4/23/21 7:18 AM, James Y Knight via llvm-dev wrote:

On Thu, Apr 22, 2021, 7:28 PM Chris Lattner <clat...@nondot.org> wrote:

> On Apr 19, 2021, at 5:24 AM, James Y Knight <jykn...@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 <saugu...@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 <saugu...@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.

[Sterling Augustine via llvm-dev]

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.

[Fangrui Song via llvm-dev]

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)

>> LLVM Developers mailing listllvm-dev@lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

[pawel k. via llvm-dev]

Hello,

*rant on*

If i was to say what we needed on couple embedded projects, if you alloc and not free it was a leak. It matters for nano archs and some rtoses. If you alloc and optimize out the store target and not free its still a leak and harder to track. If it suggests leaks in compile with 80% prob, nice to have but if runtime catches 100% leaks well still use runtime version possibly with compile time check to catch em early ones that are catchable.

Soo from ci perspective, before doing pull request you should run testsuite with runtime checks no matter you do compile checks or not. On commit both compile and runtime testsuite checks will be run.

Best regards,

Pk

[Chris Lattner via llvm-dev]

Sorry for the delay, been preoccupied with other things.  Responding to a couple of points:

On Apr 22, 2021, at 4:40 PM, David Blaikie <dbla...@gmail.com> wrote:

Code that this triggers *is* leaking memory, 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 :)

"spuriously" reachable is quite questionable here. This way of writing
code is to make the allocation quite intentionally reachable from a
global.

For instance, one of the ways to disable global destruction: (
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
)

"If all else fails, you can create an object dynamically and never
delete it by using a function-local static pointer or reference (e.g.,
static const auto& impl = *new T(args...);)."

(all else fails in a large enough codebase rather often... )

Yes, I think that it is a pragmatic point that there is a lot of code out here doing this, and it isn’t practical to expect people to mark everything with leak checkers.  This was reinforced by other comments, e.g.:

On Apr 23, 2021, at 7:18 AM, James Y Knight <jykn...@google.com> wrote:

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.

There are several other good comments that are making similar observations. I would paraphrase this as saying that “C/C++ in practice use the ‘assign pointer to a global to silence leak checkers’” as a sort of extended contract that makes these leak checkers work.  While use of volatile or attribute((used)) could theoretically be used to deal with this, I think we can all agree there is a lot of code in the wild and it isn’t reasonable to change it all, and I have no interest in breaking the world.

But here’s the problem: LLVM isn’t a C compiler.  It is used for a wide variety of purposes, including many languages that are not C.  LLVM contains a set of optimizations that can be used to do many different things, and the guiding principle is that it has an IR which defines which set of transformations are safe.  Deleting these “roots” is an entirely reasonable thing for GlobalOpt to do given the IR as is defined in LangRef.  Even C and C++ dump out global variable metadata (e.g. RTTI info or tables etc) which contain pointers to static data that is conceptually safe to squeeze according to the existing “as if” rule.

I see two paths forward here, the first of which is more principled:

1) Introduce a new “may contain a root" IR attribute that can be applied to a global variable that says “don’t delete pointer members that may root heap allocated memory”.  This captures the property in the IR, and then of course GlobalOpt would look at this to gate its transformation.  By defining this property carefully, we can allow global opt to delete pointers to static memory and other things that aren’t a problem.  Given this IR attribute, clang would generously slather it on IR global variables in the right place - we could easily add a clang command line flag to control this if needed.

2) We can change clang to tip toe around this, e.g. by adding a flag to the GlobalOpt pass itself, and having Clang run the GlobalOpt pass in a mode that disables this behavior.

Either of these would keep LLVM-the-mid-level-optimizer true to what it should be doing, without breaking the C community and associated tools.

-Chris