[PATCH] D69991: Implement __attribute__((objc_direct)), __attribute__((objc_direct_members))

[David Chisnall via Phabricator via cfe-commits]

theraven added a comment.
Herald added a reviewer: aaron.ballman.
Herald added a reviewer: jdoerfert.

Sorry for getting to this late, I assumed it was a runtime-agnostic feature until someone filed a bug against the GNUstep runtime saying that we didn't implement it.  It would be polite to tag me in reviews for features that contain runtime-specific things.

The current implementation doesn't look as if it's runtime-specific.  Is there a reason that it's in CGObjCMacCommon rather than in the generic CGObjCRuntime class?

Looking at the code, it appears that it is correct only for objects created with `+alloc`.  Consider the following example:

  #import <Foundation/Foundation.h>
  #include <objc/runtime.h>
  
  @interface X : NSObject
  - (void)test __attribute__((objc_direct));
  @end
  
  @implementation X
  + (void)initialize
  {
  	printf("+initialize called on %p\n", self);
  }
  - (void)test
  {
  	printf("Test called on %p\n", self);
  }
  @end
  
  int main()
  {
  	@autoreleasepool
  	{
  		X *x = class_createInstance(objc_getClass("X"), 0);
  		[x test];
  	}
  }

I don't believe this does the right thing (unfortunately, the latest XCode requires Catalina and it breaks too many things for me to upgrade from Mojave, so I can't test it and see if there are any tweaks to the runtime).  The API guarantee for `class_createInstance` does not require it to call `+initialize`.  I can see a few ways of fixing this:

Either:

- Unconditionally do the `[self self]` dance on all direct methods (not ideal, since it's likely to offset the performance win) for runtimes without knowledge of direct functions and one out of:
  - Set a metadata flag on any class with direct methods to require `class_createInstance` (and any other allocation paths that I haven't thought of) in the runtime to call `+initialize` on instantiation.
  - Modify the API contract of `class_createInstance` to send `+initialize` unconditionally.
  - Provide an `objc_msgSendDirect` that takes the target function in place of the selector and checks that the receiver is initialise, teach the optimisers to inline through this in cases where they can prove that the receiver is already initialised.
- Document that direct methods do not trigger `+initialize`, generalise that to class methods and remove the existing `+self` dance.

For direct class methods, avoiding the `+self` dance might best avoided by using a construct like the C++ static initialiser for a pointer to the class at the call site (rather than a direct reference to the class), which guarantees that it's initialised in the callee and skipping this when the receiver is `self` in an method (`+initialize` is guaranteed to have been called on `self` on entry to any method in this case).  The pseudocode is something like (where `+direct` is a direct method):

The commit message for this refers to `objc_opt_self`.  This is presumably a fast-path for a `self` message send, but the only reference to that anywhere in the LLVM tree is in LLDB.  We don't appear to have any tests in clang for it and neither upstream nor Apple clang appear to generate calls for it, so I'm not sure what 'an appropriate deployment target' is in the commit message.


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

https://reviews.llvm.org/D69991