r198858 - Disable LeakSanitizer in TableGen binaries, see PR18325

[Alp Toker]

On 09/01/2014 12:13, Chandler Carruth wrote:
>
> On Thu, Jan 9, 2014 at 4:07 AM, Alp Toker <alp at nuanti.com 
> <mailto: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? 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.

    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?

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/9597be88/attachment.html>