Fix mangled name of method with ns_consumed parameters.

[sdefresne]

Summary

When a function/method use a parameter with "ns_consumed" attribute,
ensure that the mangled name is the same whether -fobjc-arc is used
or not.

Since "ns_consumed" attribute is generally used to inform ARC that
a function/method does sink the reference, it mean it is usually
implemented in a compilation unit compiled without -fobjc-arc but
used form a compilation unit compiled with it.

Originally found while trying to use "ns_consumed" attribute in an
Objective-C++ file in Chromium (http://crbug.com/599980) where it
caused a linker error.

Regression introduced by revision 262278 (previously the attribute
was incorrectly not part of the mangled name).

[rjmccall]

This is a good catch, thanks!

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.

[sdefresne]

In 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.

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?

If so, then there is nothing to do, as "ns_consumed" attibute is ignored if applied to a non-parameter type as far as I can tell (see below for compilation warning clang already emits regarding ignored attributes). So, my understanding is that the ns_consumed attribute is only used if applied to a parameter, and thus only when it is relevant to include it in the mangled name.

Regarding ns_returns_retained, it is ignored if not applied to the function itself.

$ clang++ -fobjc-arc -o x.o -c x.mm
x.mm:1:19: warning: 'ns_consumed' attribute only applies to parameters
      [-Wignored-attributes]
id __attribute__((ns_consumed)) f() { return 0; }
                  ^
x.mm:2:23: warning: 'ns_consumed' attribute only applies to parameters
      [-Wignored-attributes]
id g() __attribute__((ns_consumed)) { return 0; }
                      ^
x.mm:7:26: warning: 'ns_returns_retained' only applies to function types; type
      here is 'id' [-Wignored-attributes]
void k(id __attribute__((ns_returns_retained))) {}

So, could you elaborate a bit more on what additional changes I need to make to my patch?

[rjmccall]

In D20113#426884, @sdefresne wrote:

In 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.

[sdefresne]

Ok, this make sense. I've updated my change to follow your recommendation. Can you take another look?

Using 'extern "C" { ... }" would probably not be an option in my case as I want to use "ns_consumed" for the parameter of a templated class (i.e. this probably won't work if using "C" mangling, and I'm not even sure it would compile).

[rjmccall]

That looks great, thank you.

[sdefresne]

Thank you for the comment.

I think my change still needs to be reviewed and approved by someone (at least in the Phabricator interface it still appears as "Need review"). Can you do the review and give approval if it looks good to you?

Once this is approved, should I just follow instructions at http://llvm.org/docs/Phabricator.html#subversion-and-arcanist? Do I need to request any kind of access rights?

Sorry for the questions, as I said, this is my first change against clang/llvm. Cheers,

[thakis]

We use phabricator not very dogmatically. If John says this looks good, then this looks good, even if phab didn't get the message :-)

You do need a commit bit to land things. I've landed this change for you in r270702 (http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160523/159820.html). http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access documents how to obtain commit access.

lib/AST/ItaniumMangle.cpp

void CXXNameMangler::mangleBareFunctionType(const FunctionProtoType *Proto,
   // for details on what we're trying to achieve here.
   FunctionTypeDepthState saved = FunctionTypeDepth.push();
 
   // <bare-function-type> ::= <signature type>+
   if (MangleReturnType) {
     FunctionTypeDepth.enterResultType();
 
     // Mangle ns_returns_retained as an order-sensitive qualifier here.
-    if (Proto->getExtInfo().getProducesResult())
+    if (Proto->getExtInfo().getProducesResult() && FD == nullptr)
       mangleVendorQualifier("ns_returns_retained");
 
     // Mangle the return type without any direct ARC ownership qualifiers.
     QualType ReturnTy = Proto->getReturnType();
     if (ReturnTy.getObjCLifetime()) {
       auto SplitReturnTy = ReturnTy.split();
       SplitReturnTy.Quals.removeObjCLifetime();
       ReturnTy = getASTContext().getQualifiedType(SplitReturnTy);
if (Proto->getNumParams() == 0 && !Proto->isVariadic()) {
 
     FunctionTypeDepth.pop(saved);
     return;
   }
 
   assert(!FD || FD->getNumParams() == Proto->getNumParams());
   for (unsigned I = 0, E = Proto->getNumParams(); I != E; ++I) {
     // Mangle extended parameter info as order-sensitive qualifiers here.
-    if (Proto->hasExtParameterInfos()) {
+    if (Proto->hasExtParameterInfos() && FD == nullptr) {
       mangleExtParameterInfo(Proto->getExtParameterInfo(I));
     }
 
     // Mangle the type.
     QualType ParamTy = Proto->getParamType(I);
     mangleType(Context.getASTContext().getSignatureParameterType(ParamTy));
 
     if (FD) {
case TemplateArgument::Declaration: {
     bool compensateMangling = !A.getParamTypeForDecl()->isReferenceType();
     if (compensateMangling) {
       Out << 'X';
       mangleOperatorName(OO_Amp, 1);
     }
 
     Out << 'L';
     // References to external entities use the mangled name; if the name would
-    // not normally be manged then mangle it as unqualified.
+    // not normally be mangled then mangle it as unqualified.
     mangle(D);
     Out << 'E';
 
     if (compensateMangling)
       Out << 'E';
 
     break;
   }

test/CodeGenObjCXX/arc-attrs.mm

 // RUN: %clang_cc1 -triple x86_64-apple-darwin11 -emit-llvm -fobjc-arc -o - %s | FileCheck %s
 
 id makeObject1() __attribute__((ns_returns_retained));
 id makeObject2() __attribute__((ns_returns_retained));
 void releaseObject(__attribute__((ns_consumed)) id);
 
 // CHECK-LABEL: define void @_Z10sanityTestv
 void sanityTest() {
   // CHECK: [[X:%.*]] = alloca i8*, align 8
   // CHECK-NEXT: [[OBJ1:%.*]] = call i8* @_Z11makeObject1v()
   // CHECK-NEXT: store i8* [[OBJ1]], i8** [[X]], align 8
   id x = makeObject1();
 
   // CHECK-NEXT: [[OBJ2:%.*]] = call i8* @_Z11makeObject2v()
-  // CHECK-NEXT: call void @_Z13releaseObjectU11ns_consumedP11objc_object(i8* [[OBJ2]])
+  // CHECK-NEXT: call void @_Z13releaseObjectP11objc_object(i8* [[OBJ2]])
   releaseObject(makeObject2());
 
   // CHECK-NEXT: call void @objc_storeStrong(i8** [[X]], i8* null)
   // CHECK-NEXT: ret void
 }
 
 
 template <typename T>
 T makeObjectT1() __attribute__((ns_returns_retained));
 template <typename T>
 T makeObjectT2() __attribute__((ns_returns_retained));
 
 template <typename T>
 void releaseObjectT(__attribute__((ns_consumed)) T);  
 
 // CHECK-LABEL: define void @_Z12templateTestv
 void templateTest() {
   // CHECK: [[X:%.*]] = alloca i8*, align 8
-  // CHECK-NEXT: [[OBJ1:%.*]] = call i8* @_Z12makeObjectT1IU8__strongP11objc_objectEU19ns_returns_retainedT_v()
+  // CHECK-NEXT: [[OBJ1:%.*]] = call i8* @_Z12makeObjectT1IU8__strongP11objc_objectET_v()
   // CHECK-NEXT: store i8* [[OBJ1]], i8** [[X]], align 8
   id x = makeObjectT1<id>();
 
-  // CHECK-NEXT: [[OBJ2:%.*]] = call i8* @_Z12makeObjectT2IU8__strongP11objc_objectEU19ns_returns_retainedT_v()
-  // CHECK-NEXT: call void @_Z13releaseObjectU11ns_consumedP11objc_object(i8* [[OBJ2]])
+  // CHECK-NEXT: [[OBJ2:%.*]] = call i8* @_Z12makeObjectT2IU8__strongP11objc_objectET_v()
+  // CHECK-NEXT: call void @_Z13releaseObjectP11objc_object(i8* [[OBJ2]])
   releaseObject(makeObjectT2<id>());
 
   // CHECK-NEXT: [[OBJ3:%.*]] = call i8* @_Z11makeObject1v()
-  // CHECK-NEXT: call void @_Z14releaseObjectTIU8__strongP11objc_objectEvU11ns_consumedT_(i8* [[OBJ3]])
+  // CHECK-NEXT: call void @_Z14releaseObjectTIU8__strongP11objc_objectEvT_(i8* [[OBJ3]])
   releaseObjectT(makeObject1());
 
   // CHECK-NEXT: call void @objc_storeStrong(i8** [[X]], i8* null)
   // CHECK-NEXT: ret void
 }

test/CodeGenObjCXX/arc-mangle.mm

-// RUN: %clang_cc1 -fobjc-arc -fobjc-runtime-has-weak -triple %itanium_abi_triple -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fobjc-arc -fobjc-runtime-has-weak -triple %itanium_abi_triple -emit-llvm -fblocks -o - %s | FileCheck %s
 
 // CHECK-LABEL: define {{.*}}void @_Z1fPU8__strongP11objc_object(i8**)
 void f(__strong id *) {}
 // CHECK-LABEL: define {{.*}}void @_Z1fPU6__weakP11objc_object(i8**)
 void f(__weak id *) {}
 // CHECK-LABEL: define {{.*}}void @_Z1fPU15__autoreleasingP11objc_object(i8**)
 void f(__autoreleasing id *) {}
 // CHECK-LABEL: define {{.*}}void @_Z1fPP11objc_object(i8**)
 void f(__unsafe_unretained id *) {}
 // CHECK-LABEL: define {{.*}}void @_Z1fPU8__strongKP11objc_object(i8**)
 void f(const __strong id *) {}
 // CHECK-LABEL: define {{.*}}void @_Z1fPU6__weakKP11objc_object(i8**)
 void f(const __weak id *) {}
 // CHECK-LABEL: define {{.*}}void @_Z1fPU15__autoreleasingKP11objc_object(i8**)
 void f(const __autoreleasing id *) {}
 // CHECK-LABEL: define {{.*}}void @_Z1fPKP11objc_object(i8**)
 void f(const __unsafe_unretained id *) {}
 // CHECK-LABEL: define {{.*}}void @_Z1fPFU19ns_returns_retainedP11objc_objectvE
 void f(__attribute__((ns_returns_retained)) id (*fn)()) {}
+// CHECK-LABEL: define {{.*}}void @_Z1fP11objc_object
+void f(__attribute__((ns_consumed)) id) {}
 // CHECK-LABEL: define {{.*}}void @_Z1fPFP11objc_objectU11ns_consumedS0_S0_E
 void f(id (*fn)(__attribute__((ns_consumed)) id, id)) {}
 // CHECK-LABEL: define {{.*}}void @_Z1fPFP11objc_objectS0_U11ns_consumedS0_E
 void f(__strong id (*fn)(id, __attribute__((ns_consumed)) id)) {}
+// CHECK-LABEL: define {{.*}}void @_Z1fU13block_pointerFvU11ns_consumedP11objc_objectE
+void f(void (^)(__attribute__((ns_consumed)) id)) {}
 
 template<unsigned N> struct unsigned_c { };
 
 // CHECK-LABEL: define weak_odr {{.*}}void @_Z1gIKvEvP10unsigned_cIXplszv1U8__bridgecvPT_v1U8__bridgecvP11objc_objectcvS3_Li0ELi1EEE
 template<typename T>void g(unsigned_c<sizeof((__bridge T*)(__bridge id)(T*)0) + 1>*) {}
 template void g<const void>(unsigned_c<sizeof(id) + 1> *);

test/CodeGenObjCXX/mangle.mm

-// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -std=c++11 -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -std=c++11 -emit-llvm -fblocks -o - | FileCheck %s
 
 // CHECK: @"_ZZ11+[A shared]E1a" = internal global
 // CHECK: @"_ZZ11-[A(Foo) f]E1a" = internal global
 // CHECK: v56@0:8i16i20i24i28i32i36i40i44^i48
 
 @interface A
 @end
 

 // CHECK-LABEL: define void @_Z19parameterized_test1P13ParameterizedIP1AP4TestE
 void parameterized_test1(Parameterized<A *, Test *> *p) {}
 
 // CHECK-LABEL: define void @_Z19parameterized_test2PU8__kindof13ParameterizedIP1AP4TestE
 void parameterized_test2(__kindof Parameterized<A *, Test *> *p) {}
 
 // CHECK-LABEL: define void @_Z19parameterized_test3P13Parameterized
 void parameterized_test3(Parameterized *p) {}
+
+// CHECK-LABEL: define {{.*}}void @_Z1fP11objc_object
+void f(__attribute__((ns_consumed)) id) {}
+// CHECK-LABEL: define {{.*}}void @_Z1fPFP11objc_objectS0_S0_E
+void f(id (*fn)(__attribute__((ns_consumed)) id, id)) {}
+// CHECK-LABEL: define {{.*}}void @_Z1fU13block_pointerFvP11objc_objectE
+void f(void (^)(__attribute__((ns_consumed)) id)) {}