llvm dev - Apr 2021 - Eliminating global memory roots (or not) to help leak checkers

On Wed, 14 Apr 2021 at 09:39, 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.
>
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.

_______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210414/5f4f1e72/attachment.html>

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


On Wed, Apr 14, 2021 at 2:53 PM Vitaly Buka via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
>
> On Wed, 14 Apr 2021 at 09:39, 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.
>>
>
> 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.
>
> _______________________________________________
>> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210414/5beee354/attachment.html>