On 09/01/2014 23:18, Richard Smith wrote:
On Thu, Jan 9, 2014 at 3:11 PM, Alp Toker <[email protected]
<mailto:[email protected]>> wrote:
On 09/01/2014 22:41, Richard Smith wrote:
On Thu, Jan 9, 2014 at 2:29 PM, Aaron Ballman
<[email protected] <mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>> 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).
For what it's worth plenty of those cases should go away if we handle
contextual keywords automatically in the token stream.
But overall the C++ standard implies that keeping the _global namespace_
clear of user-defined reserved names is the priority, and we've seen
real problems where conflicts with compiler builtins and system headers.
The spec section is even called "global.names" so I think it's fair to
read into that a little.
The cases you mention are "interesting" in a pedantic (and correct)
sense, but as discussed in the earlier mail with Sean, there's
diminishing value to the rules as you go down the list.
Macro define/undefine and global names conflicts do cause trouble all
the time. So while you can argue to ignore some of the rules, I don't
see any justification to ignore the whole of 17.6.4.3.
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.
Yes, that'll be good clean up too.
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.
Already established. We're talking about cases where the implementation
is not using the sanitizer (the vast majority of builders and end-user
configurations) and where the definition is therefore undefined behaviour.
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.
We all have different priorities Richard. I actually had to go back to
work to help out with this, and took half the day to get an
understanding of the original intention, discussing with others. It was
certainly not a summary decision and even came with a heads-up.
When someone reverts or reports potential problems it's best to be
grateful. If nothing else, that takes the pressure off having to get
every commit right every time -- all of this is preferable to working in
silos without review or feedback.
Alp.
(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
<[email protected] <mailto:[email protected]>
<mailto:[email protected] <mailto:[email protected]>>>
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
<[email protected] <mailto:[email protected]>
<mailto:[email protected] <mailto:[email protected]>>> 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
>> [email protected]
<mailto:[email protected]>
<mailto:[email protected] <mailto:[email protected]>>
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
>
> _______________________________________________
> cfe-commits mailing list
> [email protected] <mailto:[email protected]>
<mailto:[email protected] <mailto:[email protected]>>
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
--
http://www.nuanti.com
the browser experts
--
http://www.nuanti.com
the browser experts
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
