r198858 - Disable LeakSanitizer in TableGen binaries, see PR18325

[Alp Toker]

On 09/01/2014 16:49, Kostya Serebryany wrote:
>
>
>
> On Thu, Jan 9, 2014 at 8:03 PM, Alp Toker <alp at nuanti.com 
> <mailto:alp at nuanti.com>> wrote:
>
>
>     On 09/01/2014 15:23, Kostya Serebryany wrote:
>
>
>         On Thu, Jan 9, 2014 at 6:57 PM, Alp Toker <alp at nuanti.com
>         <mailto:alp at nuanti.com> <mailto:alp at nuanti.com
>         <mailto:alp at nuanti.com>>> wrote:
>
>
>             On 09/01/2014 12:13, Chandler Carruth wrote:
>
>
>                     Who controls / defines the interface?
>
>
>                 The sanitizers.
>
>
>             Can you report a bug with them?
>
>
>         That's us (no need to file a separate bug unless you want to)
>
>
>     That's fantastic. A proper resolution might be in sight in that
>     case..
>
>
>
>
>             Moreover, it's blocking work on a practical level by
>         triggering
>             legitimate build issues in at least one configuration.
>         Removing
>             the function doesn't break anything and resolves the issues. I
>             don't see the problem here?
>
>
>         Will you get unblocked if we put this code under #ifdef
>         LEAK_SANITIZER ?
>         Alternatively you may probably build the code with
>         -D__lsan_is_turned_off=something_else
>
>
>     If conditionalizing turns out to be necessary, it seems best to
>     not to enable the function definition unless it's explicitly leak
>     sanitizer build.
>
>     However I'm starting to understand what you were trying to achieve
>     and have a much better proposal below..
>
>
>
>         Do you have suggestions how to improve the interface
>
>
>     We should be able to find a name or mechanisms to do this
>     properly, given that you have control over the interface. 
>
>
>
>         w/o risking to clash with user-defined names?
>
>
>     That right there appears to be the point of confusion.
>
>     The problem is that you've required users to declare a function
>     name that should have been reserved for the implementation.
>
>     I said earlier this appears to be a "thinko" in the sanitizer
>     interface. I said that because, for the majority of sanitizer
>     functions, you're dealing with the reverse problem, and that
>     appears to have leaked into the naming of these functions
>     unintentionally.
>
>     So it's not a question of avoiding a clash with user-defined
>     names, but the other way round.
>
>     In summary, the problem you need to solve is identical to the
>     naming choices when declaring an entry point in a public C
>     programming interface.
>
>     Try naming this like any C API function (which is what it is) and
>     you'll have a solution. Some good LLVM-style function name
>     conventions are "include/llvm-c/Core.h" assuming you want to go
>     with the LLVM CamelCase style.
>
>     With this approach, you have the same risk of conflict as any C
>     function, which is a well-understood problem space. The sanitizer
>     is then free to give this symbol name special handling internally
>     without imposing on others.
>
>     If you do this there's no need to conditionalize, but please
>     retain the comment explaining the purpose so it doesn't get
>     "cleaned up".
>
>     Thanks for inviting a discussion Kostya. Does that sound good to you?
>
>
> So, you propose to name this function lsan_is_turned_off (or some 
> such) instead of __lsan_is_turned_off?

include/sanitizer/lsan_interface.h-  // The user may optionally provide 
this function to disallow leak checking
include/sanitizer/lsan_interface.h-  // for the program it is linked 
into (if the return value is non-zero). This
include/sanitizer/lsan_interface.h-  // function must be defined as 
returning a constant value; any behavior beyond
include/sanitizer/lsan_interface.h-  // that is unsupported.
include/sanitizer/lsan_interface.h:  int __lsan_is_turned_off();

The flaw is above. The user can't portably provide this function because 
declaring or defining a function with the reserved name is undefined 
behaviour.

That function is very different from the rest of lsan_interface.h and 
the correct way to provide it is as a C function with an ordinary 
API-style name that the user can legally define.

> That will work, but then there is a non-zero chance that a user will 
> already have such function for whatever purposes of his own.

There is always a non-zero chance of conflict with any C function.

LLVMContextCreate() has been part of the LLVM C API for years and 
conflict hasn't been an issue, any more than it would be with a 
LSANShouldDisableSanitizer() weak function.

To put this another way, even if "__"  were hypothetically not reserved, 
your approach would still be flawed because the user might as well have 
declared a __lsan_is_turned_off() function for whatever reason.

In this case, the problem is bigger because "__" is reserved, so what 
you have now has a 100% chance of conflicting by definition.

My suggestion is that, if it's important to you to not to introduce 
unprefixed names in lsan_interface.h, just put the 
LSANShouldDisableSanitizer() declaration in a separate lsan_control.h 
header file that's include only at the point of use in a single TU.

> What's worth, he will not know about the clash at the link time -- his 
> program will start behaving wrongly.
> The __ prefix is exactly the tool that allows us to avoid a clash with 
> user-named objects.

The "__" names are reserved for the implementation.

Right now there's a function declaration sitting in the middle of 
TableGen defining a function with a reserved name which is orthogonal to 
what you want.

Alp.



> Or I've got you wrong?
>
>
>
>
>     Alp.
>
>
>
>
>
>         --kcc
>
>
>             Anyway, I'm returning to the office to deal with this and I'm
>             disappointed you've tried to push it through unilaterally with
>             disregard for anyone outside of your own circle.
>
>
>             Alp.
>
>             -- http://www.nuanti.com
>             the browser experts
>
>
>
>     -- 
>     http://www.nuanti.com
>     the browser experts
>
>

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