r198885 - Revert "Disable LeakSanitizer in TableGen binaries, see PR18325"

[Richard Smith]

On Thu, Jan 9, 2014 at 3:11 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 09/01/2014 22:41, Richard Smith wrote:
>
>  On Thu, Jan 9, 2014 at 2:29 PM, Aaron Ballman <aaron at aaronballman.com<mailto:
>> aaron at aaronballman.com>> wrote:
>>
>>     If we're going to back out the revert, can we put the code into an
>>     #ifdef so that the reserved namespace identifier is protected when
>>     compiling with something that doesn't understand lsan? Then we can
>>     argue over the "right" way with some protection.
>>
>>
>> I'm not opposed to that, if we have a suitable predefine. But we already
>> have *loads* of code in Clang that defines identifiers in the reserved
>> namespace (try grepping for '[A-Za-z]__[A-Za-z]' in include/ to find a
>> bunch of them),
>>
>
> Hi Richard,
>
> The part of the specification in question relates to global names, and
> this thread is about an extern "C" function definition.
>
> Grepping for '[A-Za-z]__[A-Za-z]' through include/ and also the rest of
> lib/ doesn't show any names that are obviously used in an invalid context,
> but it's possible I've missed them -- could you clarify?


Every single hit for this regex is using the name in an invalid context.
Names containing double underscores are reserved to the implementation for
any use (in every context).

We also have some uses of names starting underscore-capital, mostly as
template parameter names, in Clang's headers. Those ones are probably worth
fixing.

 and none of our supported C++ implementations (for clang 3.5) have a
>> problem with this, so I don't see that there's a lot of value in doing so.
>>
>
> There's always value in keeping to standard C++, and keeping non-standard
> extensions guarded if they ever become a necessity as we already do with
> other constructs in LLVM. We've done fine so far and the whole source tree
> is valid save for the recent problematic commit.
>

That's simply not true; we have lots of such names currently.


> Aaron's suggestion is a way forward and in line with Jordan's earlier
> LEAK_SANITIZER ifdef suggestion if someone wants to try that.
>
> I'm still not convinced it's good form for compiler-rt, but I'd be OK
> having the definitions re-landed with a suitable ifdef guard.
>

When using a sanitizer, that sanitizer is *part of the implementation*, so
it gets to say what double-underscore names mean. And in this case, it says
this double-underscore name is available to be defined by the program.

I don't disagree that it'd be better to avoid defining this function
outside of a sanitizer. But I don't think it's high priority, and it's
certainly not revert-the-patch priority.


> (And everything aside, it's also just so much dead-code in non-sanitizer
> builds.)
>
> Alp.
>
>
>
>>     On Thu, Jan 9, 2014 at 5:07 PM, Richard Smith
>>     <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> wrote:
>>     > Please back out this revert. We are far from achieving consensus
>>     on this
>>     > revert being appropriate.
>>     >
>>     >
>>     > On Thu, Jan 9, 2014 at 11:43 AM, Alp Toker <alp at nuanti.com
>>     <mailto:alp at nuanti.com>> wrote:
>>     >>
>>     >> Author: alp
>>     >> Date: Thu Jan  9 13:43:17 2014
>>     >> New Revision: 198885
>>     >>
>>     >> URL: http://llvm.org/viewvc/llvm-project?rev=198885&view=rev
>>     >> Log:
>>     >> Revert "Disable LeakSanitizer in TableGen binaries, see PR18325"
>>     >>
>>     >> To declare or define reserved identifers is undefined behaviour in
>>     >> standard
>>     >> C++. This needs to be addressed in compiler-rt before it can be
>>     used in
>>     >> LLVM.
>>     >>
>>     >> See the list discussion for details.
>>     >>
>>     >> This reverts commit r198858.
>>     >>
>>     >> Modified:
>>     >>     cfe/trunk/utils/TableGen/TableGen.cpp
>>     >>
>>     >> Modified: cfe/trunk/utils/TableGen/TableGen.cpp
>>     >> URL:
>>     >>
>>     http://llvm.org/viewvc/llvm-project/cfe/trunk/utils/
>> TableGen/TableGen.cpp?rev=198885&r1=198884&r2=198885&view=diff
>>     >>
>>     >>
>>     ============================================================
>> ==================
>>     >> --- cfe/trunk/utils/TableGen/TableGen.cpp (original)
>>     >> +++ cfe/trunk/utils/TableGen/TableGen.cpp Thu Jan  9 13:43:17 2014
>>     >> @@ -263,10 +263,3 @@ int main(int argc, char **argv) {
>>     >>
>>     >>    return TableGenMain(argv[0], &ClangTableGenMain);
>>     >>  }
>>     >> -
>>     >> -extern "C" {
>>     >> -// Disable LeakSanitizer for this binary as it has too many
>>     leaks that
>>     >> are not
>>     >> -// very interesting to fix. __lsan_is_turned_off is explained in
>>     >> -// compiler-rt/include/sanitizer/lsan_interface.h
>>     >> -int __lsan_is_turned_off() { return 1; }
>>     >> -}  // extern "C"
>>     >>
>>     >>
>>     >> _______________________________________________
>>     >> cfe-commits mailing list
>>     >> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>>
>>     >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>     >
>>     >
>>     >
>>     > _______________________________________________
>>     > cfe-commits mailing list
>>     > cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>>     > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>     >
>>
>>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140109/4d7f1a5f/attachment.html>