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

[Mehdi AMINI via llvm-dev]

On Wed, Apr 14, 2021 at 10:22 AM Mehdi AMINI <joker.eph at gmail.com> wrote:

> Hi,
>
>
> On Wed, Apr 14, 2021 at 10:02 AM David Blaikie via llvm-dev <
> llvm-dev at 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-dev at 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)?
>

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 :)


> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210414/6f3374a2/attachment.html>