On Thu, Jan 9, 2014 at 3:36 PM, Chandler Carruth <[email protected]>wrote:
> > On Thu, Jan 9, 2014 at 3:15 AM, Alp Toker <[email protected]> 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. It may be unusual, but I don't think there is anything > in the standard that makes this a non-conforming extension. > > >> 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. > We currently don't have any macro that is used to enable lsan in llvm bootstrap. We can easily add one, no problem, and then put this code under #ifdef LEAK_SANITIZER It would make things a bit more ugly. We have tow problems to solve wrt lsan and bootstrap: 1. Make asan+lsan bootstrap clean. This is what I need to solve now to enable leak checking for the entire llvm. For this we may also use #if __has_feature(address_sanitizer). 2. Make any+lsan bootstrap clean ("any" is one of asan, tsan, msan, <none>). This may introduce more complexity and that is why the current solution is nice. I don't have to solve this general problem though for the asan+lsan bootstrap bot. --kcc
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
