RFC: Implement optional exportable wrapper function generation for objc_direct methods.

[plotfi]

Summary

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.

[plotfi]

change for clang-tidy and clang-format.

[rjmccall]

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

[plotfi]

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.

[lanza]

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.

[plotfi]

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

[plotfi]

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

[plotfi]

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?

[rjmccall]

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.

[manmanren]

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

[dmaclach]

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

[plotfi]

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.

[mwyman]

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?

[plotfi]

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.

[plotfi]

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

[mwyman]

clang/include/clang/Basic/Attr.td

2251–2256 ↗

(On Diff #451041)

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 @propertys visible, so should there be a similar attribute for properties?

clang/lib/CodeGen/CGObjCMac.cpp

4025–4036 ↗

(On Diff #451041)

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

[plotfi]

Updated with some of @mwyman's feedback.

[plotfi]

clang/include/clang/Basic/Attr.td

2251–2256 ↗

(On Diff #451041)

I'd prefer to do @propertys 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).

clang/lib/CodeGen/CGObjCMac.cpp

4025–4036 ↗

(On Diff #451041)

I like this idea. I will look into doing it next.

[plotfi]

Updated to use mangleObjCMethodName in clang/lib/AST/Mangle.cpp to set the _+<a: > direct method name.

[plotfi]

clang/include/clang/Basic/Attr.td

2251–2256 ↗

(On Diff #451041)

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.

[dmaclach]

Thanks Puyan! Awesome to see this moving forward.

clang/test/CodeGenObjC/objc-direct-wrapper.m

31

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

36

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

[plotfi]

clang/test/CodeGenObjC/objc-direct-wrapper.m

31

Ah, sorry about that. I implemented the code to have one single attr but I accidentally copy pasted both attrs in the test.

[plotfi]

Update test.

[plotfi]

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

[plotfi]

clang/test/CodeGenObjC/objc-direct-wrapper.m

36

How does this test look for protocol?

[mwyman]

clang/include/clang/Basic/Attr.td

2251–2256 ↗

(On Diff #451041)

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.

[plotfi]

clang/include/clang/Basic/Attr.td

2251–2256 ↗

(On Diff #451041)

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.

[plotfi]

@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?

[plotfi]

@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?

[ahatanak]

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?

[plotfi]

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?

[plotfi]

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?

[plotfi]

clang/include/clang/Basic/Attr.td

2251–2256 ↗

(On Diff #451041)

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

[ahatanak]

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

[plotfi]

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

[plotfi]

Updated based on @ahatanak's feedback.

[plotfi]

@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?

[mwyman]

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.

[plotfi]

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?

[mwyman]

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?

[ahatanak]

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?

clang/lib/AST/Mangle.cpp

371

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?

[plotfi]

clang/lib/AST/Mangle.cpp

371

Yeah. These are for ld and dyld. Not having a preceding underscore and the square brackets causes problems.

[plotfi]

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 ?

clang/lib/AST/Mangle.cpp

371

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.

[plotfi]

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

[mwyman]

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?

[plotfi]

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

[ahatanak]

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?

[plotfi]

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

Edit: spoke to one of my colleagues and I believe dropping \01 is needed for things to work with dlsym. Dropping '[' is so that ld64 doesn't treat the symbol as "non-regular" which means it gets stripped.

clang/include/clang/AST/DeclObjC.h

public:
   bool hasSkippedBody() const { return ObjCMethodDeclBits.HasSkippedBody; }
   void setHasSkippedBody(bool Skipped = true) {
     ObjCMethodDeclBits.HasSkippedBody = Skipped;
   }
 
   /// True if the method is tagged as objc_direct
   bool isDirectMethod() const;
 
+  /// True if method is meant to be visible.
+  ///
+  /// TODO: For now this is only true for a very narrow case where the method
+  /// must be direct and must also be explicitly marked as
+  /// __attribute__((visibility("default"))). In the future it may be possible
+  /// to assume that all direct (or even both direct and indirect) methods are
+  /// visible but for the time being it is prudent to provide this default
+  /// visibility behavior as an opt-in only.
+  bool hasMethodVisibilityDefault() const {
+    return isDirectMethod() &&
+      getExplicitVisibility(VisibilityForValue)
+      .value_or(HiddenVisibility) == DefaultVisibility;
+  }
+
   /// True if the method has a parameter that's destroyed in the callee.
   bool hasParamDestroyedInCallee() const;
 
   /// Returns the property associated with this method's selector.
   ///
   /// Note that even if this particular method is not marked as a property
   /// accessor, it is still possible for it to match a property declared in a
   /// superclass. Pass \c false if you only want to check the current class.

clang/lib/AST/Mangle.cpp

for (unsigned slotIndex = 0,
       if (numArgs)
         OS << '_';
     }
 
     return;
   }
 
   // \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: +<ContainerName(CategoryName) SelectorName>
+  //       This will include a prefix _ on Darwin.
   if (includePrefixByte) {
     OS << '\01';
   }
-  OS << (MD->isInstanceMethod() ? '-' : '+') << '[';
+  OS << (MD->isInstanceMethod() ? '-' : '+');
+  OS << (MD->hasMethodVisibilityDefault() ? '<' : '[');
   if (const auto *CID = MD->getCategory()) {
     OS << CID->getClassInterface()->getName();
     if (includeCategoryNamespace) {
       OS << '(' << *CID << ')';
     }
   } else if (const auto *CD =
                  dyn_cast<ObjCContainerDecl>(MD->getDeclContext())) {
     OS << CD->getName();
   } else {
     llvm_unreachable("Unexpected ObjC method decl context");
   }
   OS << ' ';
   MD->getSelector().print(OS);
-  OS << ']';
+  OS << (MD->hasMethodVisibilityDefault() ? '>' : ']');
 }
 
 void MangleContext::mangleObjCMethodNameAsSourceName(const ObjCMethodDecl *MD,
                                                      raw_ostream &Out) {
   SmallString<64> Name;
   llvm::raw_svector_ostream OS(Name);
 
   mangleObjCMethodName(MD, OS, /*includePrefixByte=*/false,

clang/lib/CodeGen/CGObjC.cpp

void CodeGenFunction::StartObjCMethod(const ObjCMethodDecl *OMD,
   // Check if we should generate debug info for this method.
   if (OMD->hasAttr<NoDebugAttr>())
     DebugInfo = nullptr; // disable debug info indefinitely for this function
 
   llvm::Function *Fn = CGM.getObjCRuntime().GenerateMethod(OMD, CD);
 
   const CGFunctionInfo &FI = CGM.getTypes().arrangeObjCMethodDeclaration(OMD);
   if (OMD->isDirectMethod()) {
-    Fn->setVisibility(llvm::Function::HiddenVisibility);
+    Fn->setVisibility(OMD->hasMethodVisibilityDefault()
+                          ? llvm::Function::DefaultVisibility
+                          : llvm::Function::HiddenVisibility);
     CGM.SetLLVMFunctionAttributes(OMD, FI, Fn, /*IsThunk=*/false);
     CGM.SetLLVMFunctionAttributesForDefinition(OMD, Fn);
   } else {
     CGM.SetInternalFunctionAttributes(OMD, Fn, FI);
   }
 
   args.push_back(OMD->getSelfDecl());
   if (!OMD->isDirectMethod())

clang/lib/CodeGen/CGObjCRuntime.cpp

clang::CodeGen::emitObjCProtocolObject(CodeGenModule &CGM,
   return CGM.getObjCRuntime().GetOrEmitProtocol(protocol);
 }
 
 std::string CGObjCRuntime::getSymbolNameForMethod(const ObjCMethodDecl *OMD,
                                                   bool includeCategoryName) {
   std::string buffer;
   llvm::raw_string_ostream out(buffer);
   CGM.getCXXABI().getMangleContext().mangleObjCMethodName(OMD, out,
-                                       /*includePrefixByte=*/true,
+                                       /*includePrefixByte=*/
+                                       !OMD->hasMethodVisibilityDefault(),
                                        includeCategoryName);
   return buffer;
 }

clang/test/CodeGenObjC/objc-direct-wrapper.m

+// 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 _-<C testMethod:bar:>
+         // TODO: Fix this
+// CHECK-DEFAULT: t -[C testMethod:bar:]
+// CHECK-WRAPPER-IR-DEF: define {{(dso_local )?}}void @"-<C testMethod:bar:>"
+// CHECK-WRAPPER-IR-DECLARE: declare {{(dso_local )?}}void @"-<C testMethod:bar:>"
+// CHECK-PROTOCOL-DIRECT-FAIL: error: 'objc_direct' attribute cannot be applied to methods declared in an Objective-C protocol
+
+// CHECK-WRAPPER-INDIRECT-NOT: T _-<C testMethod:bar:>
+// CHECK-WRAPPER-IR-DEF-INDIRECT-NOT: define {{(dso_local )?}}void @"-<C testMethod:bar:>"
+// CHECK-WRAPPER-IR-DECLARE-INDIRECT-NOT: declare {{(dso_local )?}}void @"-<C testMethod:bar:>"
+
+#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];
+}