This is an archive of the discontinued LLVM Phabricator instance.

[clang] Fix assert() crash when checking undeduced arg alignment
ClosedPublic

Authored by adamcz on Apr 16 2021, 11:29 AM.

Details

Summary

There already was a check for undeduced and incomplete types, but it
failed to trigger when outer type (SubstTemplateTypeParm in test) looked
fine, but inner type was not.

Diff Detail

Event Timeline

adamcz created this revision.Apr 16 2021, 11:29 AM
adamcz requested review of this revision.Apr 16 2021, 11:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2021, 11:29 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks for tracking it down, and sorry for the delay.

I thought the crash was fix in https://reviews.llvm.org/D99145. Thinking more about it, this is a recovery-expr crash, see my comment below.

clang/lib/Sema/SemaChecking.cpp
4543

The current fix is on top of kadir's fix, now the code becomes more add-hoc, it looks like a smell to me.

It looks like CheckArgAlignment has an implicit invariant (the arg type should never be undeduced), and this invariant was enforced by dropping the invalid AST. Now with RecoveryExpr, we might break the invariant -- when the RecoveryExpr preserves an undeduced type, like the test case in this patch (which is done via the code).

I think a right fix is to fix in the RecoveryExpr side, RecoveryExpr should not preserve an undeduced type, we did it for function return type already (https://reviews.llvm.org/D87350), but it didn't cover all places. I think a general fixing place is Sema::CreateRecoveryExpr, if the passing T is undeduced, fallback to dependent type.

adamcz updated this revision to Diff 340527.Apr 26 2021, 7:48 AM

Added a fix in CreateRecoveryExpr

clang/lib/Sema/SemaChecking.cpp
4543

Ooh, interesting. Thanks for the info.

I updated the change to catch this in CreateRecoveryExpr as well. I kept my original changes too, they shouldn't harm anything and it's better than crashing.

Let me know what you think about this now.

thanks, I think we can simplify a bit more.

clang/lib/Sema/SemaChecking.cpp
4543

I kept my original changes too, they shouldn't harm anything and it's better than crashing.

I'm a bit nervous about keeping the change/extending the undeduced-type here, this looks like a defensive programming, it might hide some bugs in the future. As we have figured out the root cause, I'd also remove the bailout isUndeducedType case here.

clang/lib/Sema/SemaExpr.cpp
19668

I'm not sure we need this well. I think the above T->isUndeducedType() should be enough to fix all known crashes. I'd just remove this one.

adamcz updated this revision to Diff 340802.Apr 27 2021, 5:23 AM
adamcz marked 2 inline comments as done.

review comments

adamcz added inline comments.Apr 27 2021, 5:49 AM
clang/lib/Sema/SemaExpr.cpp
19668

It's enough to fix all known cases, but I'm a little worried that we'll be playing whack-a-mole with bugs like this forever.

I think the fundamental problem is that a lot of functions have assumptions about state of their inputs and the flow is not sufficiently simple to be able to catch bugs like this in review. A little bit of defensive programming might be the best solution for the problem.
In this case, however, I'm not going to argue with you. Let's try this one more time. The updated fix is sufficient for all crashes we've seen so far.

hokein accepted this revision.Apr 27 2021, 11:03 AM

Thanks!

clang/test/SemaCXX/cxx17-undeduced-alignment.cpp
19 ↗(On Diff #340802)

The comment is stale now. Maybe add this test to the existing recovery-expr file test/SemaCXX/recovery-expr-type.cpp, as this is a recover-expr related crash (we also need to add the -std=c++17 flag to the first line of the file, I think it is fine).

This revision is now accepted and ready to land.Apr 27 2021, 11:03 AM
adamcz updated this revision to Diff 341244.Apr 28 2021, 9:41 AM

moved the test

hokein accepted this revision.Apr 29 2021, 12:39 AM

still LG

clang/test/SemaCXX/recovery-expr-type.cpp
29

oh, typeof is a GNU extension, I think -std=gnu++17 should suppress the new diagnostics.

This revision was landed with ongoing or failed builds.Apr 30 2021, 7:47 AM
This revision was automatically updated to reflect the committed changes.