r198858 - Disable LeakSanitizer in TableGen binaries, see PR18325

[Alp Toker]

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.

Alp.


-- 
http://www.nuanti.com
the browser experts