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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3905
+categories, or extensions (for the latter two it applies the attribute
+to all methods declared in the category/implementation),
+
----------------
Editing error?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3909
+called directly. It can also be used on Objective-C Categories and Extensions,
+in which case it applies to all methods declared in such an ``@interface``.
+
----------------
If this is still true (rather than being done with `objc_direct_members`),
I feel it should be documented next to the statement at the end about
`objc_direct_members` in order to make the contrasting use cases clear.
But we should discuss whether this should be using `objc_direct_members`
for that use case.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3912
+Properties can also be declared with the ``direct`` property attribute
+which causes the property getter and setter to be direct methods as well.
+
----------------
"If an Objective-C property is declared with the ``direct`` property attribute, then its getter and setter are declared to be direct unless they are explicitly declared."

I assume that's correct w.r.t the treatment of explicit declarations of getters and setters?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3918
+
+Some sematics of the message send are however preserved:
+
----------------
A message send to a direct method calls the implementation directly, as if it were a C function, rather than using ordinary Objective-C method dispatch. This is substantially faster and potentially allows the implementation to be inlined, but it also means the method cannot be overridden in subclasses or replaced dynamically, as ordinary Objective-C methods can.

Furthermore, a direct method is not listed in the class's method lists.  This substantially reduces the code-size overhead of the method but also means it cannot be called dynamically using ordinary Objective-C method dispatch at all; in particular, this means that it cannot override a superclass method or satisfy a protocol requirement.

Clang will diagnose attempts to override direct methods, override non-direct methods with direct methods, implement a protocol requirement with a direct method, or use a selector that is only known to be used for direct methods.

Symbols for direct method implementations are implicitly given hidden visibility, meaning that they can only be called within the same linkage unit.

Although a direct method is called directly as if it were a C function, it still obeys Objective-C semantics in other ways:

- If the receiver is `nil`, the call does nothing and returns the zero value for the return type.

- Calling a direct class method will cause the class to be initialized, including calling the `+initialize` method if present.

- The method still receives an implicit `_cmd` argument containing its selector.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3938
+and causes any method that has not been declared in any ``@interface``
+or ``@protocol`` this class conforms to to be direct methods.
+  }];
----------------
"decorage"

I think you could be more precise about "has not been declared in any ``@interface`` or ``@protocol``".  It's specifically the interfaces, class extensions, categories, and protocols of this class and its superclasses.

This should refer the user to the documentation about the ``objc_direct`` attribute.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:994
+def err_objc_direct_missing_on_decl : Error<
+  "method implementation is direct but its declaration was not">;
+def err_objc_direct_on_override : Error<
----------------
"direct method implementation was previously declared not direct"


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:996
+def err_objc_direct_on_override : Error<
+  "method overrides or implementing protocol conformance cannot be direct">;
+def err_objc_override_direct_method : Error<
----------------
"methods that %select{override superclass methods|implement protocol requirements}0 cannot be direct"


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:998
+def err_objc_override_direct_method : Error<
+  "cannot override direct methods">;
+
----------------
"cannot override a method that is declared direct by a superclass"


================
Comment at: clang/lib/CodeGen/CGObjC.cpp:689
+    // This function is a direct call, it has to implement a nil check
+    // on entry.
+    CGM.getObjCRuntime().GenerateDirectMethodPrologue(*this, Fn, OMD, CD);
----------------
Please add a TODO about the idea of emitting separate entry points that elide the check.


================
Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:3885
+{
+  // GNU runtime doesn't support direct calls at this time
+}
----------------
This doesn't seem to be diagnosed in Sema.


================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:1588
     // interface, we cannot perform this check.
+	//
+	// Note that for direct methods, because objc_msgSend is skipped,
----------------
MadCoder wrote:
> doh I'll fix the tabs
The indentation still seems wrong.


================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:2169
+    // direct methods will synthesize the proper _cmd so just don't bother
+    // with the argument
+    assert(!IsSuper);
----------------
We generally try to write comments as proper sentences, or at least start with a capital letter. :)


================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:2219
+  if (Method && Method->isDirectMethod()) {
+    Fn = GenerateDirectMethod(Method, Method->getClassInterface());
+  } else if (CGM.ReturnSlotInterferesWithArgs(MSI.CallInfo)) {
----------------
I'm not a fan of this method name, but it's following an existing pattern, so I'll cope.


================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:4075
+
+  if (OMD->isClassMethod()) {
+    const ObjCInterfaceDecl *OID = dyn_cast<ObjCInterfaceDecl>(CD);
----------------
MadCoder wrote:
> I suspect this function requires synthesizing some debug metadata but I have absolutely no idea how and what ;)
I'm sure Adrian has thoughts about that.


================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:4076
+  if (OMD->isClassMethod()) {
+    const ObjCInterfaceDecl *OID = dyn_cast<ObjCInterfaceDecl>(CD);
+    Selector SelfSel = GetNullarySelector("self", CGM.getContext());
----------------
When would the method container ever be an interface?  This is running on a method implementation; it should always be within an `ObjCImplDecl`.  (There was a case where this wasn't true for accessors, but Adrian just fixed that; regardless, you can definitely pass down an `ObjCImplDecl`.). From the `ObjCImplDecl` you can extract the class interface, which can never be null.


================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:4093
+                                   SelfSel, selfValue, Args, OID, nullptr);
+    }
+    Builder.CreateStore(result.getScalarVal(),
----------------
Please just extract a little helper that does this try-generate-specialized-else-generate-normally dance out of `EmitObjCMessageExpr`.


================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:4095
+    Builder.CreateStore(result.getScalarVal(),
+                        CGF.GetAddrOfLocalVar(OMD->getSelfDecl()));
+
----------------
We should also be changing `selfValue` so that the null check below will do the right thing with e.g. subclasses of weak-linked classes, where IIUC the input value might be non-null but the initialized class pointer might be null.


================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:4097
+
+    ReceiverCanBeNull = !OID || isWeakLinkedClass(OID);
+  }
----------------
We should always be able to find *some* OID.


================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:6719
   for (const auto *MD : OCD->methods()) {
-    if (MD->isInstanceMethod()) {
-      instanceMethods.push_back(MD);
-    } else {
-      classMethods.push_back(MD);
+    if (!MD->isDirectMethod()) {
+      if (MD->isInstanceMethod()) {
----------------
I would generally prefer `if (MD->isDirectMethod()) continue;` for all of these loops.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69991