Problematic line in Clang source code:

 125│   // Marking a DecompositionDecl as invalid implies all the child BindingDecl's
 126│   // are invalid too.
 127│   if (DecompositionDecl *DD = dyn_cast<DecompositionDecl>(this)) {
 128│     for (BindingDecl *Binding : DD->bindings()) {
 129├>      Binding->setInvalidDecl();
 130│     }
 131│   }
 132│ }

=> Binding is nullptr


(gdb) frame 1
#1  0x00007fffc52a8fad in clang::Decl::setInvalidDecl (this=<optimized out>, Invalid=<optimized out>) at /home/kfunk/devel/src/llvm/tools/clang/lib/AST/DeclBase.cpp:129

I know it's unfortunate that I cannot give precise instructions how to reproduce this crash using Clang/LLVM command-line utilities. But I filed this bug report anyway, maybe someone can give me a hint how to reproduce this without external helpers.

Crash is still happening even on non-managed C++ (project uses C++17 standard).

#0  clang::Decl::setInvalidDecl(bool) (this=0x0, Invalid=true) at clang/lib/AST/DeclBase.cpp:132
#1  0x00007fffb92cdf9c in clang::Decl::setInvalidDecl(bool) (this=this@entry=0x7fff76c86940, Invalid=<optimized out>)
    at clang/lib/AST/DeclBase.cpp:149
#2  0x00007fffba010bb4 in clang::ASTDeclReader::VisitDecl(clang::Decl*) (this=0x7fff927f78c0, D=0x7fff76c86940)
    at /usr/lib/llvm/9/include/llvm/ADT/SmallVector.h:148
#3  0x00007fffba011a41 in clang::ASTDeclReader::VisitNamedDecl(clang::NamedDecl*) (this=this@entry=0x7fff927f78c0, ND=ND@entry=0x7fff76c86940)
    at clang/lib/Serialization/ASTReaderDecl.cpp:657
#4  0x00007fffba011c01 in clang::ASTDeclReader::VisitValueDecl(clang::ValueDecl*) (this=0x7fff927f78c0, VD=0x7fff76c86940)
    at clang/lib/Serialization/ASTReaderDecl.cpp:806
#5  0x00007fffba011c99 in clang::ASTDeclReader::VisitDeclaratorDecl(clang::DeclaratorDecl*) (this=this@entry=0x7fff927f78c0, DD=DD@entry=0x7fff76c86940)
    at clang/lib/Serialization/ASTReaderDecl.cpp:825
#6  0x00007fffba03161c in clang::ASTDeclReader::VisitVarDeclImpl(clang::VarDecl*) (this=this@entry=0x7fff927f78c0, VD=VD@entry=0x7fff76c86940)
    at clang/lib/Serialization/ASTReaderDecl.cpp:1362
#7  0x00007fffba032153 in clang::ASTDeclReader::VisitVarDecl(clang::VarDecl*) (VD=0x7fff76c86940, this=0x7fff927f78c0)
    at clang/lib/Serialization/ASTReaderDecl.cpp:378
#8  clang::ASTDeclReader::VisitDecompositionDecl(clang::DecompositionDecl*) (this=0x7fff927f78c0, DD=0x7fff76c86940)
    at clang/lib/Serialization/ASTReaderDecl.cpp:1464
#9  0x00007fffba034c42 in clang::ASTDeclReader::Visit(clang::Decl*) (this=0x7fff927f78c0, D=0x7fff76c86940)
    at clang/lib/Serialization/ASTReaderDecl.cpp:522
#10 0x00007fffba03570f in clang::ASTReader::ReadDeclRecord(unsigned int) (this=0x7fff3d8d9f10, ID=ID@entry=208781)
    at clang/lib/Serialization/ASTReaderDecl.cpp:3967
#11 0x00007fffb9f85df9 in clang::ASTReader::GetDecl(unsigned int) (ID=208781, this=0x7fff3d8d9f10)
    at clang/lib/Serialization/ASTReader.cpp:7800
#12 clang::ASTReader::GetDecl(unsigned int) (this=0x7fff3d8d9f10, ID=208781)
    at clang/lib/Serialization/ASTReader.cpp:7787
#13 0x00007fffba037811 in clang::ASTReader::ReadDecl(clang::serialization::ModuleFile&, llvm::SmallVector<unsigned long, 64u> const&, unsigned int&)
    (I=<optimized out>, R=..., F=..., this=0x7fff3d8d9f10) at clang/include/clang/Serialization/ASTReader.h:1870
#14 clang::ASTRecordReader::readDecl() (this=<optimized out>) at clang/include/clang/Serialization/ASTReader.h:2524
#15 clang::ASTStmtReader::ReadDecl() (this=0x7fff927f7d70) at clang/lib/Serialization/ASTReaderStmt.cpp:91
#16 clang::ASTStmtReader::VisitDeclStmt(clang::DeclStmt*) (this=0x7fff927f7d70, S=0x7fff76c86920)
    at clang/lib/Serialization/ASTReaderStmt.cpp:348
#17 0x00007fffba04a47c in clang::ASTReader::ReadStmtFromStream(clang::serialization::ModuleFile&) (this=this@entry=0x7fff3d8d9f10, F=...)
    at clang/lib/Serialization/ASTReaderStmt.cpp:3464
#18 0x00007fffb9fa8db2 in clang::ASTReader::GetExternalDeclStmt(unsigned long) (this=0x7fff3d8d9f10, Offset=<optimized out>)
    at clang/lib/Serialization/ASTReader.cpp:7854
#19 0x00007fffb92c4df3 in clang::LazyOffsetPtr<clang::Stmt, unsigned long, &clang::ExternalASTSource::GetExternalDeclStmt>::get(clang::ExternalASTSource*) const (Source=<optimized out>, this=0x7fff76c84358) at clang/include/clang/AST/ExternalASTSource.h:400
#20 clang::FunctionDecl::getBody(clang::FunctionDecl const*&) const (this=this@entry=0x7fff76c842d8, Definition=@0x7fff927f8148: 0x7fff76c842d8)


(gdb) l DeclBase.cpp:149
145	  // Marking a DecompositionDecl as invalid implies all the child BindingDecl's
146	  // are invalid too.
147	  if (auto *DD = dyn_cast<DecompositionDecl>(this)) {
148	    for (auto *Binding : DD->bindings()) {
149	      Binding->setInvalidDecl();
150	    }
151	  }

Trivial hotfix helps (but the reason of the bug may be elsewhere):
--- a/lib/AST/DeclBase.cpp	2019-12-11 22:15:30.000000000 +0300
+++ b/lib/AST/DeclBase.cpp	2020-02-21 22:15:30.000000000 +0300
@@ -146,7 +146,9 @@
   // are invalid too.
   if (auto *DD = dyn_cast<DecompositionDecl>(this)) {
     for (auto *Binding : DD->bindings()) {
-      Binding->setInvalidDecl();
+      if (Binding) {
+        Binding->setInvalidDecl();
+      }
     }
   }
 }

The crash is actually very common, it makes recent KDevelop versions pretty unusable if you have at least some C++17 code in the project. The hotfix prevents the crash (tested by binary patching, didn’t get to compile the whole libclang yet...) but the declaration remains partially unparsed, the bindings don’t show up as valid locals.

(In reply to Kevin Funk from comment #0)
> I can't reproduce this crash with either clang, [...]
The key here is too lock at the stack trace:

> #2  0x00007fffbb7020aa in clang::ASTDeclReader::VisitDecl
> [...]
> #11 0x00007fffbb7276e1 in clang::ASTReader::ReadDeclRecord
> [...]
AST{,Decl}Reader aren't involved in a regular compilation. They read serialized ASTs from disk. You're observing this because libclang serializes a "preamble" as precompiled header for faster reparsing.

Now if we want to reproduce it, we just need to emit and then include a PCH. Since this is invalid code, we also need -fallow-pch-with-compiler-errors. Using your minimal reproducer,

clang -cc1 -xc++ -emit-pch -fallow-pch-with-compiler-errors -o test.pch test.h
clang -cc1 -fsyntax-only -include-pch test.pch -fallow-pch-with-compiler-errors test.cpp

That ends with a crash in clang::Decl::setInvalidDecl for me. We can actually simplify test.h:

[foo]

Looking at the AST:

|-BindingDecl 0x36acf8 <test.h:1:2> col:2 invalid foo 'NULL TYPE'
`-DecompositionDecl 0x36ad40 <col:1, <invalid sloc>> col:1 invalid 'int'
  `-BindingDecl 0x36acf8 <col:2> col:2 invalid foo 'NULL TYPE'

So there is one binding in the DecompositionDecl. My hypothesis: that isn't serialized or deserialized properly and ends up being a nullptr. Unfortunately I'm not really familiar with that code.

I'm beginning to think that this isn't a serialization issue, but a frontend issue. I haven't seen NULL TYPE before in ASTs, even on invalid code, because there is usually some recovery, either by typo correction, or if there is no type at all, by falling back to 'int'. E.g.

x;
f();

has this AST:

|-VarDecl 0xcf92e0 invalid x 'int'
`-FunctionDecl 0xcf93a8 invalid f 'int ()'

The frontend should probably do the same for BindingDecls, especially since the type of the DecompositionDecl is recovered as 'int'

(In reply to Aaron Puchert from comment #6)
> I'm beginning to think that this isn't a serialization issue, but a frontend
> issue.
I can fix the type issue, but it's unrelated. The issue is indeed in the deserialization code. We call VisitVarDecl before reading the BindingDecls:

void ASTDeclReader::VisitDecompositionDecl(DecompositionDecl *DD) {
  VisitVarDecl(DD);
  auto **BDs = DD->getTrailingObjects<BindingDecl *>();
  for (unsigned I = 0; I != DD->NumBindings; ++I) {
    BDs[I] = readDeclAs<BindingDecl>();
    BDs[I]->setDecomposedDecl(DD);
  }
}

From VisitVarDecl we go through the inheritance hierarchy up to VisitDecl, where we set the declaration as invalid:

D->setInvalidDecl(Record.readInt());

Now that function wants to be helpful and marks the contained BindingDecls as invalid, too:

  // Marking a DecompositionDecl as invalid implies all the child BindingDecl's
  // are invalid too.
  if (auto *DD = dyn_cast<DecompositionDecl>(this)) {
    for (auto *Binding : DD->bindings()) {
      Binding->setInvalidDecl();
    }
  }

But we haven't read them yet, so they are still nullptrs. A possible fix is indeed to check for nullptr here as suggested in comment 3. But under normal circumstances this can't happen: Sema::ActOnDecompositionDeclarator creates the BindingDecls and passes them to Sema::ActOnVariableDeclarator which builds the DecompositionDecl for them, so they are never nullptr.

So another fix would be to change the serialization format to have the BindingDecls first and then the DecompositionDecl, so in the same order as Sema builds them:

--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -1495,12 +1495,12 @@ void ASTDeclReader::VisitParmVarDecl(ParmVarDecl *PD) {
 }
 
 void ASTDeclReader::VisitDecompositionDecl(DecompositionDecl *DD) {
-  VisitVarDecl(DD);
   auto **BDs = DD->getTrailingObjects<BindingDecl *>();
   for (unsigned I = 0; I != DD->NumBindings; ++I) {
     BDs[I] = readDeclAs<BindingDecl>();
     BDs[I]->setDecomposedDecl(DD);
   }
+  VisitVarDecl(DD);
 }
 
 void ASTDeclReader::VisitBindingDecl(BindingDecl *BD) {
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1140,9 +1140,9 @@ void ASTDeclWriter::VisitDecompositionDecl(DecompositionDecl *D) {
   // Record the number of bindings first to simplify deserialization.
   Record.push_back(D->bindings().size());
 
-  VisitVarDecl(D);
   for (auto *B : D->bindings())
     Record.AddDeclRef(B);
+  VisitVarDecl(D);
   Code = serialization::DECL_DECOMPOSITION;
 }
 
Lastly, we could move the loop from Decl::setInvalidDecl to Sema::ActOnUninitializedDecl:

--- a/clang/lib/AST/DeclBase.cpp
+++ b/clang/lib/AST/DeclBase.cpp
@@ -142,14 +142,6 @@ void Decl::setInvalidDecl(bool Invalid) {
     // to avoid triggering asserts elsewhere in the front end.
     setAccess(AS_public);
   }
-
-  // Marking a DecompositionDecl as invalid implies all the child BindingDecl'
s
-  // are invalid too.
-  if (auto *DD = dyn_cast<DecompositionDecl>(this)) {
-    for (auto *Binding : DD->bindings()) {
-      Binding->setInvalidDecl();
-    }
-  }
 }
 
 const char *DeclContext::getDeclKindName() const {
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -12429,9 +12429,11 @@ void Sema::ActOnUninitializedDecl(Decl *RealDecl) {
     QualType Type = Var->getType();
 
     // C++1z [dcl.dcl]p1 grammar implies that an initializer is mandatory.
-    if (isa<DecompositionDecl>(RealDecl)) {
+    if (auto *DD = dyn_cast<DecompositionDecl>(RealDecl)) {
       Diag(Var->getLocation(), diag::err_decomp_decl_requires_init) << Var;
       Var->setInvalidDecl();
+      for (auto *Binding : DD->bindings())
+        Binding->setInvalidDecl();
       return;
     }
 
It's possible that we might miss other call sites though, and this would still leave the DecompositionDecl in a state it can't be in with parsed source.

Not sure what the right fix is. Both make the crash disappear, but I didn't run the entire testsuite yet.

Pushed the (de-)serialization fix as https://reviews.llvm.org/D86207.

Ended up fixing this differently, by setting InvalidDecl directly from the deserialization code instead of calling setInvalidDecl. Fix landed in master.

I think this is interesting for 11.0.0: the fix is very small, and shouldn't cause regressions.

(In reply to Aaron Puchert from comment #9)
> Ended up fixing this differently, by setting InvalidDecl directly from the
> deserialization code instead of calling setInvalidDecl. Fix landed in master.
> 
> I think this is interesting for 11.0.0: the fix is very small, and shouldn't
> cause regressions.

Okay, pushed as 96b8fd70d1572d3d38abce208e855c49f9eeac1d.