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

[adamcz]

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.

[hokein]

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]

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.

[hokein]

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 ↗

(On Diff #340527)

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]

review comments

[adamcz]

clang/lib/Sema/SemaExpr.cpp

19668 ↗

(On Diff #340527)

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]

Thanks!

clang/test/SemaCXX/cxx17-undeduced-alignment.cpp

20

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).

[adamcz]

moved the test

[hokein]

still LG

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

29 ↗

(On Diff #341244)

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

[adamcz]

typeof

clang/lib/Sema/SemaChecking.cpp

void Sema::CheckArgAlignment(SourceLocation Loc, NamedDecl *FDecl,
   if (ParamTy->isPointerType())
     ArgTy = ArgTy->getPointeeType();
 
   // Remove reference or pointer
   ParamTy = ParamTy->getPointeeType();
 
   // Find expected alignment, and the actual alignment of the passed object.
   // getTypeAlignInChars requires complete types
-  if (ParamTy->isIncompleteType() || ArgTy->isIncompleteType() ||
-      ParamTy->isUndeducedType() || ArgTy->isUndeducedType())
+  auto CheckTypeOK = [](QualType Ty) {
+    if (Ty->isIncompleteType() || Ty->isUndeducedType())
+      return false;
+    if (auto *DesugaredTy = Ty->getUnqualifiedDesugaredType())
+      if (DesugaredTy->isIncompleteType() || DesugaredTy->isUndeducedType())
+        return false;
+    return true;
+  };
+  if (!CheckTypeOK(ParamTy) || !CheckTypeOK(ArgTy))
     return;
 
   CharUnits ParamAlign = Context.getTypeAlignInChars(ParamTy);
   CharUnits ArgAlign = Context.getTypeAlignInChars(ArgTy);
 
   // If the argument is less aligned than the parameter, there is a
   // potential alignment issue.
   if (ArgAlign < ParamAlign)

clang/test/SemaCXX/cxx17-undeduced-alignment.cpp

+// RUN: %clang_cc1 -std=c++17 -verify %s
+//
+// Verifies that clang no longer crashes on undeduced, sugared types.
+
+template <class T> void foo(T &t);
+template <typename T>
+void bar(T t) {
+  foo(t);
+}
+
+template <typename T = void *>
+struct S { // expected-note {{candidate}}
+  S(T t);  // expected-note {{candidate}}
+  ~S();
+};
+template <typename T> S(T t) -> S<void *>;
+
+void baz() {
+  // S(123) is undeduced, but when passed to foo() via bar() it is wrapped in
+  // SubstTemplateTypeParm, for which isUndeduced() is false.
+  bar(S(123)); // expected-error {{no matching conversion}}
+}