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

[Sylvain Defresne via cfe-commits]

sdefresne created this revision.
sdefresne added a reviewer: rjmccall.
sdefresne added a subscriber: cfe-commits.

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

http://reviews.llvm.org/D20113

Files:
  lib/Sema/SemaType.cpp
  test/CodeGenObjCXX/arc-mangle.mm
  test/CodeGenObjCXX/mangle.mm

Index: test/CodeGenObjCXX/mangle.mm
===
--- test/CodeGenObjCXX/mangle.mm
+++ test/CodeGenObjCXX/mangle.mm
@@ -113,3 +113,10 @@
 
 // CHECK-LABEL: define void @_Z19parameterized_test3P13Parameterized
 void parameterized_test3(Parameterized *p) {}
+
+// CHECK-LABEL: define {{.*}}void @_Z1fU11ns_consumedP11objc_object
+void f(id __attribute((ns_consumed))) {}
+// 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)) {}
Index: test/CodeGenObjCXX/arc-mangle.mm
===
--- test/CodeGenObjCXX/arc-mangle.mm
+++ test/CodeGenObjCXX/arc-mangle.mm
@@ -18,6 +18,8 @@
 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 @_Z1fU11ns_consumedP11objc_object
+void f(id __attribute((ns_consumed))) {}
 // 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
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -4229,7 +4229,7 @@
 }
   }
 
-  if (LangOpts.ObjCAutoRefCount && Param->hasAttr()) {
+  if (Param->hasAttr()) {
 ExtParameterInfos[i] = ExtParameterInfos[i].withIsConsumed(true);
 HasAnyInterestingExtParameterInfos = true;
   }


Index: test/CodeGenObjCXX/mangle.mm
===
--- test/CodeGenObjCXX/mangle.mm
+++ test/CodeGenObjCXX/mangle.mm
@@ -113,3 +113,10 @@
 
 // CHECK-LABEL: define void @_Z19parameterized_test3P13Parameterized
 void parameterized_test3(Parameterized *p) {}
+
+// CHECK-LABEL: define {{.*}}void @_Z1fU11ns_consumedP11objc_object
+void f(id __attribute((ns_consumed))) {}
+// 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)) {}
Index: test/CodeGenObjCXX/arc-mangle.mm
===
--- test/CodeGenObjCXX/arc-mangle.mm
+++ test/CodeGenObjCXX/arc-mangle.mm
@@ -18,6 +18,8 @@
 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 @_Z1fU11ns_consumedP11objc_object
+void f(id __attribute((ns_consumed))) {}
 // 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
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -4229,7 +4229,7 @@
 }
   }
 
-  if (LangOpts.ObjCAutoRefCount && Param->hasAttr()) {
+  if (Param->hasAttr()) {
 ExtParameterInfos[i] = ExtParameterInfos[i].withIsConsumed(true);
 HasAnyInterestingExtParameterInfos = true;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sylvain Defresne via cfe-commits]

sdefresne added a comment.

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.


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?


http://reviews.llvm.org/D20113



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sylvain Defresne via cfe-commits]

sdefresne updated this revision to Diff 57924.
sdefresne added a comment.

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


http://reviews.llvm.org/D20113

Files:
  lib/AST/ItaniumMangle.cpp
  test/CodeGenObjCXX/arc-attrs.mm
  test/CodeGenObjCXX/arc-mangle.mm
  test/CodeGenObjCXX/mangle.mm

Index: test/CodeGenObjCXX/mangle.mm
===
--- test/CodeGenObjCXX/mangle.mm
+++ test/CodeGenObjCXX/mangle.mm
@@ -1,4 +1,4 @@
-// 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
@@ -113,3 +113,10 @@
 
 // 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)) {}
Index: test/CodeGenObjCXX/arc-mangle.mm
===
--- test/CodeGenObjCXX/arc-mangle.mm
+++ test/CodeGenObjCXX/arc-mangle.mm
@@ -1,4 +1,4 @@
-// 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 *) {}
@@ -18,10 +18,14 @@
 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 struct unsigned_c { };
 
Index: test/CodeGenObjCXX/arc-attrs.mm
===
--- test/CodeGenObjCXX/arc-attrs.mm
+++ test/CodeGenObjCXX/arc-attrs.mm
@@ -12,7 +12,7 @@
   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)
@@ -31,16 +31,16 @@
 // 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();
 
-  // 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());
 
   // 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)
Index: lib/AST/ItaniumMangle.cpp
===
--- lib/AST/ItaniumMangle.cpp
+++ lib/AST/ItaniumMangle.cpp
@@ -2243,7 +2243,7 @@
 FunctionTypeDepth.enterResultType();
 
 // Mangle ns_returns_retained as an order-sensitive qualifier here.
-if (Proto->getExtInfo().getProducesResult())
+if (Proto->getExtInfo().get

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

[Sylvain Defresne via cfe-commits]

sdefresne added a comment.

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,


http://reviews.llvm.org/D20113



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits