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

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
> On Apr 14, 2021, at 9:38 AM, Sterling Augustine via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
> 
> [Continuing discussion from https://reviews.llvm.org/D69428
<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://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20120625/145646.html>.
> 
> https://reviews.llvm.org/D69428 <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?:
> _______________________________________________
> 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/20210418/2a7e00ae/attachment-0001.html>

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-dev at 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.
>
> I’d recommend dropping the code.
>
> -Chris
>
> On Apr 14, 2021, at 9:38 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.
>
> Other possibilities?:
> _______________________________________________
> 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/20210419/b62d5998/attachment.html>