r198858 - Disable LeakSanitizer in TableGen binaries, see PR18325
alp at nuanti.com
Thu Jan 9 03:44:26 PST 2014
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?
> 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.
the browser experts
More information about the cfe-commits