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

1209–1212

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

1214–1216

Thanks for matching the previous behavior 👌

1507

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.

1508

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

1212

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.

1499

Ditto

[mwyman]

Extracted the common new code into a helper function.

[ahatanak]

clang/lib/CodeGen/CGObjC.cpp

1117

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

1117

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

1125

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

[mwyman]

clang/lib/CodeGen/CGObjC.cpp

1125

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

1125

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

1125

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

[ahatanak]

clang/lib/CodeGen/CGObjC.cpp

1125

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

 #include "clang/AST/Attr.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/StmtObjC.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Analysis/ObjCARCUtil.h"
 #include "llvm/BinaryFormat/MachO.h"
+#include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/InlineAsm.h"
 using namespace clang;
 using namespace CodeGen;
 
 typedef llvm::PointerIntPair<llvm::Value*,1,bool> TryEmitResult;
 static TryEmitResult
 tryEmitARCRetainScalarExpr(CodeGenFunction &CGF, const Expr *e);
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);
 }
 
+// emitCmdValueForGetterSetterBody - Handle emitting the load necessary for
+// the `_cmd` selector argument for getter/setter bodies. For direct methods,
+// this returns an undefined/poison value; this matches behavior prior to `_cmd`
+// being removed from the direct method ABI as the getter/setter caller would
+// never load one. For non-direct methods, this emits a load of the implicit
+// `_cmd` storage.
+static llvm::Value *emitCmdValueForGetterSetterBody(CodeGenFunction &CGF,
+                                                   ObjCMethodDecl *MD) {
+  if (MD->isDirectMethod()) {
+    // Direct methods do not have a `_cmd` argument. Emit an undefined/poison
+    // value. This will be passed to objc_getProperty/objc_setProperty, which
+    // has not appeared bothered by the `_cmd` argument being undefined before.
+    llvm::Type *selType = CGF.ConvertType(CGF.getContext().getObjCSelType());
+    return llvm::PoisonValue::get(selType);
+  }
+
+  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 = emitCmdValueForGetterSetterBody(*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 = emitCmdValueForGetterSetterBody(*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 passes undef for the `cmd` argument.
+// CHECK-LABEL: define hidden i8* @"\01-[Root objectProperty]"(
+// CHECK-LABEL: objc_direct_method.cont:
+// CHECK-NEXT: [[SELFVAL:%.*]] = load {{.*}} %self.addr,
+// CHECK-NEXT: [[SELF:%.*]] = bitcast {{.*}} [[SELFVAL]] to i8*
+// CHECK-NEXT: [[IVAR:%.*]] = load {{.*}} @"OBJC_IVAR_$_Root._objectProperty",
+// CHECK-NEXT: call i8* @objc_getProperty(i8* noundef [[SELF]], i8* noundef poison, i64 noundef [[IVAR]], {{.*}})
+
 @interface Foo : Root {
   id __strong _cause_cxx_destruct;
 }
 @property(nonatomic, readonly, direct) int getDirect_setDynamic;
 @property(nonatomic, readonly) int getDynamic_setDirect;
 @end
 
 @interface Foo ()