r198858 - Disable LeakSanitizer in TableGen binaries, see PR18325

[Kostya Serebryany]

On Thu, Jan 9, 2014 at 6:57 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 09/01/2014 12:13, Chandler Carruth wrote:
>
>
> On Thu, Jan 9, 2014 at 4:07 AM, Alp Toker <alp at nuanti.com> wrote:
>
>>  I understand that, but the question is -- do you think that usage
>>> pattern will be prevalent? Is it worth putting an external definition in
>>> the binary *always* just to catch the case where we do a link-time-only
>>> flag?
>>>
>>> Put another way, is it really too burdensome to say that LSan does
>>> require a compile time flag in order to support some usage patterns?
>>>
>>
>> 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)


> They need to provide hooks that can be implemented cleanly.
>
> It's not reasonable to force something like this on the entire community
> just because your internal schedule demands it.
>
> There are plenty of other checkers and analyzers that work fine without
> forcing problematic code, and that can be disabled without impacting
> standard builds, so it's not a big problem for the community if this takes
> a few days to get resolved out of tree.
>
> To be clear, the commits not only introduce dead code, but code that
> actively causes build issues in strict setups because it's not compliant
> with the C++ specification and steps on the reserved namespace.
>
> Furthermore, the references I could find to this group of sanitizer
> functions doesn't inspire confidence that they were thought through as
> thoroughly as you suggest:
>
> kcc commented on this revision.
> I am actually *not a big fan of this patch*.
> We know *only one use case* for it, and I really hope we can find a
> cleaner solution.
>
> I remember my concerns.
Now we know several use cases and we did not find a cleaner solution yet.

>
> samsonov requested changes to this revision.
> I agree with Kostya that *we should avoid adding this* interface function
> if possible.
>
>
>
>
>>
>> This looks like a spectacular thinko rather than being something
>> intentional -- is it possible to fix it to drop one or more underscores
>> instead of devising workarounds?
>>
>
>  I have no idea what you mean. This was a well considered change, and
> part of a design that has been developed over quite some time on the lists.
> None of these are workarounds?
>
>
>>
>> There's never a valid reason to require user code to define reserved
>> names (although it's sometimes OK for user code to _use_ reserved names).
>>
>
>  You keep making this assertion, but I still don't find any backing for
> it in the standard. I'm happy to check with some of my fellow members of
> the committee, but I would be surprised if they drew the distinction you
> are drawing here.
>
>
> I'm not making an assertion. The standard is very clear on this:
>
>
> *17.6.4.3 Reserved names [reserved.names]*
>
> If a program declares or defines a name in a context where it is reserved,
> other than as explicitly allowed by this Clause, its *behavior is
> undefined*.
>
> *17.6.4.3.2 Global names [global.names]*
>
> Certain sets of names and function signatures are always reserved to the
> implementation:
>
>
>    - *Each name that contains a double underscore __* or begins with an
>    underscore followed by an uppercase letter (2.12) *is reserved to the
>    implementation for any use*.
>    - Each name that begins with an underscore is reserved to the
>    implementation for use as a name in the global namespace.
>
>
>
>
>
>>
>> I'm heading out for a couple of hours, please gate this behind a macro or
>> revert until a resolution is found.
>
>
> Alp, I really don't think this is reasonable. This change, and the lead up
> to the change, were discussed on the list with code review. I understand
> you have some high level design concerns, but there doesn't appear to be
> broad agreement with them and I don't think it is reasonable to block
> progress here while we sort them out.
>
>  I'm happy to continue this discussion, but I do not think that this
> needs to be reverted, and I do not think we need to block progress
>
>
> I've just discussed with colleagues as well as a clang frontend developer
> (all of whom are pretty keyed in on the matter) and there's broad agreement
> that these changes are not wanted in their current form.
>
> 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

Do you have suggestions how to improve the interface w/o risking to clash
with user-defined names?

--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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140109/8106c233/attachment.html>