Fix assert in generated `direct` property getter/setters due to removal of `_cmd` parameter.

[mwyman]

Summary

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.

For direct property generated getters/setters, this now passes an undefined/uninitialized/poison value as the _cmd argument to objc_getProperty/objc_setProperty. Prior to removing the _cmd argument from the ABI of direct methods, it was left uninitialized/undefined; although references within hand-implemented methods would load the selector in the method prologue, generated getters/setters never did and just forwarded the undefined value that was passed as the argument.

This test keeps the generated code mostly similar to before, passing an uninitialized/undefined/poison value; for setters, the value argument may be moved to another register.

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

[stephanemoore]

clang/lib/CodeGen/CGObjC.cpp

1204–1207

I couldn't immediate tell if this TODO is still in the best position. I think it's fine?

clang/test/CodeGenObjC/direct-method.m

171–177

I'm not intricately familiar with the codegen checking mechanism in these tests but without fully knowing the details, this seems consistent with the checks in -[Root accessCmd] above which I would expect to require similar checks.

Did you check that these added tests reproduce the previous assertion/failure so that we can be confident that the code changes resolve the issue?

[mwyman]

Updated to eliminate loading the selector, as the prior implementation didn't pass a valid _cmd value (it just re-used the _cmd argument in the ABI, which was unset by the caller and so undefined in value).

[mwyman]

clang/test/CodeGenObjC/direct-method.m

171–177

Yes, this new test previously triggered the assertion.

[stephanemoore]

clang/lib/CodeGen/CGObjC.cpp

1209–1211

Thanks for matching the previous behavior 👌

1502

Previously this was using the default "" for the Name instead of "cmd".

I can't think of why it would intentionally want to use "". Using "cmd" is consistent with the codegen for the getter (now and before) so I don't disagree with using "cmd" for the name here.

1503

Strictly speaking, I think we can leave this line untouched now?

[plotfi]

@ahatanak how does this diff look to you?

[plotfi]

LGTM but waiting on Akira would be nice imho.

clang/lib/CodeGen/CGObjC.cpp

1207

Could this entire code sequence be moved to a helper or helper method? I think it could be good if the _cmd argument emission and the associated code comments were only written once.

1494

Ditto

[mwyman]

Extracted the common new code into a helper function.

[ahatanak]

clang/lib/CodeGen/CGObjC.cpp

1116

Since this is loading from an uninitialized alloca, can we just pass an undef to the call to objc_get/setProperty? The optimization passes will just do that, but we can still reduce the code size at -O0.

[mwyman]

Use explicit undef for the cmd parameter to objc_getProperty/objc_setProperty rather declaring and not initializing storage for the implicit _cmd.

[mwyman]

clang/lib/CodeGen/CGObjC.cpp

1116

Great suggestion! Done.

[ahatanak]

LGTM

clang/test/CodeGenObjC/direct-method.m

178

noundef means the value isn't undefined, right? Is it safe to use it with undef? We might want to fix this if it isn't.

[nlopes]

clang/lib/CodeGen/CGObjC.cpp

1124

Please consider using PoisonValue here instead (if appropriate). We are trying to get rid of undef.
Thank you!

[mwyman]

clang/lib/CodeGen/CGObjC.cpp

1124

A poison value is a result of an erroneous operation.

I could very well be misunderstanding, but based on this description from LangRef.html PoisonValue doesn't seem appropriate here; this is not "erroneous" behavior: it is just continuing the behavior prior to removing the _cmd implicit argument from the ABI, which was that the value was undef from the generated getter/setter's caller.

https://github.com/llvm/llvm-project/blob/49acab3f1408be9439a57833aa7b67678c9a05ba/clang/lib/CodeGen/CGObjCMac.cpp#L2142

Since this is (essentially) replicating that behavior, I'm not sure PoisonValue is what we want.

clang/test/CodeGenObjC/direct-method.m

178

It does feel questionable, however the pre-D131424 behavior was similar with the undef value coming from the caller and opaque to these generated getters/setters.

At least according to the OSS drop of libobjc, nothing in objc_getProperty/objc_setProperty actually references the _cmd argument so it seems safe-ish for now: https://github.com/apple-oss-distributions/objc4/blob/8701d5672d3fd3cd817aeb84db1077aafe1a1604/runtime/objc-accessors.mm#L48

FWIW, searching the code, I do see a handful of noundef undef in some other tests.

[nlopes]

clang/lib/CodeGen/CGObjC.cpp

1124

If the value is not used, then it can be poison.
If it gets used somehow then it depends on the callers. I don't know anything about ObjC to know what the right answer is here.
As we want to remove undef, the fewer the uses the better. Thank you!

[mwyman]

Updated to use PoisonValue rather than UndefValue.

[mwyman]

clang/lib/CodeGen/CGObjC.cpp

1124

Fair enough. I've changed to pass poison into the helper functions.

[ahatanak]

clang/lib/CodeGen/CGObjC.cpp

1124

It looks like _cmd is unused in both objc_getProperty and objc_setProperty, so I think this is fine.

https://github.com/opensource-apple/objc4/blob/master/runtime/objc-accessors.mm

clang/lib/CodeGen/CGObjC.cpp

static void emitCPPObjectAtomicGetterCall(CodeGenFunction &CGF,
   llvm::FunctionCallee copyCppAtomicObjectFn =
       CGF.CGM.getObjCRuntime().GetCppAtomicObjectGetFunction();
   CGCallee callee = CGCallee::forDirect(copyCppAtomicObjectFn);
   CGF.EmitCall(
       CGF.getTypes().arrangeBuiltinFunctionCall(CGF.getContext().VoidTy, args),
                callee, ReturnValueSlot(), args);
 }
 
+// emitCmdLoadForGetterSetterBody - Handle emitting local storage declarations
+// for the `_cmd` argument that no longer exists for direct methods.
+static llvm::Value *emitCmdLoadForGetterSetterBody(CodeGenFunction &CGF,
+                                                   ObjCMethodDecl *MD) {
+  if (MD->isDirectMethod()) {
+    // Direct methods no longer have a `_cmd` argument, so storage must be
+    // emitted for it to be passed to the property helper. Since the `_cmd`
+    // argument was never being initialized by the caller before, still pass
+    // an uninitialized/undefined value here.
+    CGF.EmitVarDecl(*MD->getCmdDecl());
+  }
+
+  return CGF.Builder.CreateLoad(CGF.GetAddrOfLocalVar(MD->getCmdDecl()), "cmd");
+}
+
 void
 CodeGenFunction::generateObjCGetterBody(const ObjCImplementationDecl *classImpl,
                                         const ObjCPropertyImplDecl *propImpl,
                                         const ObjCMethodDecl *GetterMethodDecl,
                                         llvm::Constant *AtomicHelperFn) {
   // If there's a non-trivial 'get' expression, we just have to emit that.
   if (!hasTrivialGetExpr(propImpl)) {
     if (!AtomicHelperFn) {
case PropertyImplStrategy::GetSetProperty: {
     llvm::FunctionCallee getPropertyFn =
         CGM.getObjCRuntime().GetPropertyGetFunction();
     if (!getPropertyFn) {
       CGM.ErrorUnsupported(propImpl, "Obj-C getter requiring atomic copy");
       return;
     }
     CGCallee callee = CGCallee::forDirect(getPropertyFn);
 
     // 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 = emitCmdLoadForGetterSetterBody(*this, getterMethod);
     llvm::Value *self = Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy);
     llvm::Value *ivarOffset =
       EmitIvarOffset(classImpl->getClassInterface(), ivar);
 
     CallArgList args;
     args.add(RValue::get(self), getContext().getObjCIdType());
     args.add(RValue::get(cmd), getContext().getObjCSelType());
     args.add(RValue::get(ivarOffset), getContext().getPointerDiffType());
     args.add(RValue::get(Builder.getInt1(strategy.isAtomic())),
              getContext().BoolTy);
 
     // FIXME: We shouldn't need to get the function info here, the
else {
       if (!setPropertyFn) {
         CGM.ErrorUnsupported(propImpl, "Obj-C setter requiring atomic copy");
         return;
       }
     }
 
     // Emit objc_setProperty((id) self, _cmd, offset, arg,
     //                       <is-atomic>, <is-copy>).
-    llvm::Value *cmd =
-      Builder.CreateLoad(GetAddrOfLocalVar(setterMethod->getCmdDecl()));
+    llvm::Value *cmd = emitCmdLoadForGetterSetterBody(*this, setterMethod);
     llvm::Value *self =
       Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy);
     llvm::Value *ivarOffset =
       EmitIvarOffset(classImpl->getClassInterface(), ivar);
     Address argAddr = GetAddrOfLocalVar(*setterMethod->param_begin());
     llvm::Value *arg = Builder.CreateLoad(argAddr, "arg");
     arg = Builder.CreateBitCast(arg, VoidPtrTy);
 
     CallArgList args;
     args.add(RValue::get(self), getContext().getObjCIdType());
     args.add(RValue::get(cmd), getContext().getObjCSelType());
     if (setOptimizedPropertyFn) {
       args.add(RValue::get(arg), getContext().getObjCIdType());
       args.add(RValue::get(ivarOffset), getContext().getPointerDiffType());
       CGCallee callee = CGCallee::forDirect(setOptimizedPropertyFn);
       EmitCall(getTypes().arrangeBuiltinFunctionCall(getContext().VoidTy, args),
                callee, ReturnValueSlot(), args);
     } else {
       args.add(RValue::get(ivarOffset), getContext().getPointerDiffType());

clang/test/CodeGenObjC/direct-method.m

 // RUN: %clang_cc1 -no-opaque-pointers -emit-llvm -fobjc-arc -triple x86_64-apple-darwin10 %s -o - | FileCheck %s
 
 struct my_complex_struct {
   int a, b;
 };
 
 struct my_aggregate_struct {
   int a, b;
   char buf[128];
 };
 
 __attribute__((objc_root_class))
 @interface Root
 - (int)getInt __attribute__((objc_direct));
 @property(direct, readonly) int intProperty;
 @property(direct, readonly) int intProperty2;
+@property(direct, readonly) id objectProperty;
 @end
 
 @implementation Root
 // CHECK-LABEL: define hidden i32 @"\01-[Root intProperty2]"
 - (int)intProperty2 {
   return 42;
 }
 
- (void)accessCmd __attribute__((objc_direct)) {
   // CHECK-NEXT: [[CMD1:%.*]] = load i8*, i8** @OBJC_SELECTOR_REFERENCES_
   // CHECK-NEXT: store i8* [[CMD1]], i8** [[CMDVAL]],
   SEL sel = _cmd;
 }
 
 @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: entry:
+// CHECK-NEXT: [[RETVAL:%.*]] = alloca i8*,
+// CHECK-NEXT: [[SELFADDR:%.*]] = alloca %0*,
+// CHECK-NEXT: [[CMDVAL:%_cmd]] = alloca i8*,
+// CHECK-LABEL: objc_direct_method.cont:
+// CHECK: [[CMD:%.*]] = load i8*, i8** [[CMDVAL]],
+// CHECK: call i8* @objc_getProperty({{.*}}, i8*{{.*}} [[CMD]], {{.*}})
+
 @interface Foo : Root {
   id __strong _cause_cxx_destruct;
 }
 @property(nonatomic, readonly, direct) int getDirect_setDynamic;
 @property(nonatomic, readonly) int getDynamic_setDirect;
 @end
 
 @interface Foo ()