[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

19669

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

19669

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

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

[adamcz]

moved the test

[hokein]

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.

[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())
+  if (ParamTy->isIncompleteType() || ArgTy->isIncompleteType())
     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/lib/Sema/SemaExpr.cpp

 ExprResult Sema::CreateRecoveryExpr(SourceLocation Begin, SourceLocation End,
                                     ArrayRef<Expr *> SubExprs, QualType T) {
   if (!Context.getLangOpts().RecoveryAST)
     return ExprError();
 
   if (isSFINAEContext())
     return ExprError();
 
-  if (T.isNull() || !Context.getLangOpts().RecoveryASTType)
+  if (T.isNull() || T->isUndeducedType() ||
+      !Context.getLangOpts().RecoveryASTType)
     // We don't know the concrete type, fallback to dependent type.
     T = Context.DependentTy;
+
   return RecoveryExpr::Create(Context, T, Begin, End, SubExprs);
 }

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

-// RUN: %clang_cc1 -triple=x86_64-unknown-unknown -frecovery-ast -frecovery-ast-type -o - %s -fsyntax-only -verify
+// RUN: %clang_cc1 -triple=x86_64-unknown-unknown -frecovery-ast -frecovery-ast-type -o - %s -std=gnu++17 -fsyntax-only -verify
 
 namespace test0 {
 struct Indestructible {
   // Indestructible();
   ~Indestructible() = delete; // expected-note 2{{deleted}}
 };
 Indestructible make_indestructible();
 

 // verify the "not an integral constant expression" diagnostic is suppressed.
 static_assert(1 == foo(1), ""); // expected-error {{no matching function}}
 }
 
 namespace test2 {
 void foo(); // expected-note 3{{requires 0 arguments}}
 void func() {
   // verify that "field has incomplete type" diagnostic is suppressed.
-  typeof(foo(42)) var; // expected-error {{no matching function}}
+  typeof(foo(42)) var; // expected-error {{no matching function}} \
 
   // FIXME: suppress the "cannot initialize a variable" diagnostic.
   int a = foo(1); // expected-error {{no matching function}} \
                   // expected-error {{cannot initialize a variable of type}}
 
   // FIXME: suppress the "invalid application" diagnostic.
   int s = sizeof(foo(42)); // expected-error {{no matching function}} \
                            // expected-error {{invalid application of 'sizeof'}}

 }
 
 namespace test10 {
 // Ensure we don't assert here.
 int f(); // expected-note {{candidate}}
 template<typename T> const int k = f(T()); // expected-error {{no matching function}}
 static_assert(k<int> == 1, ""); // expected-note {{instantiation of}}
 }
+
+namespace test11 {
+// Verify we do not assert()-fail here.
+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() {
+  bar(S(123)); // expected-error {{no matching conversion}}
+}
+} // namespace test11