This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by plotfi on Aug 17 2020, 12:40 AM.

Details

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.

Diff Detail

Event Timeline

plotfi created this revision.Aug 17 2020, 12:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2020, 12:40 AM
plotfi requested review of this revision.Aug 17 2020, 12:40 AM
plotfi updated this revision to Diff 285925.
plotfi updated this revision to Diff 286141.Aug 17 2020, 1:59 PM

change for clang-tidy and clang-format.

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

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 added a comment.Aug 19 2020, 9:50 AM

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.

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

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 updated this revision to Diff 289616.Sep 2 2020, 6:18 PM

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 added a comment.Sep 3 2020, 7:39 PM

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?

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.

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

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

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 added a subscriber: mwyman.Jul 19 2022, 12:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2022, 12:32 PM
Herald added a subscriber: MaskRay. · View Herald Transcript

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?

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 updated this revision to Diff 451041.Aug 8 2022, 10:10 PM

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 added inline comments.Aug 9 2022, 10:16 AM
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
4111–4122

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

plotfi updated this revision to Diff 459257.Sep 9 2022, 10:42 PM

Updated with some of @mwyman's feedback.

plotfi added inline comments.Sep 9 2022, 10:46 PM
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
4111–4122

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

plotfi updated this revision to Diff 459260.Sep 9 2022, 11:06 PM

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

plotfi marked an inline comment as done.Sep 9 2022, 11:09 PM
plotfi added inline comments.
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.

plotfi updated this revision to Diff 459261.Sep 9 2022, 11:10 PM

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 added inline comments.Sep 10 2022, 12:25 AM
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 updated this revision to Diff 459267.Sep 10 2022, 12:26 AM

Update test.

plotfi marked an inline comment as done.Sep 10 2022, 12:26 AM
plotfi updated this revision to Diff 459268.Sep 10 2022, 12:38 AM

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

plotfi marked an inline comment as done.Sep 10 2022, 12:38 AM
plotfi added inline comments.
clang/test/CodeGenObjC/objc-direct-wrapper.m
36

How does this test look for protocol?

mwyman added inline comments.Sep 12 2022, 3:23 PM
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 marked an inline comment as done.Sep 13 2022, 12:29 PM
plotfi added inline comments.
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.

@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 added a comment.EditedSep 21 2022, 1:45 PM

@ahatanak @rjmccall

I think the decision tree goes

  1. Do we change the existing visibility behavior of objc methods? Yes / No
  1. If we leave hidden as the default do we change the behavior for objc_direct? Yes / No
  1. 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?

  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?

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

  1. 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?

  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?

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

  1. 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?

  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?

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

  1. 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 added inline comments.Sep 23 2022, 6:48 PM
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

  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?

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

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

  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?

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

  1. 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 this revision to Diff 462944.Sep 26 2022, 9:36 AM

Updated based on @ahatanak's feedback.

plotfi updated this revision to Diff 462999.Sep 26 2022, 12:47 PM
plotfi marked 2 inline comments as done.

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

plotfi updated this revision to Diff 463064.Sep 26 2022, 6:03 PM

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

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?

@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 ↗(On Diff #463064)

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 added inline comments.Sep 27 2022, 12:40 PM
clang/lib/AST/Mangle.cpp
371 ↗(On Diff #463064)

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

@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 ↗(On Diff #463064)

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.

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

@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 added a comment.EditedSep 28 2022, 8:57 AM

@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 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 added a comment.EditedSep 28 2022, 10:18 AM

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

plotfi marked an inline comment as done.Sep 28 2022, 1:43 PM