[PATCH] D135091: Load the `_cmd` selector for generated getters/setters of `direct` Objective-C properties.

[Michael Wyman via Phabricator via cfe-commits]

mwyman created this revision.
mwyman added reviewers: ahatanak, plotfi, dmaclach.
mwyman added a project: clang.
Herald added a project: All.
mwyman requested review of this revision.
Herald added a subscriber: cfe-commits.

This fixes a bug from https://reviews.llvm.org/D131424 that removed the implicit `_cmd` parameter as an argument to `objc_direct` method implementations. In many cases the generated getter/setter will call `objc_getProperty` or `objc_setProperty`, both of which require the selector of the getter/setter; since `_cmd` didn't automatically have backing storage, attempting to load the address asserted.

This adds an explicit load of the selector, similar to if it's referenced in an `objc_direct` method body; since the getter/setter is entirely synthesized, there's no reason to preemptively load the selector and store it locally, so the value is directly loaded and no additional storage is created for it.

Added a test that triggers the assert prior to the implementation code.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135091

Files:
  clang/lib/CodeGen/CGObjC.cpp
  clang/test/CodeGenObjC/direct-method.m


Index: clang/test/CodeGenObjC/direct-method.m
===================================================================
--- clang/test/CodeGenObjC/direct-method.m
+++ clang/test/CodeGenObjC/direct-method.m
@@ -14,6 +14,7 @@
 - (int)getInt __attribute__((objc_direct));
 @property(direct, readonly) int intProperty;
 @property(direct, readonly) int intProperty2;
+ at property(direct, readonly) id objectProperty;
 @end
 
 @implementation Root
@@ -167,6 +168,14 @@
 @end
 // CHECK-LABEL: define hidden i32 @"\01-[Root intProperty]"
 
+// Check the synthesized objectProperty calls objc_getProperty(); this also
+// checks that the synthesized method accesses _cmd (or rather loads the
+// selector, as it is an argument to objc_getProperty).
+// CHECK-LABEL: define hidden i8* @"\01-[Root objectProperty]"(
+// CHECK-LABEL: objc_direct_method.cont:
+// CHECK-NEXT: load i8*, i8** @OBJC_SELECTOR_REFERENCES_
+// CHECK: call i8* @objc_getProperty({{.*}})
+
 @interface Foo : Root {
   id __strong _cause_cxx_destruct;
 }
Index: clang/lib/CodeGen/CGObjC.cpp
===================================================================
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -1189,8 +1189,17 @@
     // Return (ivar-type) objc_getProperty((id) self, _cmd, offset, true).
     // FIXME: Can't this be simpler? This might even be worse than the
     // corresponding gcc code.
-    llvm::Value *cmd =
-      Builder.CreateLoad(GetAddrOfLocalVar(getterMethod->getCmdDecl()), "cmd");
+    llvm::Value *cmd;
+    if (getterMethod->isDirectMethod()) {
+      // Direct methods do not have the `_cmd` parameter; the selector must be
+      // loaded explicitly. Since it is not otherwise used in the getter, there
+      // is no reason to create a local variable for it.
+      cmd =
+          CGM.getObjCRuntime().GetSelector(*this, getterMethod->getSelector());
+    } else {
+      cmd = Builder.CreateLoad(GetAddrOfLocalVar(getterMethod->getCmdDecl()),
+                               "cmd");
+    }
     llvm::Value *self = Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy);
     llvm::Value *ivarOffset =
       EmitIvarOffset(classImpl->getClassInterface(), ivar);
@@ -1475,10 +1484,18 @@
 
     // Emit objc_setProperty((id) self, _cmd, offset, arg,
     //                       <is-atomic>, <is-copy>).
-    llvm::Value *cmd =
-      Builder.CreateLoad(GetAddrOfLocalVar(setterMethod->getCmdDecl()));
-    llvm::Value *self =
-      Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy);
+    llvm::Value *cmd;
+    if (setterMethod->isDirectMethod()) {
+      // Direct methods do not have the `_cmd` parameter; the selector must be
+      // loaded explicitly. Since it is not otherwise used in the setter, there
+      // is no reason to create a local variable for it.
+      cmd =
+          CGM.getObjCRuntime().GetSelector(*this, setterMethod->getSelector());
+    } else {
+      cmd = Builder.CreateLoad(GetAddrOfLocalVar(setterMethod->getCmdDecl()),
+                               "cmd");
+    }
+    llvm::Value *self = Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy);
     llvm::Value *ivarOffset =
       EmitIvarOffset(classImpl->getClassInterface(), ivar);
     Address argAddr = GetAddrOfLocalVar(*setterMethod->param_begin());


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D135091.464747.patch
Type: text/x-patch
Size: 3252 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20221003/9c273d03/attachment.bin>