Page MenuHomePhabricator

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

Authored by mwyman on Oct 3 2022, 11:15 AM.

Details

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.

Diff Detail

Event Timeline

mwyman created this revision.Oct 3 2022, 11:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2022, 11:15 AM
mwyman requested review of this revision.Oct 3 2022, 11:15 AM
stephanemoore accepted this revision.Oct 6 2022, 1:24 PM
stephanemoore added inline comments.
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?

This revision is now accepted and ready to land.Oct 6 2022, 1:24 PM
mwyman updated this revision to Diff 465857.Oct 6 2022, 1:33 PM
mwyman retitled this revision from Load the `_cmd` selector for generated getters/setters of `direct` Objective-C properties. to Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties..
mwyman edited the summary of this revision. (Show Details)

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 marked an inline comment as done.Oct 6 2022, 1:34 PM
mwyman added inline comments.
clang/test/CodeGenObjC/direct-method.m
171–177

Yes, this new test previously triggered the assertion.

stephanemoore accepted this revision.Oct 6 2022, 2:18 PM
stephanemoore added inline comments.
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 added a comment.Oct 6 2022, 2:42 PM

@ahatanak how does this diff look to you?

plotfi added a comment.Oct 6 2022, 2:49 PM

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 updated this revision to Diff 465896.Oct 6 2022, 3:15 PM
mwyman marked an inline comment as done.

Extracted the common new code into a helper function.

mwyman marked an inline comment as done.Oct 6 2022, 3:15 PM
ahatanak added inline comments.Oct 6 2022, 3:56 PM
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 updated this revision to Diff 465925.Oct 6 2022, 4:36 PM

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

mwyman marked 2 inline comments as done.Oct 6 2022, 4:37 PM
mwyman added inline comments.
clang/lib/CodeGen/CGObjC.cpp
1117

Great suggestion! Done.

mwyman marked an inline comment as done.Oct 6 2022, 4:37 PM
ahatanak accepted this revision.Oct 6 2022, 5:24 PM

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 added inline comments.Oct 6 2022, 11:26 PM
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 added inline comments.Oct 7 2022, 10:36 AM
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 added inline comments.Oct 7 2022, 2:12 PM
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 this revision to Diff 466212.Oct 7 2022, 4:03 PM
mwyman edited the summary of this revision. (Show Details)

Updated to use PoisonValue rather than UndefValue.

mwyman marked 2 inline comments as done.Oct 7 2022, 4:04 PM
mwyman added inline comments.
clang/lib/CodeGen/CGObjC.cpp
1125

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

mwyman marked an inline comment as done.Oct 7 2022, 4:04 PM
mwyman marked an inline comment as done.
mwyman marked an inline comment as done.
ahatanak added inline comments.Oct 7 2022, 4:33 PM
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

mwyman retitled this revision from Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties. to Fix assert in generated `direct` property getter/setters due to removal of `_cmd` parameter..Oct 10 2022, 11:41 AM
mwyman marked an inline comment as done.
mwyman updated this revision to Diff 466602.Oct 10 2022, 1:26 PM
This revision was landed with ongoing or failed builds.Oct 11 2022, 9:16 PM
This revision was automatically updated to reflect the committed changes.