[clang] Make CXXRecrdDecl invalid if it contains any undeduced fields.

[adamcz]

Summary

Undeduced fields in a valid record cause crashes when trying to obtain
the size and/or layout of the record.

[hokein]

clang/test/SemaCXX/cxx11-crashes.cpp

117

I think the root cause is that this incomplete var declaration is marked valid.

Looks like there are a few inconsistencies in clang:

struct Bar {
  static constexpr auto A1; // case1: invalid
  static constexpr auto A2 = ; // case2: valid
};

// static var decl in global context
static constexpr auto A3; // case3: invalid
static constexpr auto A4 = ; // case4: invalid

so it looks like marking case2 valid is a bug, I think we should invalidate it.

[adamcz]

changed to marking field type as invalid

[adamcz]

clang/test/SemaCXX/cxx11-crashes.cpp

117

I updated the change to mark a FieldDecl with undeduced type as invalid, thus making the record invalid too. Also added a test using -ast-dump to verify that both field and record are invalid, but kept the crash test too.

[hokein]

clang/test/SemaCXX/cxx11-crashes.cpp

117

sorry, my comment was not clear enough -- if I understand your new change correctly, the change makes the field decl Foo<decltype(A)>::type B; but not the decl A.

The problem here is static constexpr auto A = ;, this declaration (var, not field) is valid, which causes the field decl below Foo<decltype(A)>::type B; valid. If we invalidate the Decl A, then the filed decl B should be invalidated automatically.

leaving A valid is problematic -- we can't have a valid decl with an undeduced auto type in clang. the following case would lead another crash:

struct Bar {
  static constexpr auto A = ;
};
constexpr int s = sizeof(Bar::A);

So the solution is to make Decl A invalid.

[adamcz]

Fix a more generic case of invalid static initializer

[adamcz]

OK, I think we've got it now :-)

I kept the original crash test in the change, but I'm not sure if it's appropriate anymore. Let me know if you think I should remove it.

[hokein]

In D105478#2867120, @adamcz wrote:

OK, I think we've got it now :-)

I kept the original crash test in the change, but I'm not sure if it's appropriate anymore. Let me know if you think I should remove it.

Thanks, that sounds good to me, the original crash test is valuable, worth to keep it.

clang/lib/Parse/ParseDeclCXX.cpp

2994

it seems to me calling Sema::ActOnInitializerError(ThisDecl) is a better fit, rather than invalidating the decl once the initializer is failed to parse, for example static const int Member = ; we want to keep the decl valid.

[adamcz]

ActOnInializerError instead of SetInvalidDecl

clang/lib/Parse/ParseDeclCXX.cpp

2994

Amusingly, this change led me to discover https://bugs.llvm.org/show_bug.cgi?id=36064, which seems like the same issue, only solved for a very specific example. I had to update the test for that, since now we solve it more generically and thus error messages are different.

[hokein]

Thanks!

clang/lib/Parse/ParseDeclCXX.cpp

if (HasInClassInit != ICIS_NoInit) {
           ThisDecl->setInvalidDecl();
       } else
         ParseCXXNonStaticMemberInitializer(ThisDecl);
     } else if (HasStaticInitializer) {
       // Normal initializer.
       ExprResult Init = ParseCXXMemberInitializer(
           ThisDecl, DeclaratorInfo.isDeclarationOfFunction(), EqualLoc);
 
-      if (Init.isInvalid())
+      if (Init.isInvalid()) {
+        if (ThisDecl)
+          Actions.ActOnUninitializedDecl(ThisDecl);
         SkipUntil(tok::comma, StopAtSemi | StopBeforeMatch);
-      else if (ThisDecl)
+      } else if (ThisDecl)
         Actions.AddInitializerToDecl(ThisDecl, Init.get(), EqualLoc.isInvalid());
     } else if (ThisDecl && DS.getStorageClassSpec() == DeclSpec::SCS_static)
       // No initializer.
       Actions.ActOnUninitializedDecl(ThisDecl);
 
     if (ThisDecl) {
       if (!ThisDecl->isInvalidDecl()) {
         // Set the Decl for any late parsed attributes

clang/test/AST/ast-dump-undeduced-expr.cpp

+// RUN: not %clang_cc1 -triple x86_64-unknown-unknown -ast-dump %s | FileCheck %s
+
+struct Foo {
+  static constexpr auto Bar = ;
+};
+
+// CHECK: -VarDecl {{.*}} invalid Bar 'const auto' static constexpr

clang/test/SemaCXX/crash-auto-36064.cpp

 // RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify
-template <typename A, decltype(new A)> // expected-error{{new expression for type 'auto' requires a constructor argument}}
+template <typename A, decltype(new A)>
 struct b;
 struct d {
   static auto c = ;              // expected-error{{expected expression}}
+                                 // expected-error@-1 {{declaration of variable 'c' with deduced type 'auto' requires an initializer}}
+
   decltype(b<decltype(c), int>); // expected-error{{expected '(' for function-style cast or type construction}}
-                                 // expected-note@-1{{while substituting prior template arguments into non-type template parameter [with A = auto]}}
 };

clang/test/SemaCXX/cxx11-crashes.cpp

namespace pr29091 {
   bool foo() { return __has_nothrow_constructor(Y); }
   bool bar() { return __has_nothrow_copy(Y); }
 
   struct A { template <typename T> A(); };
   struct B : A { using A::A; };
   bool baz() { return __has_nothrow_constructor(B); }
   bool qux() { return __has_nothrow_copy(B); }
 }
+
+namespace undeduced_field {
+template<class T>
+struct Foo {
+  typedef T type;
+};
+
+struct Bar {
+  Bar();
+  // The missing expression makes A undeduced.
+  static constexpr auto A = ;  // expected-error {{expected expression}}
+                               // expected-error@-1 {{declaration of variable 'A' with deduced type 'const auto' requires an initializer}}
+
+  Foo<decltype(A)>::type B;  // The type of B is also undeduced (wrapped in Elaborated).
+};
+
+// This used to crash when trying to get the layout of B.
+Bar x;
+}