[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

In D86049#3821183 , @plotfi wrote:

> In D86049#3819109 , @mwyman wrote:
>
>> In D86049#3818981 , @plotfi wrote:
>>
>>> @ahatanak I can revive some of what I was working on from 
>>> https://reviews.llvm.org/D86049?id=285923 if we think we need a thunk for 
>>> the checks as @rjmccall mentioned.
>>
>> I believe the generated direct methods already handle the null checks and 
>> class init in `CGObjCCommonMac::GenerateDirectMethodPrologue`, meaning the 
>> thunks aren't strictly necessary for the callee to handle them.
>>
>> Could the thunks instead allow us to have publicly-visible mangled names 
>> (something akin to the new selector stubs `_objc_msgSend$selectorName` but 
>> for `_objc_direct$Class_selectorName`) while leaving the actual impl name 
>> alone, letting the stack traces see normal ObjC symbol names?
>
> I think the square brackets are still problematic for linking, so is LLVM's 
> handling of \01 (I believe).

@mwyman @ahatanak The mangling code change is to appease ld64 by the way:

https://github.com/apple-oss-distributions/ld64/blob/ld64-609/src/ld/Options.cpp#L1378-L1408

The wildCardMatch function does some symbol stripping off of '?' and '['. I 
alter ']' just for stylistic consistency however. And the dropping of prefix 
byte is so that you get the preceding underbar that all visible Darwin symbols 
seem to need to have (but I am not as certain on that one).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

In D86049#3819109 , @mwyman wrote:

> In D86049#3818981 , @plotfi wrote:
>
>> @ahatanak I can revive some of what I was working on from 
>> https://reviews.llvm.org/D86049?id=285923 if we think we need a thunk for 
>> the checks as @rjmccall mentioned.
>
> I believe the generated direct methods already handle the null checks and 
> class init in `CGObjCCommonMac::GenerateDirectMethodPrologue`, meaning the 
> thunks aren't strictly necessary for the callee to handle them.

I don't know exactly what the thunk is supposed to do, but it sounds like the 
thunk is going to do the initialization by calling `objc_opt_self` and then 
call the direct class method. So the initialization isn't done inside the 
direct method.

If the thunk is inlined, the call to `objc_opt_self` can be optimized away if 
it we can prove `self` is already initialized. But we'd have to teach llvm to 
remove the call to `objc_opt_self` or add a new optimization pass to do so (see 
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGObjCMac.cpp#L4057).

John, is that what you had in mind?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

In D86049#3819109 , @mwyman wrote:

> In D86049#3818981 , @plotfi wrote:
>
>> @ahatanak I can revive some of what I was working on from 
>> https://reviews.llvm.org/D86049?id=285923 if we think we need a thunk for 
>> the checks as @rjmccall mentioned.
>
> I believe the generated direct methods already handle the null checks and 
> class init in `CGObjCCommonMac::GenerateDirectMethodPrologue`, meaning the 
> thunks aren't strictly necessary for the callee to handle them.
>
> Could the thunks instead allow us to have publicly-visible mangled names 
> (something akin to the new selector stubs `_objc_msgSend$selectorName` but 
> for `_objc_direct$Class_selectorName`) while leaving the actual impl name 
> alone, letting the stack traces see normal ObjC symbol names?

I think the square brackets are still problematic for lining, so is LLVM's 
handling of \01 (I believe).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Michael Wyman via Phabricator via cfe-commits]

mwyman added a comment.

In D86049#3818981 , @plotfi wrote:

> @ahatanak I can revive some of what I was working on from 
> https://reviews.llvm.org/D86049?id=285923 if we think we need a thunk for the 
> checks as @rjmccall mentioned.

I believe the generated direct methods already handle the null checks and class 
init in `CGObjCCommonMac::GenerateDirectMethodPrologue`, meaning the thunks 
aren't strictly necessary for the callee to handle them.

Could the thunks instead allow us to have publicly-visible mangled names 
(something akin to the new selector stubs `_objc_msgSend$selectorName` but for 
`_objc_direct$Class_selectorName`) while leaving the actual impl name alone, 
letting the stack traces see normal ObjC symbol names?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

@ahatanak I can revive some of what I was working on from 
https://reviews.llvm.org/D86049?id=285923 if we think we need a thunk for the 
checks as @rjmccall mentioned.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a subscriber: kyulee1.
plotfi added a comment.

In D86049#3818923 , @ahatanak wrote:

> In D86049#3818696 , @plotfi wrote:
>
>> In D86049#3818435 , @mwyman wrote:
>>
>>> In D86049#3816006 , @plotfi wrote:
>>>
 @dmaclach @ahatanak @mwyman How do things look from here? Do you want 
 something for properties as well or would it be ok if we did this in a 
 later commit?
>>>
>>> Huh, for some reason I thought when I'd last poked at using the 
>>> `visibility` attribute it wasn't allowed on ObjC methods, which is why I'd 
>>> suggested adding the enum on `objc_direct`, but as that no longer appears 
>>> to be the case yes I like this approach better.
>>
>> @dmaclach @mwyman  I am also very happy with the fact that we can just reuse 
>> the regular `visibility` attribute. In the future we can decide on revised 
>> behavior for `hasMethodVisibilityDefault`.
>>
>> @ahatanak Do you have feedback on this?
>
> The visibility attribute changes look good to me.
>
> Do we have the answers to the last two questions John raised in 
> https://reviews.llvm.org/D86049#2255738? I think we should get it right the 
> first time since, once we make the direct methods visible, it'd be hard to 
> change where the null check or class initialization is done since that would 
> be an ABI change. Should we run experiments to measure the impact on code 
> size?

I think it may be required to do thunk generation after all to do the null and 
init checks at the callee. What are your thoughts on this @ahatanak ?




Comment at: clang/lib/AST/Mangle.cpp:371
+  OS << (MD->isInstanceMethod() ? '-' : '+');
+  OS << (MD->hasMethodVisibilityDefault() ? '<' : '[');
   if (const auto *CID = MD->getCategory()) {

plotfi wrote:
> ahatanak wrote:
> > Sorry, I might have missed the discussion, but what's the reason we need 
> > this change in mangling? Is it because the linker cannot handle the 
> > standard mangling scheme?
> Yeah. These are for ld and dyld. Not having a preceding underscore and the 
> square brackets causes problems. 
I need to ask @kyulee / @kyulee1 about the details. We went with these mangling 
changes some time ago and it is not fresh in my mind. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added inline comments.



Comment at: clang/lib/AST/Mangle.cpp:371
+  OS << (MD->isInstanceMethod() ? '-' : '+');
+  OS << (MD->hasMethodVisibilityDefault() ? '<' : '[');
   if (const auto *CID = MD->getCategory()) {

ahatanak wrote:
> Sorry, I might have missed the discussion, but what's the reason we need this 
> change in mangling? Is it because the linker cannot handle the standard 
> mangling scheme?
Yeah. These are for ld and dyld. Not having a preceding underscore and the 
square brackets causes problems. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

In D86049#3818696 , @plotfi wrote:

> In D86049#3818435 , @mwyman wrote:
>
>> In D86049#3816006 , @plotfi wrote:
>>
>>> @dmaclach @ahatanak @mwyman How do things look from here? Do you want 
>>> something for properties as well or would it be ok if we did this in a 
>>> later commit?
>>
>> Huh, for some reason I thought when I'd last poked at using the `visibility` 
>> attribute it wasn't allowed on ObjC methods, which is why I'd suggested 
>> adding the enum on `objc_direct`, but as that no longer appears to be the 
>> case yes I like this approach better.
>
> @dmaclach @mwyman  I am also very happy with the fact that we can just reuse 
> the regular `visibility` attribute. In the future we can decide on revised 
> behavior for `hasMethodVisibilityDefault`.
>
> @ahatanak Do you have feedback on this?

The visibility attribute changes look good to me.

Do we have the answers to the last two questions John raised in 
https://reviews.llvm.org/D86049#2255738? I think we should get it right the 
first time since, once we make the direct methods visible, it'd be hard to 
change where the null check or class initialization is done since that would be 
an ABI change. Should we run experiments to measure the impact on code size?




Comment at: clang/lib/AST/Mangle.cpp:371
+  OS << (MD->isInstanceMethod() ? '-' : '+');
+  OS << (MD->hasMethodVisibilityDefault() ? '<' : '[');
   if (const auto *CID = MD->getCategory()) {

Sorry, I might have missed the discussion, but what's the reason we need this 
change in mangling? Is it because the linker cannot handle the standard 
mangling scheme?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Michael Wyman via Phabricator via cfe-commits]

mwyman added a comment.

I think it's reasonable to do `@property` behavior in a follow-up.

Are we thinking to allow `__attribute__((visibility("default")))` on an 
`@property` declaration, to keep the visibility behavior consistent?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

In D86049#3818435 , @mwyman wrote:

> In D86049#3816006 , @plotfi wrote:
>
>> @dmaclach @ahatanak @mwyman How do things look from here? Do you want 
>> something for properties as well or would it be ok if we did this in a later 
>> commit?
>
> Huh, for some reason I thought when I'd last poked at using the `visibility` 
> attribute it wasn't allowed on ObjC methods, which is why I'd suggested 
> adding the enum on `objc_direct`, but as that no longer appears to be the 
> case yes I like this approach better.

@dmaclach @mwyman  I am also very happy with the fact that we can just reuse 
the regular `visibility` attribute. In the future we can decide on revised 
behavior for `hasMethodVisibilityDefault`.

@ahatanak Do you have feedback on this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Michael Wyman via Phabricator via cfe-commits]

mwyman added a comment.

In D86049#3816006 , @plotfi wrote:

> @dmaclach @ahatanak @mwyman How do things look from here? Do you want 
> something for properties as well or would it be ok if we did this in a later 
> commit?

Huh, for some reason I thought when I'd last poked at using the `visibility` 
attribute it wasn't allowed on ObjC methods, which is why I'd suggested adding 
the enum on `objc_direct`, but as that no longer appears to be the case yes I 
like this approach better.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi updated this revision to Diff 463064.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

Files:
  clang/include/clang/AST/DeclObjC.h
  clang/lib/AST/Mangle.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCRuntime.cpp
  clang/test/CodeGenObjC/objc-direct-wrapper.m

Index: clang/test/CodeGenObjC/objc-direct-wrapper.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/objc-direct-wrapper.m
@@ -0,0 +1,86 @@
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s | llvm-nm - | \
+// RUN: FileCheck -check-prefix=CHECK-DEFAULT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEF %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE %s
+
+// RUN: not %clang -fobjc-arc -Wno-objc-root-class -DENABLE_PROTOCOL_DIRECT_FAIL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm 2>&1 | \
+// RUN: FileCheck -check-prefix=CHECK-PROTOCOL-DIRECT-FAIL %s
+
+
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=0 \
+// RUN: -target x86_64-apple-macosx10.15.0 -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER-INDIRECT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=0 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEF-INDIRECT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=0 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE-INDIRECT %s
+
+// CHECK-WRAPPER: T _-
+ // TODO: Fix this
+// CHECK-DEFAULT: t -[C testMethod:bar:]
+// CHECK-WRAPPER-IR-DEF: define {{(dso_local )?}}void @"-"
+// CHECK-WRAPPER-IR-DECLARE: declare {{(dso_local )?}}void @"-"
+// CHECK-PROTOCOL-DIRECT-FAIL: error: 'objc_direct' attribute cannot be applied to methods declared in an Objective-C protocol
+
+// CHECK-WRAPPER-INDIRECT-NOT: T _-
+// CHECK-WRAPPER-IR-DEF-INDIRECT-NOT: define {{(dso_local )?}}void @"-"
+// CHECK-WRAPPER-IR-DECLARE-INDIRECT-NOT: declare {{(dso_local )?}}void @"-"
+
+#if ENABLE_VISIBLE_OBJC_DIRECT
+#define OBJC_DIRECT __attribute__((objc_direct)) __attribute__((visibility("default")))
+#else
+#define OBJC_DIRECT
+#endif
+
+@interface C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT;
+@end
+
+#ifndef NO_OBJC_IMPL
+@implementation C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT {
+}
+@end
+#endif
+
+#ifdef ENABLE_PROTOCOL_DIRECT_FAIL
+@protocol ProtoDirectVisibleFail
+- (void)protoMethod OBJC_DIRECT;  // expected-error {{'objc_direct' attribute cannot be applied to methods declared in an Objective-C protocol}}
+@end
+#endif
+
+C *c;
+
+void f() {
+  [c testMethod:1 bar:1.0];
+}
Index: clang/lib/CodeGen/CGObjCRuntime.cpp
===
--- clang/lib/CodeGen/CGObjCRuntime.cpp
+++ clang/lib/CodeGen/CGObjCRuntime.cpp
@@ -474,7 +474,8 @@
   std::string buffer;
   llvm::raw_string_ostream out(buffer);
   CGM.getCXXABI().getMangleContext().mangleObjCMethodName(OMD, out,
-   /*includePrefixByte=*/true,
+   /*includePrefixByte=*/
+   !OMD->hasMethodVisibilityDefault(),
includeCategoryName);
   return buffer;
 }
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -760,7 +760,9 @@
 
   const CGFunctionInfo  = CGM.getTypes().arrangeObjCMethodDeclaration(OMD);
   if (OMD->isDirectMethod()) {
-

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

@dmaclach @ahatanak @mwyman How do things look from here? Do you want something 
for properties as well or would it be ok if we did this in a later commit?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi updated this revision to Diff 462999.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

Files:
  clang/include/clang/AST/DeclObjC.h
  clang/lib/AST/DeclObjC.cpp
  clang/lib/AST/Mangle.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCRuntime.cpp
  clang/test/CodeGenObjC/objc-direct-wrapper.m

Index: clang/test/CodeGenObjC/objc-direct-wrapper.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/objc-direct-wrapper.m
@@ -0,0 +1,86 @@
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s | llvm-nm - | \
+// RUN: FileCheck -check-prefix=CHECK-DEFAULT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEF %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE %s
+
+// RUN: not %clang -fobjc-arc -Wno-objc-root-class -DENABLE_PROTOCOL_DIRECT_FAIL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm 2>&1 | \
+// RUN: FileCheck -check-prefix=CHECK-PROTOCOL-DIRECT-FAIL %s
+
+
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=0 \
+// RUN: -target x86_64-apple-macosx10.15.0 -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER-INDIRECT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=0 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEF-INDIRECT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=0 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE-INDIRECT %s
+
+// CHECK-WRAPPER: T _-
+ // TODO: Fix this
+// CHECK-DEFAULT: t -[C testMethod:bar:]
+// CHECK-WRAPPER-IR-DEF: define {{(dso_local )?}}void @"-"
+// CHECK-WRAPPER-IR-DECLARE: declare {{(dso_local )?}}void @"-"
+// CHECK-PROTOCOL-DIRECT-FAIL: error: 'objc_direct' attribute cannot be applied to methods declared in an Objective-C protocol
+
+// CHECK-WRAPPER-INDIRECT-NOT: T _-
+// CHECK-WRAPPER-IR-DEF-INDIRECT-NOT: define {{(dso_local )?}}void @"-"
+// CHECK-WRAPPER-IR-DECLARE-INDIRECT-NOT: declare {{(dso_local )?}}void @"-"
+
+#if ENABLE_VISIBLE_OBJC_DIRECT
+#define OBJC_DIRECT __attribute__((objc_direct)) __attribute__((visibility("default")))
+#else
+#define OBJC_DIRECT
+#endif
+
+@interface C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT;
+@end
+
+#ifndef NO_OBJC_IMPL
+@implementation C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT {
+}
+@end
+#endif
+
+#ifdef ENABLE_PROTOCOL_DIRECT_FAIL
+@protocol ProtoDirectVisibleFail
+- (void)protoMethod OBJC_DIRECT;  // expected-error {{'objc_direct' attribute cannot be applied to methods declared in an Objective-C protocol}}
+@end
+#endif
+
+C *c;
+
+void f() {
+  [c testMethod:1 bar:1.0];
+}
Index: clang/lib/CodeGen/CGObjCRuntime.cpp
===
--- clang/lib/CodeGen/CGObjCRuntime.cpp
+++ clang/lib/CodeGen/CGObjCRuntime.cpp
@@ -474,7 +474,8 @@
   std::string buffer;
   llvm::raw_string_ostream out(buffer);
   CGM.getCXXABI().getMangleContext().mangleObjCMethodName(OMD, out,
-   /*includePrefixByte=*/true,
+   /*includePrefixByte=*/
+   !OMD->isVisibleDirectMethod(),
includeCategoryName);
   return buffer;
 }
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -760,7 +760,9 @@
 
   const CGFunctionInfo  = CGM.getTypes().arrangeObjCMethodDeclaration(OMD);
   if 

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi updated this revision to Diff 462944.
plotfi added a comment.

Updated based on @ahatanak's feedback.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

Files:
  clang/include/clang/AST/DeclObjC.h
  clang/lib/AST/DeclObjC.cpp
  clang/lib/AST/Mangle.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCRuntime.cpp
  clang/test/CodeGenObjC/objc-direct-wrapper.m

Index: clang/test/CodeGenObjC/objc-direct-wrapper.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/objc-direct-wrapper.m
@@ -0,0 +1,63 @@
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s | llvm-nm - | \
+// RUN: FileCheck -check-prefix=CHECK-DEFAULT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEF %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE %s
+
+// RUN: not %clang -fobjc-arc -Wno-objc-root-class -DENABLE_PROTOCOL_DIRECT_FAIL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm 2>&1 | \
+// RUN: FileCheck -check-prefix=CHECK-PROTOCOL-DIRECT-FAIL %s
+
+// CHECK-WRAPPER: T _-
+ // TODO: Fix this
+// CHECK-DEFAULT: t -[C testMethod:bar:]
+// CHECK-WRAPPER-IR-DEF: define {{(dso_local )?}}void @"-"
+// CHECK-WRAPPER-IR-DECLARE: declare {{(dso_local )?}}void @"-"
+// CHECK-PROTOCOL-DIRECT-FAIL: error: 'objc_direct' attribute cannot be applied to methods declared in an Objective-C protocol
+
+#if ENABLE_VISIBLE_OBJC_DIRECT
+#define OBJC_DIRECT __attribute__((objc_direct)) __attribute__((visibility("default")))
+#else
+#define OBJC_DIRECT
+#endif
+
+@interface C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT;
+@end
+
+#ifndef NO_OBJC_IMPL
+@implementation C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT {
+}
+@end
+#endif
+
+#ifdef ENABLE_PROTOCOL_DIRECT_FAIL
+@protocol ProtoDirectVisibleFail
+- (void)protoMethod OBJC_DIRECT;  // expected-error {{'objc_direct' attribute cannot be applied to methods declared in an Objective-C protocol}}
+@end
+#endif
+
+C *c;
+
+void f() {
+  [c testMethod:1 bar:1.0];
+}
Index: clang/lib/CodeGen/CGObjCRuntime.cpp
===
--- clang/lib/CodeGen/CGObjCRuntime.cpp
+++ clang/lib/CodeGen/CGObjCRuntime.cpp
@@ -474,7 +474,8 @@
   std::string buffer;
   llvm::raw_string_ostream out(buffer);
   CGM.getCXXABI().getMangleContext().mangleObjCMethodName(OMD, out,
-   /*includePrefixByte=*/true,
+   /*includePrefixByte=*/
+   !OMD->isVisibleDirectMethod(),
includeCategoryName);
   return buffer;
 }
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -760,7 +760,9 @@
 
   const CGFunctionInfo  = CGM.getTypes().arrangeObjCMethodDeclaration(OMD);
   if (OMD->isDirectMethod()) {
-Fn->setVisibility(llvm::Function::HiddenVisibility);
+Fn->setVisibility(OMD->isVisibleDirectMethod()
+  ? llvm::Function::DefaultVisibility
+  : llvm::Function::HiddenVisibility);
 CGM.SetLLVMFunctionAttributes(OMD, FI, Fn, /*IsThunk=*/false);
 CGM.SetLLVMFunctionAttributesForDefinition(OMD, Fn);
   } else {
Index: clang/lib/AST/Mangle.cpp
===
--- clang/lib/AST/Mangle.cpp
+++ clang/lib/AST/Mangle.cpp
@@ -359,10 +359,16 @@
   }
 
   // \01+[ContainerName(CategoryName) SelectorName]
+  // Note: If we are mangling for an objc_direct method with external visibility
+  //   then the standard mangling scheme will not work. We must replace the
+  //   square braces with angle braces so in the case of visible direct objc
+  //   methods it results in: +
+  //   This will include a prefix _ on Darwin.
   if (includePrefixByte) {
 OS << '\01';
   }
-  OS << (MD->isInstanceMethod() ? '-' : '+') 

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

In D86049#3814111 , @ahatanak wrote:

> In D86049#3812412 , @plotfi wrote:
>
>> In D86049#3812068 , @ahatanak wrote:
>>
>>> In D86049#3806898 , @plotfi wrote:
>>>
 1. Do we change the existing visibility behavior of objc methods? Yes / No
>>>
>>> I don't think we want to change the existing visibility behavior of 
>>> non-direct objc methods. Is there a use reason or use case for making them 
>>> visible outside the linkage unit?
>>>
 2. If we leave hidden as the default do we change the behavior for 
 objc_direct? Yes / No
>>>
>>> I think direct methods shouldn't be hidden by default (i.e., they should 
>>> get the default visibility). But it's not clear to me whether we should 
>>> make that change right away as I've heard concerns from people internally. 
>>> I think I need more time to understand what exactly their concerns are.
>>>
 3. If we leave objc_direct as hidden by default do we expand the existing 
 objc_direct attr to have the enum as you said so 
 `__attribute__((objc_direct("visible")))` or do we add a new attr as I 
 have done so far?
>>>
>>> I wasn't sure why it wasn't possible to use the existing 
>>> `__attribute__((visibility("default")))` attribute. Is it not possible to 
>>> make only the direct methods get the default visibility?
>>
>> This is not possible because default visibility is implicit (as far as I 
>> understand). It can not be checked if  
>> `__attribute__((visibility("default")))` is set because it is always set 
>> unless -fvisibility=hidden is passed on the command line. So either we treat 
>> direct methods like everything else, and hide them when  
>> `__attribute__((visibility("hidden")))` or the command line to hide 
>> everything by default is used, or we need a new attr or a new enum on the 
>> existing objc_direct attr.
>>
>> Does this make sense or am I missing some details?
>
> But there are ways to check whether the user explicitly added a visibility 
> attribute (e.g., `__attribute__((visibility("default")))`), right?  For 
> example, `NamedDecl::getExplicitVisibility`.
>
> I'm just not sure whether we want to add support for a new attribute like 
> `__attribute__((objc_direct("default")))` since it seems equivalent to 
> `__attribute__((objc_direct, visibility("default")))`.

Ah, thank you Akira. I had tried to explicitly check for this in some ways but 
not through `NamedDecl::getExplicitVisibility`. I will give this a try and if 
this works I think we can move forward.

Thanks Again

Puyan


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

In D86049#3812412 , @plotfi wrote:

> In D86049#3812068 , @ahatanak wrote:
>
>> In D86049#3806898 , @plotfi wrote:
>>
>>> 1. Do we change the existing visibility behavior of objc methods? Yes / No
>>
>> I don't think we want to change the existing visibility behavior of 
>> non-direct objc methods. Is there a use reason or use case for making them 
>> visible outside the linkage unit?
>>
>>> 2. If we leave hidden as the default do we change the behavior for 
>>> objc_direct? Yes / No
>>
>> I think direct methods shouldn't be hidden by default (i.e., they should get 
>> the default visibility). But it's not clear to me whether we should make 
>> that change right away as I've heard concerns from people internally. I 
>> think I need more time to understand what exactly their concerns are.
>>
>>> 3. If we leave objc_direct as hidden by default do we expand the existing 
>>> objc_direct attr to have the enum as you said so 
>>> `__attribute__((objc_direct("visible")))` or do we add a new attr as I have 
>>> done so far?
>>
>> I wasn't sure why it wasn't possible to use the existing 
>> `__attribute__((visibility("default")))` attribute. Is it not possible to 
>> make only the direct methods get the default visibility?
>
> This is not possible because default visibility is implicit (as far as I 
> understand). It can not be checked if  
> `__attribute__((visibility("default")))` is set because it is always set 
> unless -fvisibility=hidden is passed on the command line. So either we treat 
> direct methods like everything else, and hide them when  
> `__attribute__((visibility("hidden")))` or the command line to hide 
> everything by default is used, or we need a new attr or a new enum on the 
> existing objc_direct attr.
>
> Does this make sense or am I missing some details?

But there are ways to check whether the user explicitly added a visibility 
attribute (e.g., `__attribute__((visibility("default")))`), right?  For 
example, `NamedDecl::getExplicitVisibility`.

I'm just not sure whether we want to add support for a new attribute like 
`__attribute__((objc_direct("default")))` since it seems equivalent to 
`__attribute__((objc_direct, visibility("default")))`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2251-2256
+def ObjCDirectVisible : Attr {
+  let Spellings = [Clang<"objc_direct_visible">];
+  let Subjects = SubjectList<[ObjCMethod], ErrorDiag>;
+  let LangOpts = [ObjC];
+  let Documentation = [ObjCDirectVisibleDocs];
+}

plotfi wrote:
> mwyman wrote:
> > plotfi wrote:
> > > plotfi wrote:
> > > > mwyman wrote:
> > > > > Should this inherit `ObjCDirect`, to include both objc_direct and the 
> > > > > visibility aspect? I don't see any reason we would want to add 
> > > > > `objc_direct_visible` without also having `objc_direct`, so why make 
> > > > > developers add both?
> > > > > 
> > > > > As an alternative, would it make sense to allow adding 
> > > > > `__attribute__((visibility("default")))` on direct methods?
> > > > > 
> > > > > Also, it doesn't seem like this allows making `@property`s visible, 
> > > > > so should there be a similar attribute for properties?
> > > > I'd prefer to do `@property`s in a separate commit, but I suppose you 
> > > > are thinking like a `objc_direct_members_visible` attribute? I think I 
> > > > can add that in a subsequent commit. 
> > > > 
> > > > I took a look at how to make things inherit and I think the most 
> > > > straightforward way is to have `handleObjCDirectVisibleAttr` set the 
> > > > objc_direct attribute if it is not set already.
> > > > 
> > > > As for `__attribute__((visibility("default")))` I think the trouble 
> > > > lies in what we want the default visibility behavior for objc methods 
> > > > to be and if we want the behavior to be controlled by `-fvisibility=`. 
> > > > I tried going by attribute visibility before and had some trouble too 
> > > > (I forget exactly what though). 
> > > > 
> > > > 
> > > I gave visibility a try and it seems that the trouble is everything is 
> > > visible by default where for objc methods we want them hidden by default. 
> > > I think I would rather add a separate attr for this than add an 
> > > additional non-conformant visibility mode. 
> > Re: visibility, I wonder if it might make sense to create an optional enum 
> > argument on the `objc_direct` and `objc_direct_members` attributes, with 
> > either `hidden` or `visible` values (and presumably `hidden` being 
> > default); if we have an `objc_direct_members_visible`-like attribute, would 
> > there be cases where someone may wish to hide individual members?
> > 
> > This is quite possibly over-thinking the issue, but it also then avoids 
> > having an entirely new pair of method attributes. It doesn't solve the 
> > `@property` attributes, which don't have arguments, but it may be 
> > unavoidable to add a completely new attribute for that.
> Ah so something like `__attribute__((objc_direct("default")))` or  
> `__attribute__((objc_direct("visible")))` then? Hmm I wonder if that could be 
> just what we want, that actually sounds pretty good.
> Ah so something like `__attribute__((objc_direct("default")))` or  
> `__attribute__((objc_direct("visible")))` then? Hmm I wonder if that could be 
> just what we want, that actually sounds pretty good.

@mwyman One thing I am not sure of is, is it possible to add an enum argument 
to something like `__atttribute__(objc_direct)` while providing a default enum 
argument so that the visibility string doesn't always have to be specified. I 
have not seen a version of this 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

In D86049#3812068 , @ahatanak wrote:

> In D86049#3806898 , @plotfi wrote:
>
>> 1. Do we change the existing visibility behavior of objc methods? Yes / No
>
> I don't think we want to change the existing visibility behavior of 
> non-direct objc methods. Is there a use reason or use case for making them 
> visible outside the linkage unit?
>
>> 2. If we leave hidden as the default do we change the behavior for 
>> objc_direct? Yes / No
>
> I think direct methods shouldn't be hidden by default (i.e., they should get 
> the default visibility). But it's not clear to me whether we should make that 
> change right away as I've heard concerns from people internally. I think I 
> need more time to understand what exactly their concerns are.
>
>> 3. If we leave objc_direct as hidden by default do we expand the existing 
>> objc_direct attr to have the enum as you said so 
>> `__attribute__((objc_direct("visible")))` or do we add a new attr as I have 
>> done so far?
>
> I wasn't sure why it wasn't possible to use the existing 
> `__attribute__((visibility("default")))` attribute. Is it not possible to 
> make only the direct methods get the default visibility?

Also, if we make things visible at all we still need to alter the mangling. I 
could make the mangling changes into a separate diff to land that separately. 
Would that be preferable?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

In D86049#3812068 , @ahatanak wrote:

> In D86049#3806898 , @plotfi wrote:
>
>> 1. Do we change the existing visibility behavior of objc methods? Yes / No
>
> I don't think we want to change the existing visibility behavior of 
> non-direct objc methods. Is there a use reason or use case for making them 
> visible outside the linkage unit?
>
>> 2. If we leave hidden as the default do we change the behavior for 
>> objc_direct? Yes / No
>
> I think direct methods shouldn't be hidden by default (i.e., they should get 
> the default visibility). But it's not clear to me whether we should make that 
> change right away as I've heard concerns from people internally. I think I 
> need more time to understand what exactly their concerns are.
>
>> 3. If we leave objc_direct as hidden by default do we expand the existing 
>> objc_direct attr to have the enum as you said so 
>> `__attribute__((objc_direct("visible")))` or do we add a new attr as I have 
>> done so far?
>
> I wasn't sure why it wasn't possible to use the existing 
> `__attribute__((visibility("default")))` attribute. Is it not possible to 
> make only the direct methods get the default visibility?

This is not possible because default visibility is implicit (as far as I 
understand). It can not be checked if  `__attribute__((visibility("default")))` 
is set because it is always set unless -fvisibility=hidden is passed on the 
command line. So either we treat direct methods like everything else, and hide 
them when  `__attribute__((visibility("hidden")))` or the command line to hide 
everything by default is used, or we need a new attr or a new enum on the 
existing objc_direct attr.

Does this make sense or am I missing some details?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

In D86049#3806898 , @plotfi wrote:

> 1. Do we change the existing visibility behavior of objc methods? Yes / No

I don't think we want to change the existing visibility behavior of non-direct 
objc methods. Is there a use reason or use case for making them visible outside 
the linkage unit?

> 2. If we leave hidden as the default do we change the behavior for 
> objc_direct? Yes / No

I think direct methods shouldn't be hidden by default (i.e., they should get 
the default visibility). But it's not clear to me whether we should make that 
change right away as I've heard concerns from people internally. I think I need 
more time to understand what exactly their concerns are.

> 3. If we leave objc_direct as hidden by default do we expand the existing 
> objc_direct attr to have the enum as you said so 
> `__attribute__((objc_direct("visible")))` or do we add a new attr as I have 
> done so far?

I wasn't sure why it wasn't possible to use the existing 
`__attribute__((visibility("default")))` attribute. Is it not possible to make 
only the direct methods get the default visibility?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

@ahatanak @rjmccall

I think the decision tree goes

1. Do we change the existing visibility behavior of objc methods? Yes / No

2. If we leave hidden as the default do we change the behavior for objc_direct? 
Yes / No

3. If we leave objc_direct as hidden by default do we expand the existing 
objc_direct attr to have the enum as you said so 
__attribute__((objc_direct("visible"))) or do we add a new attr as I have done 
so far?

What do you think?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a subscriber: ahatanak.
plotfi added a comment.

@ahatanak @mwyman @rjmccall @dmaclach  I am going to work on a version of this 
patch where the visibility can be encoded into the objc_direct attribute. Does 
that seem reasonable to do from here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi marked an inline comment as done.
plotfi added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2251-2256
+def ObjCDirectVisible : Attr {
+  let Spellings = [Clang<"objc_direct_visible">];
+  let Subjects = SubjectList<[ObjCMethod], ErrorDiag>;
+  let LangOpts = [ObjC];
+  let Documentation = [ObjCDirectVisibleDocs];
+}

mwyman wrote:
> plotfi wrote:
> > plotfi wrote:
> > > mwyman wrote:
> > > > Should this inherit `ObjCDirect`, to include both objc_direct and the 
> > > > visibility aspect? I don't see any reason we would want to add 
> > > > `objc_direct_visible` without also having `objc_direct`, so why make 
> > > > developers add both?
> > > > 
> > > > As an alternative, would it make sense to allow adding 
> > > > `__attribute__((visibility("default")))` on direct methods?
> > > > 
> > > > Also, it doesn't seem like this allows making `@property`s visible, so 
> > > > should there be a similar attribute for properties?
> > > I'd prefer to do `@property`s in a separate commit, but I suppose you are 
> > > thinking like a `objc_direct_members_visible` attribute? I think I can 
> > > add that in a subsequent commit. 
> > > 
> > > I took a look at how to make things inherit and I think the most 
> > > straightforward way is to have `handleObjCDirectVisibleAttr` set the 
> > > objc_direct attribute if it is not set already.
> > > 
> > > As for `__attribute__((visibility("default")))` I think the trouble lies 
> > > in what we want the default visibility behavior for objc methods to be 
> > > and if we want the behavior to be controlled by `-fvisibility=`. I tried 
> > > going by attribute visibility before and had some trouble too (I forget 
> > > exactly what though). 
> > > 
> > > 
> > I gave visibility a try and it seems that the trouble is everything is 
> > visible by default where for objc methods we want them hidden by default. I 
> > think I would rather add a separate attr for this than add an additional 
> > non-conformant visibility mode. 
> Re: visibility, I wonder if it might make sense to create an optional enum 
> argument on the `objc_direct` and `objc_direct_members` attributes, with 
> either `hidden` or `visible` values (and presumably `hidden` being default); 
> if we have an `objc_direct_members_visible`-like attribute, would there be 
> cases where someone may wish to hide individual members?
> 
> This is quite possibly over-thinking the issue, but it also then avoids 
> having an entirely new pair of method attributes. It doesn't solve the 
> `@property` attributes, which don't have arguments, but it may be unavoidable 
> to add a completely new attribute for that.
Ah so something like `__attribute__((objc_direct("default")))` or  
`__attribute__((objc_direct("visible")))` then? Hmm I wonder if that could be 
just what we want, that actually sounds pretty good.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Michael Wyman via Phabricator via cfe-commits]

mwyman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2251-2256
+def ObjCDirectVisible : Attr {
+  let Spellings = [Clang<"objc_direct_visible">];
+  let Subjects = SubjectList<[ObjCMethod], ErrorDiag>;
+  let LangOpts = [ObjC];
+  let Documentation = [ObjCDirectVisibleDocs];
+}

plotfi wrote:
> plotfi wrote:
> > mwyman wrote:
> > > Should this inherit `ObjCDirect`, to include both objc_direct and the 
> > > visibility aspect? I don't see any reason we would want to add 
> > > `objc_direct_visible` without also having `objc_direct`, so why make 
> > > developers add both?
> > > 
> > > As an alternative, would it make sense to allow adding 
> > > `__attribute__((visibility("default")))` on direct methods?
> > > 
> > > Also, it doesn't seem like this allows making `@property`s visible, so 
> > > should there be a similar attribute for properties?
> > I'd prefer to do `@property`s in a separate commit, but I suppose you are 
> > thinking like a `objc_direct_members_visible` attribute? I think I can add 
> > that in a subsequent commit. 
> > 
> > I took a look at how to make things inherit and I think the most 
> > straightforward way is to have `handleObjCDirectVisibleAttr` set the 
> > objc_direct attribute if it is not set already.
> > 
> > As for `__attribute__((visibility("default")))` I think the trouble lies in 
> > what we want the default visibility behavior for objc methods to be and if 
> > we want the behavior to be controlled by `-fvisibility=`. I tried going by 
> > attribute visibility before and had some trouble too (I forget exactly what 
> > though). 
> > 
> > 
> I gave visibility a try and it seems that the trouble is everything is 
> visible by default where for objc methods we want them hidden by default. I 
> think I would rather add a separate attr for this than add an additional 
> non-conformant visibility mode. 
Re: visibility, I wonder if it might make sense to create an optional enum 
argument on the `objc_direct` and `objc_direct_members` attributes, with either 
`hidden` or `visible` values (and presumably `hidden` being default); if we 
have an `objc_direct_members_visible`-like attribute, would there be cases 
where someone may wish to hide individual members?

This is quite possibly over-thinking the issue, but it also then avoids having 
an entirely new pair of method attributes. It doesn't solve the `@property` 
attributes, which don't have arguments, but it may be unavoidable to add a 
completely new attribute for that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi marked an inline comment as done.
plotfi added inline comments.



Comment at: clang/test/CodeGenObjC/objc-direct-wrapper.m:35
+
+@interface C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT;

dmaclach wrote:
> I'd like to see a test for the protocol case for coverage.
How does this test look for protocol? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi updated this revision to Diff 459268.
plotfi added a comment.

Adding a test case to cover @protocol methods not being allowed to contain 
direct


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

Files:
  clang/include/clang/AST/DeclObjC.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/AST/DeclObjC.cpp
  clang/lib/AST/Mangle.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCRuntime.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGenObjC/objc-direct-wrapper.m

Index: clang/test/CodeGenObjC/objc-direct-wrapper.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/objc-direct-wrapper.m
@@ -0,0 +1,63 @@
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s | llvm-nm - | \
+// RUN: FileCheck -check-prefix=CHECK-DEFAULT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEFINE %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE %s
+
+// RUN: not %clang -fobjc-arc -Wno-objc-root-class -DENABLE_PROTOCOL_DIRECT_FAIL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm 2>&1 | \
+// RUN: FileCheck -check-prefix=CHECK-PROTOCOL-DIRECT-FAIL %s
+
+// CHECK-WRAPPER: T _-
+ // TODO: Fix this
+// CHECK-DEFAULT: t -[C testMethod:bar:]
+// CHECK-WRAPPER-IR-DEFINE: define {{(dso_local )?}}void @"-"
+// CHECK-WRAPPER-IR-DECLARE: declare {{(dso_local )?}}void @"-"
+// CHECK-PROTOCOL-DIRECT-FAIL: error: 'objc_direct' attribute cannot be applied to methods declared in an Objective-C protocol
+
+#if ENABLE_VISIBLE_OBJC_DIRECT
+#define OBJC_DIRECT __attribute__((objc_direct_visible))
+#else
+#define OBJC_DIRECT
+#endif
+
+@interface C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT;
+@end
+
+#ifndef NO_OBJC_IMPL
+@implementation C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT {
+}
+@end
+#endif
+
+#ifdef ENABLE_PROTOCOL_DIRECT_FAIL
+@protocol ProtoDirectVisibleFail
+- (void)protoMethod OBJC_DIRECT;  // expected-error {{'objc_direct' attribute cannot be applied to methods declared in an Objective-C protocol}}
+@end
+#endif
+
+C *c;
+
+void f() {
+  [c testMethod:1 bar:1.0];
+}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -2943,6 +2943,22 @@
   }
 }
 
+static void handleObjCDirectVisibleAttr(Sema , Decl *D, const ParsedAttr ) {
+  // objc_direct cannot be set on methods declared in the context of a protocol
+  if (isa(D->getDeclContext())) {
+S.Diag(AL.getLoc(), diag::err_objc_direct_on_protocol) << false;
+return;
+  }
+
+  if (S.getLangOpts().ObjCRuntime.allowsDirectDispatch()) {
+if (!D->hasAttr())
+  handleSimpleAttribute(S, D, AL);
+handleSimpleAttribute(S, D, AL);
+  } else {
+S.Diag(AL.getLoc(), diag::warn_objc_direct_ignored) << AL;
+  }
+}
+
 static void handleObjCMethodFamilyAttr(Sema , Decl *D, const ParsedAttr ) {
   const auto *M = cast(D);
   if (!AL.isArgIdent(0)) {
@@ -8783,6 +8799,9 @@
   case ParsedAttr::AT_ObjCDirect:
 handleObjCDirectAttr(S, D, AL);
 break;
+  case ParsedAttr::AT_ObjCDirectVisible:
+handleObjCDirectVisibleAttr(S, D, AL);
+break;
   case ParsedAttr::AT_ObjCDirectMembers:
 handleObjCDirectMembersAttr(S, D, AL);
 handleSimpleAttribute(S, D, AL);
Index: clang/lib/CodeGen/CGObjCRuntime.cpp
===
--- clang/lib/CodeGen/CGObjCRuntime.cpp
+++ clang/lib/CodeGen/CGObjCRuntime.cpp
@@ -474,7 +474,7 @@
   std::string buffer;
   llvm::raw_string_ostream out(buffer);
   CGM.getCXXABI().getMangleContext().mangleObjCMethodName(OMD, out,
-   /*includePrefixByte=*/true,
+   /*includePrefixByte=*/!OMD->isDirectMethodVisible(),
includeCategoryName);
   return buffer;
 }
Index: clang/lib/CodeGen/CGObjC.cpp

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi updated this revision to Diff 459267.
plotfi added a comment.

Update test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

Files:
  clang/include/clang/AST/DeclObjC.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/AST/DeclObjC.cpp
  clang/lib/AST/Mangle.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCRuntime.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGenObjC/objc-direct-wrapper.m

Index: clang/test/CodeGenObjC/objc-direct-wrapper.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/objc-direct-wrapper.m
@@ -0,0 +1,50 @@
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s | llvm-nm - | \
+// RUN: FileCheck -check-prefix=CHECK-DEFAULT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEFINE %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE %s
+
+// CHECK-WRAPPER: T _-
+ // TODO: Fix this
+// CHECK-DEFAULT: t -[C testMethod:bar:]
+// CHECK-WRAPPER-IR-DEFINE: define {{(dso_local )?}}void @"-"
+// CHECK-WRAPPER-IR-DECLARE: declare {{(dso_local )?}}void @"-"
+
+#if ENABLE_VISIBLE_OBJC_DIRECT
+#define OBJC_DIRECT __attribute__((objc_direct_visible))
+#else
+#define OBJC_DIRECT
+#endif
+
+@interface C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT;
+@end
+
+#ifndef NO_OBJC_IMPL
+@implementation C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT {
+}
+@end
+#endif
+
+C *c;
+
+void f() {
+  [c testMethod:1 bar:1.0];
+}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -2943,6 +2943,22 @@
   }
 }
 
+static void handleObjCDirectVisibleAttr(Sema , Decl *D, const ParsedAttr ) {
+  // objc_direct cannot be set on methods declared in the context of a protocol
+  if (isa(D->getDeclContext())) {
+S.Diag(AL.getLoc(), diag::err_objc_direct_on_protocol) << false;
+return;
+  }
+
+  if (S.getLangOpts().ObjCRuntime.allowsDirectDispatch()) {
+if (!D->hasAttr())
+  handleSimpleAttribute(S, D, AL);
+handleSimpleAttribute(S, D, AL);
+  } else {
+S.Diag(AL.getLoc(), diag::warn_objc_direct_ignored) << AL;
+  }
+}
+
 static void handleObjCMethodFamilyAttr(Sema , Decl *D, const ParsedAttr ) {
   const auto *M = cast(D);
   if (!AL.isArgIdent(0)) {
@@ -8783,6 +8799,9 @@
   case ParsedAttr::AT_ObjCDirect:
 handleObjCDirectAttr(S, D, AL);
 break;
+  case ParsedAttr::AT_ObjCDirectVisible:
+handleObjCDirectVisibleAttr(S, D, AL);
+break;
   case ParsedAttr::AT_ObjCDirectMembers:
 handleObjCDirectMembersAttr(S, D, AL);
 handleSimpleAttribute(S, D, AL);
Index: clang/lib/CodeGen/CGObjCRuntime.cpp
===
--- clang/lib/CodeGen/CGObjCRuntime.cpp
+++ clang/lib/CodeGen/CGObjCRuntime.cpp
@@ -474,7 +474,7 @@
   std::string buffer;
   llvm::raw_string_ostream out(buffer);
   CGM.getCXXABI().getMangleContext().mangleObjCMethodName(OMD, out,
-   /*includePrefixByte=*/true,
+   /*includePrefixByte=*/!OMD->isDirectMethodVisible(),
includeCategoryName);
   return buffer;
 }
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -760,7 +760,9 @@
 
   const CGFunctionInfo  = CGM.getTypes().arrangeObjCMethodDeclaration(OMD);
   if (OMD->isDirectMethod()) {
-Fn->setVisibility(llvm::Function::HiddenVisibility);
+Fn->setVisibility(OMD->isDirectMethodVisible()
+  ? llvm::Function::DefaultVisibility
+  : llvm::Function::HiddenVisibility);
 CGM.SetLLVMFunctionAttributes(OMD, FI, Fn, /*IsThunk=*/false);
 CGM.SetLLVMFunctionAttributesForDefinition(OMD, Fn);
   } else {
Index: clang/lib/AST/Mangle.cpp
===
--- clang/lib/AST/Mangle.cpp

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added inline comments.



Comment at: clang/test/CodeGenObjC/objc-direct-wrapper.m:30
+#if ENABLE_VISIBLE_OBJC_DIRECT
+#define OBJC_DIRECT __attribute((objc_direct)) 
__attribute__((objc_direct_visible))
+#else

dmaclach wrote:
> This is the case that mwyman described above where we would prefer to only 
> have the single attribute correct?
Ah, sorry about that. I implemented the code to have one single attr but I 
accidentally copy pasted both attrs in the test. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Dave MacLachlan via Phabricator via cfe-commits]

dmaclach added a comment.

Thanks Puyan! Awesome to see this moving forward.




Comment at: clang/test/CodeGenObjC/objc-direct-wrapper.m:30
+#if ENABLE_VISIBLE_OBJC_DIRECT
+#define OBJC_DIRECT __attribute((objc_direct)) 
__attribute__((objc_direct_visible))
+#else

This is the case that mwyman described above where we would prefer to only have 
the single attribute correct?



Comment at: clang/test/CodeGenObjC/objc-direct-wrapper.m:35
+
+@interface C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT;

I'd like to see a test for the protocol case for coverage.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi updated this revision to Diff 459261.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

Files:
  clang/include/clang/AST/DeclObjC.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/AST/DeclObjC.cpp
  clang/lib/AST/Mangle.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCRuntime.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGenObjC/objc-direct-wrapper.m

Index: clang/test/CodeGenObjC/objc-direct-wrapper.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/objc-direct-wrapper.m
@@ -0,0 +1,50 @@
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s | llvm-nm - | \
+// RUN: FileCheck -check-prefix=CHECK-DEFAULT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEFINE %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE %s
+
+// CHECK-WRAPPER: T _-
+ // TODO: Fix this
+// CHECK-DEFAULT: t -[C testMethod:bar:]
+// CHECK-WRAPPER-IR-DEFINE: define {{(dso_local )?}}void @"-"
+// CHECK-WRAPPER-IR-DECLARE: declare {{(dso_local )?}}void @"-"
+
+#if ENABLE_VISIBLE_OBJC_DIRECT
+#define OBJC_DIRECT __attribute((objc_direct)) __attribute__((objc_direct_visible))
+#else
+#define OBJC_DIRECT
+#endif
+
+@interface C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT;
+@end
+
+#ifndef NO_OBJC_IMPL
+@implementation C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT {
+}
+@end
+#endif
+
+C *c;
+
+void f() {
+  [c testMethod:1 bar:1.0];
+}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -2943,6 +2943,22 @@
   }
 }
 
+static void handleObjCDirectVisibleAttr(Sema , Decl *D, const ParsedAttr ) {
+  // objc_direct cannot be set on methods declared in the context of a protocol
+  if (isa(D->getDeclContext())) {
+S.Diag(AL.getLoc(), diag::err_objc_direct_on_protocol) << false;
+return;
+  }
+
+  if (S.getLangOpts().ObjCRuntime.allowsDirectDispatch()) {
+if (!D->hasAttr())
+  handleSimpleAttribute(S, D, AL);
+handleSimpleAttribute(S, D, AL);
+  } else {
+S.Diag(AL.getLoc(), diag::warn_objc_direct_ignored) << AL;
+  }
+}
+
 static void handleObjCMethodFamilyAttr(Sema , Decl *D, const ParsedAttr ) {
   const auto *M = cast(D);
   if (!AL.isArgIdent(0)) {
@@ -8783,6 +8799,9 @@
   case ParsedAttr::AT_ObjCDirect:
 handleObjCDirectAttr(S, D, AL);
 break;
+  case ParsedAttr::AT_ObjCDirectVisible:
+handleObjCDirectVisibleAttr(S, D, AL);
+break;
   case ParsedAttr::AT_ObjCDirectMembers:
 handleObjCDirectMembersAttr(S, D, AL);
 handleSimpleAttribute(S, D, AL);
Index: clang/lib/CodeGen/CGObjCRuntime.cpp
===
--- clang/lib/CodeGen/CGObjCRuntime.cpp
+++ clang/lib/CodeGen/CGObjCRuntime.cpp
@@ -474,7 +474,7 @@
   std::string buffer;
   llvm::raw_string_ostream out(buffer);
   CGM.getCXXABI().getMangleContext().mangleObjCMethodName(OMD, out,
-   /*includePrefixByte=*/true,
+   /*includePrefixByte=*/!OMD->isDirectMethodVisible(),
includeCategoryName);
   return buffer;
 }
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -760,7 +760,9 @@
 
   const CGFunctionInfo  = CGM.getTypes().arrangeObjCMethodDeclaration(OMD);
   if (OMD->isDirectMethod()) {
-Fn->setVisibility(llvm::Function::HiddenVisibility);
+Fn->setVisibility(OMD->isDirectMethodVisible()
+  ? llvm::Function::DefaultVisibility
+  : llvm::Function::HiddenVisibility);
 CGM.SetLLVMFunctionAttributes(OMD, FI, Fn, /*IsThunk=*/false);
 CGM.SetLLVMFunctionAttributesForDefinition(OMD, Fn);
   } else {
Index: clang/lib/AST/Mangle.cpp
===
--- clang/lib/AST/Mangle.cpp
+++ 

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi marked an inline comment as done.
plotfi added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2251-2256
+def ObjCDirectVisible : Attr {
+  let Spellings = [Clang<"objc_direct_visible">];
+  let Subjects = SubjectList<[ObjCMethod], ErrorDiag>;
+  let LangOpts = [ObjC];
+  let Documentation = [ObjCDirectVisibleDocs];
+}

plotfi wrote:
> mwyman wrote:
> > Should this inherit `ObjCDirect`, to include both objc_direct and the 
> > visibility aspect? I don't see any reason we would want to add 
> > `objc_direct_visible` without also having `objc_direct`, so why make 
> > developers add both?
> > 
> > As an alternative, would it make sense to allow adding 
> > `__attribute__((visibility("default")))` on direct methods?
> > 
> > Also, it doesn't seem like this allows making `@property`s visible, so 
> > should there be a similar attribute for properties?
> I'd prefer to do `@property`s in a separate commit, but I suppose you are 
> thinking like a `objc_direct_members_visible` attribute? I think I can add 
> that in a subsequent commit. 
> 
> I took a look at how to make things inherit and I think the most 
> straightforward way is to have `handleObjCDirectVisibleAttr` set the 
> objc_direct attribute if it is not set already.
> 
> As for `__attribute__((visibility("default")))` I think the trouble lies in 
> what we want the default visibility behavior for objc methods to be and if we 
> want the behavior to be controlled by `-fvisibility=`. I tried going by 
> attribute visibility before and had some trouble too (I forget exactly what 
> though). 
> 
> 
I gave visibility a try and it seems that the trouble is everything is visible 
by default where for objc methods we want them hidden by default. I think I 
would rather add a separate attr for this than add an additional non-conformant 
visibility mode. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi updated this revision to Diff 459260.
plotfi added a comment.

Updated to use mangleObjCMethodName in clang/lib/AST/Mangle.cpp to set the 
`_+` direct method name.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

Files:
  clang/include/clang/AST/DeclObjC.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/AST/DeclObjC.cpp
  clang/lib/AST/Mangle.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCRuntime.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGenObjC/objc-direct-wrapper.m

Index: clang/test/CodeGenObjC/objc-direct-wrapper.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/objc-direct-wrapper.m
@@ -0,0 +1,50 @@
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s | llvm-nm - | \
+// RUN: FileCheck -check-prefix=CHECK-DEFAULT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEFINE %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE %s
+
+// CHECK-WRAPPER: T _-
+ // TODO: Fix this
+// CHECK-DEFAULT: t -[C testMethod:bar:]
+// CHECK-WRAPPER-IR-DEFINE: define {{(dso_local )?}}void @"-"
+// CHECK-WRAPPER-IR-DECLARE: declare {{(dso_local )?}}void @"-"
+
+#if ENABLE_VISIBLE_OBJC_DIRECT
+#define OBJC_DIRECT __attribute((objc_direct)) __attribute__((objc_direct_visible))
+#else
+#define OBJC_DIRECT
+#endif
+
+@interface C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT __attribute__((visibility("default"))) ;
+@end
+
+#ifndef NO_OBJC_IMPL
+@implementation C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT {
+}
+@end
+#endif
+
+C *c;
+
+__attribute__((visibility("hidden"))) void f() {
+  [c testMethod:1 bar:1.0];
+}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -2943,6 +2943,22 @@
   }
 }
 
+static void handleObjCDirectVisibleAttr(Sema , Decl *D, const ParsedAttr ) {
+  // objc_direct cannot be set on methods declared in the context of a protocol
+  if (isa(D->getDeclContext())) {
+S.Diag(AL.getLoc(), diag::err_objc_direct_on_protocol) << false;
+return;
+  }
+
+  if (S.getLangOpts().ObjCRuntime.allowsDirectDispatch()) {
+if (!D->hasAttr())
+  handleSimpleAttribute(S, D, AL);
+handleSimpleAttribute(S, D, AL);
+  } else {
+S.Diag(AL.getLoc(), diag::warn_objc_direct_ignored) << AL;
+  }
+}
+
 static void handleObjCMethodFamilyAttr(Sema , Decl *D, const ParsedAttr ) {
   const auto *M = cast(D);
   if (!AL.isArgIdent(0)) {
@@ -8783,6 +8799,9 @@
   case ParsedAttr::AT_ObjCDirect:
 handleObjCDirectAttr(S, D, AL);
 break;
+  case ParsedAttr::AT_ObjCDirectVisible:
+handleObjCDirectVisibleAttr(S, D, AL);
+break;
   case ParsedAttr::AT_ObjCDirectMembers:
 handleObjCDirectMembersAttr(S, D, AL);
 handleSimpleAttribute(S, D, AL);
Index: clang/lib/CodeGen/CGObjCRuntime.cpp
===
--- clang/lib/CodeGen/CGObjCRuntime.cpp
+++ clang/lib/CodeGen/CGObjCRuntime.cpp
@@ -474,7 +474,7 @@
   std::string buffer;
   llvm::raw_string_ostream out(buffer);
   CGM.getCXXABI().getMangleContext().mangleObjCMethodName(OMD, out,
-   /*includePrefixByte=*/true,
+   /*includePrefixByte=*/!OMD->isDirectMethodVisible(),
includeCategoryName);
   return buffer;
 }
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -760,7 +760,9 @@
 
   const CGFunctionInfo  = CGM.getTypes().arrangeObjCMethodDeclaration(OMD);
   if (OMD->isDirectMethod()) {
-Fn->setVisibility(llvm::Function::HiddenVisibility);
+Fn->setVisibility(OMD->isDirectMethodVisible()
+  ? llvm::Function::DefaultVisibility
+  : llvm::Function::HiddenVisibility);
 CGM.SetLLVMFunctionAttributes(OMD, FI, Fn, /*IsThunk=*/false);
 

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2251-2256
+def ObjCDirectVisible : Attr {
+  let Spellings = [Clang<"objc_direct_visible">];
+  let Subjects = SubjectList<[ObjCMethod], ErrorDiag>;
+  let LangOpts = [ObjC];
+  let Documentation = [ObjCDirectVisibleDocs];
+}

mwyman wrote:
> Should this inherit `ObjCDirect`, to include both objc_direct and the 
> visibility aspect? I don't see any reason we would want to add 
> `objc_direct_visible` without also having `objc_direct`, so why make 
> developers add both?
> 
> As an alternative, would it make sense to allow adding 
> `__attribute__((visibility("default")))` on direct methods?
> 
> Also, it doesn't seem like this allows making `@property`s visible, so should 
> there be a similar attribute for properties?
I'd prefer to do `@property`s in a separate commit, but I suppose you are 
thinking like a `objc_direct_members_visible` attribute? I think I can add that 
in a subsequent commit. 

I took a look at how to make things inherit and I think the most 
straightforward way is to have `handleObjCDirectVisibleAttr` set the 
objc_direct attribute if it is not set already.

As for `__attribute__((visibility("default")))` I think the trouble lies in 
what we want the default visibility behavior for objc methods to be and if we 
want the behavior to be controlled by `-fvisibility=`. I tried going by 
attribute visibility before and had some trouble too (I forget exactly what 
though). 





Comment at: clang/lib/CodeGen/CGObjCMac.cpp:4025-4036
+  if (OMD->isDirectMethodVisible() && Fn->getName().str()[0] == '\1') {
+// Drop '\1' to work with dlsym.
+std::string Name = Fn->getName().str().substr(1);
+
+assert(Name[0] == '-' || Name[0] == '+');
+assert(Name[1] == '[' && Name[Name.length() - 1] == ']');
+

mwyman wrote:
> Should this move to clang/lib/AST/Mangle.cpp's mangleObjCMethodName?
I like this idea. I will look into doing it next.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi updated this revision to Diff 459257.
plotfi added a comment.

Updated with some of @mwyman's feedback.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

Files:
  clang/include/clang/AST/DeclObjC.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/AST/DeclObjC.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGenObjC/objc-direct-wrapper.m

Index: clang/test/CodeGenObjC/objc-direct-wrapper.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/objc-direct-wrapper.m
@@ -0,0 +1,50 @@
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s | llvm-nm - | \
+// RUN: FileCheck -check-prefix=CHECK-DEFAULT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEFINE %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE %s
+
+// CHECK-WRAPPER: T _-
+ // TODO: Fix this
+// CHECK-DEFAULT: t -[C testMethod:bar:]
+// CHECK-WRAPPER-IR-DEFINE: define {{(dso_local )?}}void @"-"
+// CHECK-WRAPPER-IR-DECLARE: declare {{(dso_local )?}}void @"-"
+
+#if ENABLE_VISIBLE_OBJC_DIRECT
+#define OBJC_DIRECT __attribute((objc_direct)) __attribute__((objc_direct_visible))
+#else
+#define OBJC_DIRECT
+#endif
+
+@interface C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT __attribute__((visibility("default"))) ;
+@end
+
+#ifndef NO_OBJC_IMPL
+@implementation C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT {
+}
+@end
+#endif
+
+C *c;
+
+__attribute__((visibility("hidden"))) void f() {
+  [c testMethod:1 bar:1.0];
+}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -2943,6 +2943,22 @@
   }
 }
 
+static void handleObjCDirectVisibleAttr(Sema , Decl *D, const ParsedAttr ) {
+  // objc_direct cannot be set on methods declared in the context of a protocol
+  if (isa(D->getDeclContext())) {
+S.Diag(AL.getLoc(), diag::err_objc_direct_on_protocol) << false;
+return;
+  }
+
+  if (S.getLangOpts().ObjCRuntime.allowsDirectDispatch()) {
+if (!D->hasAttr())
+  handleSimpleAttribute(S, D, AL);
+handleSimpleAttribute(S, D, AL);
+  } else {
+S.Diag(AL.getLoc(), diag::warn_objc_direct_ignored) << AL;
+  }
+}
+
 static void handleObjCMethodFamilyAttr(Sema , Decl *D, const ParsedAttr ) {
   const auto *M = cast(D);
   if (!AL.isArgIdent(0)) {
@@ -8783,6 +8799,9 @@
   case ParsedAttr::AT_ObjCDirect:
 handleObjCDirectAttr(S, D, AL);
 break;
+  case ParsedAttr::AT_ObjCDirectVisible:
+handleObjCDirectVisibleAttr(S, D, AL);
+break;
   case ParsedAttr::AT_ObjCDirectMembers:
 handleObjCDirectMembersAttr(S, D, AL);
 handleSimpleAttribute(S, D, AL);
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -4021,6 +4021,20 @@
 DirectMethodDefinitions.insert(std::make_pair(COMD, Fn));
   }
 
+  // If we want to explicitly export objc_direct methods, we need a name fixup.
+  if (OMD->isDirectMethodVisible() && Fn->getName().str()[0] == '\1') {
+// Drop '\1' to work with dlsym.
+std::string Name = Fn->getName().str().substr(1);
+
+assert(Name[0] == '-' || Name[0] == '+');
+assert(Name[1] == '[' && Name[Name.length() - 1] == ']');
+
+// replace "[ ]" by  "< >" to avoid strip by ld64.
+Name = Name.substr(0, 1) + "<" + Name.substr(2, Name.length() - 3) + ">";
+
+Fn->setName(Name);
+  }
+
   return Fn;
 }
 
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -760,7 +760,9 @@
 
   const CGFunctionInfo  = CGM.getTypes().arrangeObjCMethodDeclaration(OMD);
   if (OMD->isDirectMethod()) {
-Fn->setVisibility(llvm::Function::HiddenVisibility);
+Fn->setVisibility(OMD->isDirectMethodVisible()
+  ? 

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Michael Wyman via Phabricator via cfe-commits]

mwyman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:2251-2256
+def ObjCDirectVisible : Attr {
+  let Spellings = [Clang<"objc_direct_visible">];
+  let Subjects = SubjectList<[ObjCMethod], ErrorDiag>;
+  let LangOpts = [ObjC];
+  let Documentation = [ObjCDirectVisibleDocs];
+}

Should this inherit `ObjCDirect`, to include both objc_direct and the 
visibility aspect? I don't see any reason we would want to add 
`objc_direct_visible` without also having `objc_direct`, so why make developers 
add both?

As an alternative, would it make sense to allow adding 
`__attribute__((visibility("default")))` on direct methods?

Also, it doesn't seem like this allows making `@property`s visible, so should 
there be a similar attribute for properties?



Comment at: clang/lib/CodeGen/CGObjCMac.cpp:4025-4036
+  if (OMD->isDirectMethodVisible() && Fn->getName().str()[0] == '\1') {
+// Drop '\1' to work with dlsym.
+std::string Name = Fn->getName().str().substr(1);
+
+assert(Name[0] == '-' || Name[0] == '+');
+assert(Name[1] == '[' && Name[Name.length() - 1] == ']');
+

Should this move to clang/lib/AST/Mangle.cpp's mangleObjCMethodName?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi updated this revision to Diff 451041.
plotfi added a comment.
Herald added a reviewer: aaron.ballman.

Updating implementation to use an objc_direct_visible attr to explicitly mark 
when we want objc_direct to be exposed outside of the link unit.

Work on dropping the selector param has been proposed in D131424 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

Files:
  clang/include/clang/AST/DeclObjC.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/AST/DeclObjC.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGenObjC/objc-direct-wrapper.m

Index: clang/test/CodeGenObjC/objc-direct-wrapper.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/objc-direct-wrapper.m
@@ -0,0 +1,41 @@
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -target x86_64-apple-macosx10.15.0 -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s | llvm-nm - | \
+// RUN: FileCheck -check-prefix=CHECK-DEFAULT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEFINE %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE %s
+
+// CHECK-WRAPPER: T _-
+ // TODO: Fix this
+// CHECK-DEFAULT: T -[C testMethod:bar:]
+// CHECK-WRAPPER-IR-DEFINE: define {{(dso_local )?}}void @"-"
+// CHECK-WRAPPER-IR-DECLARE: declare {{(dso_local )?}}void @"-"
+
+@interface C
+- (void)testMethod:(int)arg1 bar:(float)arg2 __attribute((objc_direct)) __attribute__((objc_direct_visible));
+@end
+
+#ifndef NO_OBJC_IMPL
+@implementation C
+- (void)testMethod:(int)arg1 bar:(float)arg2 __attribute((objc_direct)) __attribute__((objc_direct_visible)) {
+}
+@end
+#endif
+
+C *c;
+
+void f() {
+  [c testMethod:1 bar:1.0];
+}
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -2943,6 +2943,20 @@
   }
 }
 
+static void handleObjCDirectVisibleAttr(Sema , Decl *D, const ParsedAttr ) {
+  // objc_direct cannot be set on methods declared in the context of a protocol
+  if (isa(D->getDeclContext())) {
+S.Diag(AL.getLoc(), diag::err_objc_direct_on_protocol) << false;
+return;
+  }
+
+  if (S.getLangOpts().ObjCRuntime.allowsDirectDispatch()) {
+handleSimpleAttribute(S, D, AL);
+  } else {
+S.Diag(AL.getLoc(), diag::warn_objc_direct_ignored) << AL;
+  }
+}
+
 static void handleObjCMethodFamilyAttr(Sema , Decl *D, const ParsedAttr ) {
   const auto *M = cast(D);
   if (!AL.isArgIdent(0)) {
@@ -8782,6 +8796,9 @@
   case ParsedAttr::AT_ObjCDirect:
 handleObjCDirectAttr(S, D, AL);
 break;
+  case ParsedAttr::AT_ObjCDirectVisible:
+handleObjCDirectVisibleAttr(S, D, AL);
+break;
   case ParsedAttr::AT_ObjCDirectMembers:
 handleObjCDirectMembersAttr(S, D, AL);
 handleSimpleAttribute(S, D, AL);
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -4021,6 +4021,20 @@
 DirectMethodDefinitions.insert(std::make_pair(COMD, Fn));
   }
 
+  // If we want to explicitly export objc_direct methods, we need a name fixup.
+  if (OMD->isDirectMethodVisible() && Fn->getName().str()[0] == '\1') {
+// Drop '\1' to work with dlsym.
+std::string Name = Fn->getName().str().substr(1);
+
+assert(Name[0] == '-' || Name[0] == '+');
+assert(Name[1] == '[' && Name[Name.length() - 1] == ']');
+
+// replace "[ ]" by  "< >" to avoid strip by ld64.
+Name = Name.substr(0, 1) + "<" + Name.substr(2, Name.length() - 3) + ">";
+
+Fn->setName(Name);
+  }
+
   return Fn;
 }
 
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -761,6 +761,8 @@
   const CGFunctionInfo  = CGM.getTypes().arrangeObjCMethodDeclaration(OMD);
   if (OMD->isDirectMethod()) {
 Fn->setVisibility(llvm::Function::HiddenVisibility);
+if (OMD->isDirectMethodVisible())
+  Fn->setVisibility(llvm::Function::DefaultVisibility);
 CGM.SetLLVMFunctionAttributes(OMD, FI, Fn, /*IsThunk=*/false);
 

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

In D86049#3663726 , @mwyman wrote:

> Hi, I work with @dmaclach. I know this has been sitting around without much 
> activity for two years, but we believe there is a solid use-case for this on 
> our end and I'd like to help get the ABI nailed down and land this change (or 
> one accomplishing the same goal). Do you still have interest in this?

Absolutely. I would be willing to work with all of you to evolve this. Only 
reason I put it aside was that I thought interest was not there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Michael Wyman via Phabricator via cfe-commits]

mwyman added a comment.

Hi, I work with @dmaclach. I know this has been sitting around without much 
activity for two years, but we believe there is a solid use-case for this on 
our end and I'd like to help get the ABI nailed down and land this change (or 
one accomplishing the same goal). Do you still have interest in this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

In D86049#2844766 , @dmaclach wrote:

> Has anything happened with this at all or did it get dropped?

I can work with you and @rjmccall to land a version of this. We can chat 
offline if you'd like too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Dave MacLachlan via Phabricator via cfe-commits]

dmaclach added a comment.

Has anything happened with this at all or did it get dropped?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Manman Ren via Phabricator via cfe-commits]

manmanren added a comment.



In D86049#2255738 , @rjmccall wrote:

> We just talk about it.  I agree with Nathan that we shouldn't just add this 
> as a short-term hack; we should design the ABI right and then do what we want.
>
> I think these are basically all the ABI questions:
>
> - Is there a good reason to preserve signature compatibility with normal ObjC 
> methods, or should we just drop the selector argument on direct methods?
> - Should we do the null test on the caller side or the callee side?
> - In direct class methods, is the caller's responsibility or the callee's to 
> ensure that `self` is initialized?
>
> For the first, I think dropping the argument is fine.

Dropping the argument sounds good to me! We are already passing the argument as 
undefined.

> For the second, I'm less certain.

It feels cleaner to do the null test on the callee side (i.e inside the 
wrapper). It may have binary size advantage compared to checking at each 
callsite.

> For the third, making it the callee's responsibility is generally better for 
> code size but harder to optimize.  But we only have the code-size cost on the 
> caller side when materializing from a global, and that's actually something 
> we can also eliminate as redundant.  So maybe the best approach is to 
> introduce a weak+hidden thunk that we use when making a call to a direct 
> class method on a specific class, and we have that thunk do the entire 
> materialization sequence.

Completely agree. For direct class method, making sure 'self' is initialized on 
the callee side is cleaner and better for code size. But caller has the context 
to know whether the class is already initialized.
Maybe we can start with doing it on callee's side and make an incremental 
improvement by implementing the approach that can get both benefits.

Do we need to support direct methods with variadic arguments for the wrapper 
implementation or are we going with the non-wrapper version? @plotfi

Thanks!
Manman


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

We just talk about it.  I agree with Nathan that we shouldn't just add this as 
a short-term hack; we should design the ABI right and then do what we want.

I think these are basically all the ABI questions:

- Is there a good reason to preserve signature compatibility with normal ObjC 
methods, or should we just drop the selector argument on direct methods?
- Should we do the null test on the caller side or the callee side?
- In direct class methods, is the caller's responsibility or the callee's to 
ensure that `self` is initialized?

For the first, I think dropping the argument is fine.

For the second, I'm less certain.

For the third, making it the callee's responsibility is generally better for 
code size but harder to optimize.  But we only have the code-size cost on the 
caller side when materializing from a global, and that's actually something we 
can also eliminate as redundant.  So maybe the best approach is to introduce a 
weak+hidden thunk that we use when making a call to a direct class method on a 
specific class, and we have that thunk do the entire materialization sequence.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

In D86049#2253705 , @plotfi wrote:

> I've updated the diff to reflect the alternate non-wrapper exposure. This way 
> requires the sanitizing of the exported objc_direct method name.

@rjmccall Is there a process for discussing ABI issues and considerations for 
something like this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi updated this revision to Diff 289616.
plotfi added a comment.

I've updated the diff to reflect the alternate non-wrapper exposure. This way 
requires the sanitizing of the exported objc_direct method name.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenObjC/objc-direct-wrapper.m

Index: clang/test/CodeGenObjC/objc-direct-wrapper.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/objc-direct-wrapper.m
@@ -0,0 +1,44 @@
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN:   -fobjc-export-direct-methods -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s | llvm-nm - | \
+// RUN: FileCheck -check-prefix=CHECK-DEFAULT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN:   -fobjc-export-direct-methods -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEFINE %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN:   -fobjc-export-direct-methods -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE %s
+
+// CHECK-WRAPPER: T _-
+ // TODO: Fix this
+// CHECK-DEFAULT: T -[C testMethod:bar:]
+// CHECK-WRAPPER-IR-DEFINE: define {{(dso_local )?}}void @"-"
+// CHECK-WRAPPER-IR-DECLARE: declare {{(dso_local )?}}void @"-"
+
+@interface C
+- (void)testMethod:(int)arg1 bar:(float)arg2 __attribute((objc_direct));
+@end
+
+#ifndef NO_OBJC_IMPL
+@implementation C
+- (void)testMethod:(int)arg1 bar:(float)arg2 __attribute((objc_direct)) {
+}
+@end
+#endif
+
+C *c;
+
+void f() {
+  [c testMethod:1 bar:1.0];
+}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -811,6 +811,7 @@
   Opts.DebugFwdTemplateParams = Args.hasArg(OPT_debug_forward_template_params);
   Opts.EmbedSource = Args.hasArg(OPT_gembed_source);
   Opts.ForceDwarfFrameSection = Args.hasArg(OPT_fforce_dwarf_frame);
+  Opts.ObjCExportDirectMethods = Args.hasArg(OPT_fobjc_export_direct_methods);
 
   for (const auto  : Args.getAllArgValues(OPT_fdebug_prefix_map_EQ)) {
 auto Split = StringRef(Arg).split('=');
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -6267,6 +6267,9 @@
   Input.getInputArg().renderAsInput(Args, CmdArgs);
   }
 
+  if (Args.hasArg(options::OPT_fobjc_export_direct_methods))
+CmdArgs.push_back("-fobjc-export-direct-methods");
+
   // Finally add the compile command to the compilation.
   if (Args.hasArg(options::OPT__SLASH_fallback) &&
   Output.getType() == types::TY_Object &&
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -4068,6 +4068,21 @@
 DirectMethodDefinitions.insert(std::make_pair(COMD, Fn));
   }
 
+  // If we want to explicitly export objc_direct methods, we need a name fixup.
+  if (CGM.getCodeGenOpts().ObjCExportDirectMethods &&
+  Fn->getName().str()[0] == '\1') {
+// Drop '\1' to work with dlsym.
+std::string Name = Fn->getName().str().substr(1);
+
+assert(Name[0] == '-' || Name[0] == '+');
+assert(Name[1] == '[' && Name[Name.length() - 1] == ']');
+
+// replace "[ ]" by  "< >" to avoid strip by ld64.
+Name = Name.substr(0, 1) + "<" + Name.substr(2, Name.length() - 3) + ">";
+
+Fn->setName(Name);
+  }
+
   return Fn;
 }
 
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -689,6 +689,8 @@
   const CGFunctionInfo  = CGM.getTypes().arrangeObjCMethodDeclaration(OMD);
   if (OMD->isDirectMethod()) {
 Fn->setVisibility(llvm::Function::HiddenVisibility);
+if (CGM.getCodeGenOpts().ObjCExportDirectMethods)
+  Fn->setVisibility(llvm::Function::DefaultVisibility);
 CGM.SetLLVMFunctionAttributes(OMD, FI, Fn);
 

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

@lanza I did it as a CodeGen option for now because we don't want anything like 
this to be the default until the ABI is fleshed out.
I think one danger in altering the name of the function to some extent is you 
dont want to clash potentially with user defined C functions.

In D86049#2226507 , @lanza wrote:

>> This change provides a codegen options flag to clang 
>> -fobjc-export-direct-method-wrappers to generate the wrapper functions that 
>> begin with the prefix objc_direct_wrapper and are marked as 
>> attribute__((alwaysinline)). This way within a link unit the wrapper 
>> functions should be inlined away at their call sites, but across a dylib 
>> boundary the wrapper call is used.
>
> I think this needing a flag is a bit magical for what seems like such a 
> simple concept for the ObjC language. There shouldn't need to be a flag to 
> make it work properly.
>
> We have defined more-or-less non-`virtual` methods for ObjC with 
> `objc_direct`. The symbol visibility should behave as a reasonable developer 
> should expect and the front end should communicate it. Requiring a flag is 
> the compiler requiring the developer to understand compiler implementation 
> details for his iOS app to work properly. Currently, clang decides the 
> visibility is hidden but can't communicate this across library boundaries and 
> you end up with confusing link errors for the developer.
>
> There's two routes that make sense in my opinion: library private and class 
> private OR library public and class public.
>
> As per the ABI, I don't know of any reasons why `objc_direct` methods require 
> the standard ObjC `-[class method]` signature since they won't be 
> participating in any part of the ObjectiveC runtime. If that's the case,  a 
> proper underscored symbol name seems like the cleaning and most reasonable 
> fix.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Nathan Lanza via Phabricator via cfe-commits]

lanza added a comment.

> This change provides a codegen options flag to clang 
> -fobjc-export-direct-method-wrappers to generate the wrapper functions that 
> begin with the prefix objc_direct_wrapper and are marked as 
> attribute__((alwaysinline)). This way within a link unit the wrapper 
> functions should be inlined away at their call sites, but across a dylib 
> boundary the wrapper call is used.

I think this needing a flag is a bit magical for what seems like such a simple 
concept for the ObjC language. There shouldn't need to be a flag to make it 
work properly.

We have defined more-or-less non-`virtual` methods for ObjC with `objc_direct`. 
The symbol visibility should behave as a reasonable developer should expect and 
the front end should communicate it. Requiring a flag is the compiler requiring 
the developer to understand compiler implementation details for his iOS app to 
work properly. Currently, clang decides the visibility is hidden but can't 
communicate this across library boundaries and you end up with confusing link 
errors for the developer.

There's two routes that make sense in my opinion: library private and class 
private OR library public and class public.

As per the ABI, I don't know of any reasons why `objc_direct` methods require 
the standard ObjC `-[class method]` signature since they won't be participating 
in any part of the ObjectiveC runtime. If that's the case,  a proper 
underscored symbol name seems like the cleaning and most reasonable fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

In D86049#2225202 , @rjmccall wrote:

> I think that, if we want to do this, we need to think carefully about what 
> exactly we want the ABI to be.

I agree with this very much. Open to suggestions and ideas. Already started 
noticing some interesting characteristic with the symbol naming and dylib 
loading.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I think that, if we want to do this, we need to think carefully about what 
exactly we want the ABI to be.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

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

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi updated this revision to Diff 286141.
plotfi added a comment.

change for clang-tidy and clang-format.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenObjC/objc-direct-wrapper.m

Index: clang/test/CodeGenObjC/objc-direct-wrapper.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/objc-direct-wrapper.m
@@ -0,0 +1,43 @@
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN:   -fobjc-export-direct-method-wrappers -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s | llvm-nm - | \
+// RUN: FileCheck -check-prefix=CHECK-DEFAULT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN:   -fobjc-export-direct-method-wrappers -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEFINE %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN:   -fobjc-export-direct-method-wrappers -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE %s
+
+// CHECK-WRAPPER: T ___objc_direct_wrapper-[C testMethod:bar:]
+// CHECK-DEFAULT-NOT: ___objc_direct_wrapper
+// CHECK-WRAPPER-IR-DEFINE: define {{(dso_local )?}}{{(dso_local )?}}void @"__objc_direct_wrapper-[C testMethod:bar:]"
+// CHECK-WRAPPER-IR-DECLARE: declare {{(dso_local )?}}void @"__objc_direct_wrapper-[C testMethod:bar:]"
+
+@interface C
+- (void)testMethod:(int)arg1 bar:(float)arg2 __attribute((objc_direct));
+@end
+
+#ifndef NO_OBJC_IMPL
+@implementation C
+- (void)testMethod:(int)arg1 bar:(float)arg2 __attribute((objc_direct)) {
+}
+@end
+#endif
+
+C *c;
+
+void f() {
+  [c testMethod:1 bar:1.0];
+}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1283,6 +1283,8 @@
 }
   }
 
+  if (Args.hasArg(OPT_fobjc_export_direct_method_wrappers))
+Opts.ObjCExportDirectMethodWrappers = 1;
 
   if (Args.hasArg(OPT_fno_objc_convert_messages_to_runtime_calls))
 Opts.ObjCConvertMessagesToRuntimeCalls = 0;
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3462,6 +3462,9 @@
   CmdArgs.push_back("-fno-objc-convert-messages-to-runtime-calls");
   }
 
+  if (Args.hasArg(options::OPT_fobjc_export_direct_method_wrappers))
+CmdArgs.push_back("-fobjc-export-direct-method-wrappers");
+
   // -fobjc-infer-related-result-type is the default, except in the Objective-C
   // rewriter.
   if (InferCovariantReturns)
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -2143,6 +2143,36 @@
   return false;
 }
 
+/// Create or look up the declaration of an objc_direct method wrapper.
+llvm::Function *getObjCDirectWrapper(llvm::Function ) {
+  std::string NewName = "__objc_direct_wrapper";
+  for (auto C : F.getName().str()) {
+if (C == '\1')
+  continue;
+NewName += C;
+  }
+
+  auto WI = llvm::find_if(*F.getParent(), [NewName](const llvm::Function ) {
+return F.getName() == NewName;
+  });
+
+  llvm::Function *Wrapper = nullptr;
+  if (WI == F.getParent()->end()) {
+llvm::Module  = *F.getParent();
+llvm::FunctionType *FnTy = F.getFunctionType();
+Wrapper = llvm::Function::Create(FnTy, F.getLinkage(), F.getAddressSpace(),
+ NewName);
+Wrapper->setLinkage(llvm::GlobalValue::ExternalLinkage);
+Wrapper->addFnAttr(llvm::Attribute::AlwaysInline);
+Wrapper->setVisibility(llvm::Function::DefaultVisibility);
+M.getFunctionList().insert(F.getIterator(), Wrapper);
+  } else {
+Wrapper = &*WI;
+  }
+
+  return Wrapper;
+}
+
 CodeGen::RValue
 CGObjCCommonMac::EmitMessageSend(CodeGen::CodeGenFunction ,
  ReturnValueSlot Return,
@@ -2214,7 +2244,13 @@
 
   llvm::FunctionCallee Fn = nullptr;
   if (Method && Method->isDirectMethod()) {
-Fn = GenerateDirectMethod(Method, 

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi updated this revision to Diff 285925.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86049/new/

https://reviews.llvm.org/D86049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenObjC/objc-direct-wrapper.m

Index: clang/test/CodeGenObjC/objc-direct-wrapper.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/objc-direct-wrapper.m
@@ -0,0 +1,43 @@
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN:   -fobjc-export-direct-method-wrappers -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s | llvm-nm - | \
+// RUN: FileCheck -check-prefix=CHECK-DEFAULT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN:   -fobjc-export-direct-method-wrappers -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEFINE %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN:   -fobjc-export-direct-method-wrappers -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE %s
+
+// CHECK-WRAPPER: T ___objc_direct_wrapper-[C testMethod:bar:]
+// CHECK-DEFAULT-NOT: ___objc_direct_wrapper
+// CHECK-WRAPPER-IR-DEFINE: define {{(dso_local )?}}{{(dso_local )?}}void @"__objc_direct_wrapper-[C testMethod:bar:]"
+// CHECK-WRAPPER-IR-DECLARE: declare {{(dso_local )?}}void @"__objc_direct_wrapper-[C testMethod:bar:]"
+
+@interface C
+- (void)testMethod:(int)arg1 bar:(float)arg2 __attribute((objc_direct));
+@end
+
+#ifndef NO_OBJC_IMPL
+@implementation C
+- (void)testMethod:(int)arg1 bar:(float)arg2 __attribute((objc_direct)) {
+}
+@end
+#endif
+
+C *c;
+
+void f() {
+  [c testMethod:1 bar:1.0];
+}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1283,6 +1283,8 @@
 }
   }
 
+  if (Args.hasArg(OPT_fobjc_export_direct_method_wrappers))
+Opts.ObjCExportDirectMethodWrappers = 1;
 
   if (Args.hasArg(OPT_fno_objc_convert_messages_to_runtime_calls))
 Opts.ObjCConvertMessagesToRuntimeCalls = 0;
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3462,6 +3462,9 @@
   CmdArgs.push_back("-fno-objc-convert-messages-to-runtime-calls");
   }
 
+  if (Args.hasArg(options::OPT_fobjc_export_direct_method_wrappers))
+CmdArgs.push_back("-fobjc-export-direct-method-wrappers");
+
   // -fobjc-infer-related-result-type is the default, except in the Objective-C
   // rewriter.
   if (InferCovariantReturns)
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -2143,6 +2143,36 @@
   return false;
 }
 
+/// Create or look up the declaration of an objc_direct method wrapper.
+llvm::Function *getObjCDirectWrapper(llvm::Function ) {
+  std::string NewName = "__objc_direct_wrapper";
+  for (auto C : F.getName().str()) {
+if (C == '\1')
+  continue;
+NewName += C;
+  }
+
+  auto WI = llvm::find_if(*F.getParent(), [NewName](const llvm::Function ) {
+return F.getName() == NewName;
+  });
+
+  llvm::Function *Wrapper = nullptr;
+  if (WI == F.getParent()->end()) {
+llvm::Module  = *F.getParent();
+llvm::FunctionType *FnTy = F.getFunctionType();
+Wrapper = llvm::Function::Create(FnTy, F.getLinkage(), F.getAddressSpace(),
+ NewName);
+Wrapper->setLinkage(llvm::GlobalValue::ExternalLinkage);
+Wrapper->addFnAttr(llvm::Attribute::AlwaysInline);
+Wrapper->setVisibility(llvm::Function::DefaultVisibility);
+M.getFunctionList().insert(F.getIterator(), Wrapper);
+  } else {
+Wrapper = &*WI;
+  }
+
+  return Wrapper;
+}
+
 CodeGen::RValue
 CGObjCCommonMac::EmitMessageSend(CodeGen::CodeGenFunction ,
  ReturnValueSlot Return,
@@ -2214,7 +2244,13 @@
 
   llvm::FunctionCallee Fn = nullptr;
   if (Method && Method->isDirectMethod()) {
-Fn = GenerateDirectMethod(Method, Method->getClassInterface());
+if (CGM.getCodeGenOpts().ObjCExportDirectMethodWrappers) 

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi created this revision.
plotfi added reviewers: rjmccall, manmanren, MadCoder.
Herald added subscribers: cfe-commits, dang.
Herald added a project: clang.
plotfi requested review of this revision.

Hi @rjmccall @MadCoder

I'd like to preface this diff: I mostly want to discuss the prospects of 
exporting generated wrappers that call objc_direct methods, or alternately 
exporting the objc_direct methods themselves when an optional CodeGenOpts 
command-line flag is specified. This diff implements generating wrappers when a 
CodeGenOpts command-line flag is provided since C wrappers were mentioned in 
the original objc_direct diff. However I do think it might be possible to 
export the methods directly provided the implicit '_' underbar prefix is 
prepended to satisfy C naming on Darwin, and if thats the case I can post a 
diff that does that instead.  Anyways, thats all I wanted to preface with. 
Thanks.

Motivated by the potential benefit of using the objc_direct attribute, this 
diff aims to expand support to cross dylib objc_direct method calls by 
automatically generating exportable wrapper functions that call objc_direct 
methods internal to a dylib.

In the original patch landed in https://reviews.llvm.org/D69991 it mentions:

"The symbol for the method has enforced hidden visibility and such direct
calls are hence unreachable cross image. An explicit C function must be
made if so desired to wrap them."

This change provides a codegen options flag to clang 
`-fobjc-export-direct-method-wrappers` to generate the wrapper functions that 
begin with the prefix __objc_direct_wrapper and are marked as 
__attribute__((alwaysinline)). This way within a link unit the wrapper 
functions should be inlined away at their call sites, but across a dylib 
boundary the wrapper call is used.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86049

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGObjCMac.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp

Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1283,6 +1283,8 @@
 }
   }
 
+  if (Args.hasArg(OPT_fobjc_export_direct_method_wrappers))
+Opts.ObjCExportDirectMethodWrappers = 1;
 
   if (Args.hasArg(OPT_fno_objc_convert_messages_to_runtime_calls))
 Opts.ObjCConvertMessagesToRuntimeCalls = 0;
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3462,6 +3462,9 @@
   CmdArgs.push_back("-fno-objc-convert-messages-to-runtime-calls");
   }
 
+  if (Args.hasArg(options::OPT_fobjc_export_direct_method_wrappers))
+CmdArgs.push_back("-fobjc-export-direct-method-wrappers");
+
   // -fobjc-infer-related-result-type is the default, except in the Objective-C
   // rewriter.
   if (InferCovariantReturns)
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -2143,6 +2143,36 @@
   return false;
 }
 
+/// Create or look up the declaration of an objc_direct method wrapper.
+llvm::Function *getObjCDirectWrapper(llvm::Function ) {
+  std::string NewName = "__objc_direct_wrapper";
+  for (auto C : F.getName().str()) {
+if (C == '\1')
+  continue;
+NewName += C;
+  }
+
+  auto WI = llvm::find_if(*F.getParent(), [NewName](const llvm::Function ) {
+return F.getName() == NewName;
+  });
+
+  llvm::Function *Wrapper = nullptr;
+  if (WI == F.getParent()->end()) {
+llvm::Module  = *F.getParent();
+llvm::FunctionType *FnTy = F.getFunctionType();
+Wrapper = llvm::Function::Create(FnTy, F.getLinkage(), F.getAddressSpace(),
+ NewName);
+Wrapper->setLinkage(llvm::GlobalValue::ExternalLinkage);
+Wrapper->addFnAttr(llvm::Attribute::AlwaysInline);
+Wrapper->setVisibility(llvm::Function::DefaultVisibility);
+M.getFunctionList().insert(F.getIterator(), Wrapper);
+  } else {
+Wrapper = &*WI;
+  }
+
+  return Wrapper;
+}
+
 CodeGen::RValue
 CGObjCCommonMac::EmitMessageSend(CodeGen::CodeGenFunction ,
  ReturnValueSlot Return,
@@ -2214,7 +2244,13 @@
 
   llvm::FunctionCallee Fn = nullptr;
   if (Method && Method->isDirectMethod()) {
-Fn = GenerateDirectMethod(Method, Method->getClassInterface());
+if (CGM.getCodeGenOpts().ObjCExportDirectMethodWrappers) {
+  llvm::Function *FnDirect =
+GenerateDirectMethod(Method, Method->getClassInterface());
+  Fn = getObjCDirectWrapper(*FnDirect);
+} else {
+  Fn = GenerateDirectMethod(Method,