r198858 - Disable LeakSanitizer in TableGen binaries, see PR18325

[Kostya Serebryany]

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

>
> On 09/01/2014 11:36, Chandler Carruth wrote:
>
>
>> On Thu, Jan 9, 2014 at 3:15 AM, Alp Toker <alp at nuanti.com <mailto:
>> alp at nuanti.com>> wrote:
>>
>>     __builtin functions are introduced by the implementation (either
>>     the compiler or system header declarations) so there's no problem.
>>
>>     Sanitizer is a system library so it's free to use the identifiers
>>     it wants.
>>
>>
>> The sanitizers aren't system libraries as I define them (things in
>> /usr/include or /usr/lib). They're compiler runtime libraries. They get
>> installed in lib/clang/... along with other compiler resources. They are
>> tightly coupled and version locked with the compiler itself as they
>> interface with compiler generated instrumentation in most cases. The leak
>> sanitizer is unusual in that it *could* be separated into something other
>> than the compiler runtime libraries, but we explicitly chose not to as a
>> design goal: we want to be able to offer it coupled with address sanitizer.
>>
>> Also, system libraries aren't free to use reserved names... so maybe you
>> meant something different from what is commonly called a system library on
>> linux/mac?
>>
>
> s/libraries/headers/ clearly.
>
>
>
>
>>
>>     TableGen, on the other hand, is not a part of the implementation.
>>
>>
>> No one is claiming that it is.
>>
>>
>>
>>
>>         Can you clarify exactly what the error is and why? It's
>>         possible we could provide a macro of some kind in the
>>         sanitizer headers to bundle this functionality up if we need
>>         some special marking of this for checkers, but without more
>>         information its hard to hell what the actual problem is.
>>
>>
>>     These two commits _introduced_ names in the reserved namespace.
>>
>>
>> Reserved names are reserved for any use. That can include an
>> implementation specifically blessing the introduction of a name that uses
>> double underscores.
>>
>
> That's a pretty abstract argument and the standard says nothing like that.
>
> Regardless, the implementation I'm using here doesn't have such a
> provision.
>
>
>  It may be unusual, but I don't think there is anything in the standard
>> that makes this a non-conforming extension.
>>
>
> C++ 7.1.3 Reserved identifiers
> "If the program declares or defines an identifier in a context in which it
> is reserved or defines a reserved identifier as a macro name, the behavior
> is undefined."
>
>
>
>>
>>     The checker I have is a strict warning flag I've been trying to
>>     encode the rules from the standard and in this case the diagnostic
>>     looks legitimate.
>>
>>
>> Ok, well, we disagree. ;]
>>
>> If the concern is that this is an ugly interface, I'm somewhat
>> sympathetic and we could explore better ways to expose this interface to
>> users, but I don't think this is incorrect as is, it just looks slightly
>> sub-optimal. And I don't think there is any standards problem here, as this
>> is and intentional extension provided by Clang+LSan.
>>
>> Now, one thing that I meant to ask for and didn't is to hid all of this
>> behind some macro which is used to enable LSan. If we're not compiling with
>> a compiler that supports this extension, it seems reasonable (if not likely
>> of practical importance) to avoid touching the reserved name.
>>
>
>
> Kostya's commit is the only instance I can find of this violation in
> dozens of large software projects. It's a first grader mistake (in the
> interface, not necessarily the commit), not something that should exist in
> a compiler toolchain codebase, thus my concern :-)
>
> If you really want to keep it however, let's compromise and you can put it
> behind a macro. I do however want to give a strong nudge to get this
> interface fixed. A single-underscore or no-underscore variant would do --
> nothing fancy needed.


But the choice of the prefix is deliberate -- we don't want to clash with
any user code.


>
>
> Alp.
>
>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140109/2dbfbc28/attachment.html>