[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 <https://reviews.llvm.org/D86049#3818923>, @ahatanak wrote:

> In D86049#3818696 <https://reviews.llvm.org/D86049#3818696>, @plotfi wrote:
>
>> In D86049#3818435 <https://reviews.llvm.org/D86049#3818435>, @mwyman wrote:
>>
>>> In D86049#3816006 <https://reviews.llvm.org/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