[PATCH] Don't localize globals that may serve as leak checker roots.

[Reid Kleckner]

On Thu, Sep 26, 2013 at 1:36 AM, Alexey Samsonov <samsonov at google.com>wrote:

>
> On Thu, Sep 26, 2013 at 12:37 AM, Rafael Espíndola <
> rafael.espindola at gmail.com> wrote:
>
>> >> I agree. Wouldn't it be better for clang to attach metadata to a
>> >> global variable to say it is a GC root? That way the optimization is
>> >> only disabled when using a sanitizer.
>> >
>> >
>> > Huh? Isn't it just as broken with valgrind's leak checker? If not, why
>> isn't
>> > it?
>>
>> I assume it is broken with valgrind, but the llvm language spec
>> doesn't say we cannot hide "leak check roots". That is why I think
>> this should be controllable from the FE with an attribute (I now
>> realize it cannot be metadata since we don't want it dropped).
>>
>
> I've verified that this optimization is indeed broken with valgrind.
> Personally, I'm not sure this optimization is very useful - as Reid
> mentions,
> it fires for a very limited set of globals, and localizes them only to
> main(). In the meantime,
> it causes noticeable problems with leak checkers deployment - storing a
> pointer to a
> never-deleted object in a global variable seems to be a common pattern.
>

IMO moving globals to locals in main is basically a SPEC benchmark hack.
 It's only profitable when most of the program can be inlined into main and
the globals become register allocated.

The C++ standard sez [basic.start.main]p6: "The function main shall not be
used (3.2) within a program."  The C standard has no such language, so this
optimization isn't really valid for C.  IIRC this caused PNaCl some
problems.

So if we want to remove this optimization, it should be on *that* basis,
and not because we want leak checking to work out of the box.


> The long way - introduce a new -fsanitize=valgrind flag (are there another
> use cases for this?),
> teach FE to attach specific newly-introduced attributes to emitted globals
> if any sanitizer is turned on,
> and then consult these attributes in (already working) isLeakCheckerRoot()
> function - seems like an overkill,
> and will still be a problem for Valgrind users (after all, the benefit of
> this tool is that you don't have to recompile
> your code and adding -fsanitize=valgrind might be annoying)...
>
>
>> Cheers,
>> Rafael
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
>
> --
> Alexey Samsonov, MSK
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130926/5dcf8da3/attachment.html>