<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jan 9, 2014 at 3:11 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 09/01/2014 22:41, Richard Smith wrote:<div class="im"><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Thu, Jan 9, 2014 at 2:29 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a> <mailto:<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a><u></u>>> wrote:<br>
<br>
If we're going to back out the revert, can we put the code into an<br>
#ifdef so that the reserved namespace identifier is protected when<br>
compiling with something that doesn't understand lsan? Then we can<br>
argue over the "right" way with some protection.<br>
<br>
<br>
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),<br>
</blockquote>
<br></div>
Hi Richard,<br>
<br>
The part of the specification in question relates to global names, and this thread is about an extern "C" function definition.<br>
<br>
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?</blockquote>
<div><br></div><div>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).</div><div><br></div><div>
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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
</blockquote>
<br></div>
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.<br>
</blockquote><div><br></div><div>That's simply not true; we have lots of such names currently.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Aaron's suggestion is a way forward and in line with Jordan's earlier LEAK_SANITIZER ifdef suggestion if someone wants to try that.<br>
<br>
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.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
(And everything aside, it's also just so much dead-code in non-sanitizer builds.)<br>
<br>
Alp.<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
<br>
On Thu, Jan 9, 2014 at 5:07 PM, Richard Smith<br></div><div class="im">
<<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a> <mailto:<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>><u></u>> wrote:<br>
> Please back out this revert. We are far from achieving consensus<br>
on this<br>
> revert being appropriate.<br>
><br>
><br>
> On Thu, Jan 9, 2014 at 11:43 AM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br></div><div><div class="h5">
<mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
>><br>
>> Author: alp<br>
>> Date: Thu Jan 9 13:43:17 2014<br>
>> New Revision: 198885<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=198885&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=198885&view=rev</a><br>
>> Log:<br>
>> Revert "Disable LeakSanitizer in TableGen binaries, see PR18325"<br>
>><br>
>> To declare or define reserved identifers is undefined behaviour in<br>
>> standard<br>
>> C++. This needs to be addressed in compiler-rt before it can be<br>
used in<br>
>> LLVM.<br>
>><br>
>> See the list discussion for details.<br>
>><br>
>> This reverts commit r198858.<br>
>><br>
>> Modified:<br>
>> cfe/trunk/utils/TableGen/<u></u>TableGen.cpp<br>
>><br>
>> Modified: cfe/trunk/utils/TableGen/<u></u>TableGen.cpp<br>
>> URL:<br>
>><br>
<a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/utils/TableGen/TableGen.cpp?rev=198885&r1=198884&r2=198885&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/cfe/trunk/utils/<u></u>TableGen/TableGen.cpp?rev=<u></u>198885&r1=198884&r2=198885&<u></u>view=diff</a><br>
>><br>
>><br>
==============================<u></u>==============================<u></u>==================<br>
>> --- cfe/trunk/utils/TableGen/<u></u>TableGen.cpp (original)<br>
>> +++ cfe/trunk/utils/TableGen/<u></u>TableGen.cpp Thu Jan 9 13:43:17 2014<br>
>> @@ -263,10 +263,3 @@ int main(int argc, char **argv) {<br>
>><br>
>> return TableGenMain(argv[0], &ClangTableGenMain);<br>
>> }<br>
>> -<br>
>> -extern "C" {<br>
>> -// Disable LeakSanitizer for this binary as it has too many<br>
leaks that<br>
>> are not<br>
>> -// very interesting to fix. __lsan_is_turned_off is explained in<br>
>> -// compiler-rt/include/sanitizer/<u></u>lsan_interface.h<br>
>> -int __lsan_is_turned_off() { return 1; }<br>
>> -} // extern "C"<br>
>><br>
>><br>
>> ______________________________<u></u>_________________<br>
>> cfe-commits mailing list<br></div></div>
>> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a> <mailto:<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.<u></u>edu</a>><div class="im">
<br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><br>
><br>
><br>
><br>
> ______________________________<u></u>_________________<br>
> cfe-commits mailing list<br></div>
> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a> <mailto:<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.<u></u>edu</a>><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><br>
><br>
<br><span class="HOEnZb"><font color="#888888">
</font></span></blockquote><span class="HOEnZb"><font color="#888888">
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</font></span></blockquote></div><br></div></div>