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