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

[David Blaikie via llvm-dev]

On Thu, Apr 22, 2021 at 4:28 PM Chris Lattner via llvm-dev
<llvm-dev at lists.llvm.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, 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 <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.
>
> 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.

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