Page MenuHomePhabricator

No longer crash when a consteval function returns a structure
Needs ReviewPublic

Authored by aaron.ballman on Oct 12 2021, 12:19 PM.

Details

Summary

I don't think the aggregate emitter needs to emit anything when handling a constant expression which has an unused result. The unused result means we have nowhere to store the value anyway during codegen, so this early returns in that case.

This addresses PR51484.

Diff Detail

Event Timeline

aaron.ballman requested review of this revision.Oct 12 2021, 12:19 PM
aaron.ballman created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2021, 12:19 PM
erichkeane accepted this revision.Oct 26 2021, 12:02 PM
This revision is now accepted and ready to land.Oct 26 2021, 12:02 PM
rjmccall requested changes to this revision.Oct 26 2021, 4:31 PM

Hmm. Generally these cases are expected to handle the situation where there's no result slot passed in, which currently isn't exclusive to an ignored result (although you could argue it should be). IIRC we usually don't pass in a slot when evaluating an expression of agg type as anything except an initializer, e.g. when evaluating the E in E.member. The general fix is to call EnsureDest before accessing Dest. The only reason we wouldn't need to do that is if ConstantExpr is restricted in where it can appear such that that's impossible.

Skipping an unnecessary initialization does sound like a good optimization in general, but that's on top of the bug fix, not an alternative.

Separately, seeing this code makes me worried that it doesn't support the full set of constant initialization. We do have some kinds of constant initialization that can't be emitted as abstract initializers, e.g. address-diversified pointer auth. But we can deal with that as a separate patch.

This revision now requires changes to proceed.Oct 26 2021, 4:31 PM

Updated to use EnsureDest() based on review feedback.

Hmm. Generally these cases are expected to handle the situation where there's no result slot passed in, which currently isn't exclusive to an ignored result (although you could argue it should be). IIRC we usually don't pass in a slot when evaluating an expression of agg type as anything except an initializer, e.g. when evaluating the E in E.member. The general fix is to call EnsureDest before accessing Dest. The only reason we wouldn't need to do that is if ConstantExpr is restricted in where it can appear such that that's impossible.

Thank you for the suggestion to use EnsureDest, that worked like a charm.

Skipping an unnecessary initialization does sound like a good optimization in general, but that's on top of the bug fix, not an alternative.

Separately, seeing this code makes me worried that it doesn't support the full set of constant initialization. We do have some kinds of constant initialization that can't be emitted as abstract initializers, e.g. address-diversified pointer auth. But we can deal with that as a separate patch.

FWIW, there are actually quite a few crashes during codegen involving consteval support (https://bugs.llvm.org/buglist.cgi?quicksearch=consteval&list_id=224889 has quite a few of them), so I share your worries.

erichkeane accepted this revision.Nov 3 2021, 9:38 AM

Looks to accomplish this exactly the way that @rjmccall suggested!

Thank you for the review! I've commit in c524f1a0764da0c8d1775b2860dfc33901ca9c8f.

Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2022, 10:21 AM