[PATCH] D20113: Fix mangled name of method with ns_consumed parameters.

[John McCall via cfe-commits]

rjmccall added a comment.

In http://reviews.llvm.org/D20113#426884, @sdefresne wrote:

> In http://reviews.llvm.org/D20113#425984, @rjmccall wrote:
>
> > This is a good catch, thanks!
>
>
> Thank you for the quick reply.
>
> Please excuse me if I misunderstood you or if my remark appear off the mark, this is my first time sending patches to clang. I'm not yet completely familiar with all clang concepts :-)
>
> > As a slight adjustment, It's probably better to just ignore this attribute when mangling the function type of an entity, the same way that we generally don't mangle return types because they don't affect overloading.  That will require an extra flag to be passed down in a few places, but that's pretty reasonable.  This will generally allow NS_CONSUMED and NS_RETURNS_RETAINED to be placed on existing APIs without changing their mangling unless the attribute is used in a secondary position (such as the type of an argument).
>
> > 
>
> > Finally, you should give ns_returns_retained the same treatment, as well as the parameter-ABI attributes.
>


Actually, I'm sorry, I completely failed to read your patch correctly.  This is not the appropriate way to fix this problem; we do not want NS_CONSUMED to become semantically meaningful in non-ARC modes.

> I'm not really sure what you mean by "ignore this attribute when mangling the function type of an entity". I read this as "we should ensure that the ns_consumed attribute is only mangled if it is applied to a parameter". Is this correct?


No.  There are two reasons we mangle function types.  The first is when the type appears somewhere "secondary" in the signature of the entity we're mangling, e.g. in a template argument or as the pointee type of a pointer or reference; it's important that this include all the information that distinguishes two canonically-different function types, including things like NS_CONSUMED under ARC.  The second is when we're mangling the declared type of a specific function entity.  It's only important that this include all the information that can distinguish overloads, and since you can't overload two functions based on whether one of them takes an NS_CONSUMED argument, this doesn't need to include those attributes.  This has multiple benefits: first, it makes shorter symbols and thus saves binary size, and second, it allows the same declaration to mangle the same in both ARC and non-ARC modes as long as it doesn't feature NS_CONSUMED in a secondary position.

So I think the appropriate solution is to change the mangler to ignore things like calling conventions, NS_CONSUMED, NS_RETURNS_RETAINED, etc. when mangling the direct function type of a function entity.  This will allow the same declaration to mangle the same way in ARC and non-ARC modes as long as NS_CONSUMED etc. are only used in the declaration of the function itself and not in, say, the types of its parameters.

That is, this declaration would mangle the same in both language modes:

  void foo(NS_CONSUMED id x);

But this declaration would not:

  void foo(void (^block)(NS_CONSUMED id x));

I think this is the best we can do, because we really don't want these attributes to have any semantic impact in non-ARC language modes.  If this isn't good enough for your use case, you will need to declare the function extern "C".

So, here's what you need to do.  First, don't change SemaType.cpp.  Instead, you should change mangleBareFunctionType in ItaniumMangle.cpp so that it does not mangle ns_returns_retained or anything from ExtParameterInfo when FD is non-null.

Thanks, and sorry for the confusion.


http://reviews.llvm.org/D20113