r198858 - Disable LeakSanitizer in TableGen binaries, see PR18325

[Kostya Serebryany]

On Thu, Jan 9, 2014 at 8:03 PM, Alp Toker <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>> 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?
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.
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.
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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140109/3a5262eb/attachment.html>