[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.

[Michael Wyman via Phabricator via cfe-commits]

mwyman added inline comments.


================
Comment at: clang/lib/CodeGen/CGObjC.cpp:1125
+    llvm::Type *selType = CGF.ConvertType(CGF.getContext().getObjCSelType());
+    return llvm::UndefValue::get(selType);
+  }
----------------
nlopes wrote:
> Please consider using PoisonValue here instead (if appropriate). We are trying to get rid of undef.
> Thank you!
> 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.


================
Comment at: clang/test/CodeGenObjC/direct-method.m:178
+// CHECK-NEXT: [[IVAR:%.*]] = load {{.*}} @"OBJC_IVAR_$_Root._objectProperty",
+// CHECK-NEXT: call i8* @objc_getProperty(i8* noundef [[SELF]], i8* noundef undef, i64 noundef [[IVAR]], {{.*}})
+
----------------
ahatanak wrote:
> `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.
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.


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

https://reviews.llvm.org/D135091