Page MenuHomePhabricator

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