[modules] Fix crash in call to `FunctionDecl::setPure()`

[andrewjcg]

Summary

In some cases, when deserializing a CXXMethodDecl of a CXXSpecializationTemplateDecl,
the call to FunctionDecl::setPure() happens before the DefinitionData member has been
populated (which appears to happen lower down in a mergeRedeclarable call), causing a
crash (https://reviews.llvm.org/P8228).

This diff fixes this by deferring the FunctionDecl::setPure() till after the DefinitionData has
been filled in.

[bruno]

clang/lib/Serialization/ASTReaderDecl.cpp

874

pure -> Pure

clang/test/Modules/set-pure-crash.cpp

3

To speed this up a little, instead of -O0 -emit-llvm just use -fsyntax-only

[andrewjcg]

feedback

[lxfind]

@rsmith, @v.g.vassilev hey I stamped this patch assuming it looks ok. But definitely shout at me if more feedback needs to be addressed. Happy to follow up.

[bruno]

I forgot to follow up, but LGTM too.

@rsmith, @v.g.vassilev hey I stamped this patch assuming it looks ok. But definitely shout at me if more feedback needs to be addressed. Happy to follow up.

FWIW, this has been working on our code base for sometime now.

clang/lib/Serialization/ASTReaderDecl.cpp

void ASTDeclReader::VisitFunctionDecl(FunctionDecl *FD) {
 
   // FunctionDecl's body is handled last at ASTDeclReader::Visit,
   // after everything else is read.
 
   FD->setStorageClass(static_cast<StorageClass>(Record.readInt()));
   FD->setInlineSpecified(Record.readInt());
   FD->setImplicitlyInline(Record.readInt());
   FD->setVirtualAsWritten(Record.readInt());
-  FD->setPure(Record.readInt());
+  // We defer calling `FunctionDecl::setPure()` here as for methods of
+  // `CXXTemplateSpecializationDecl`s, we may not have connected up the
+  // definition (which is required for `setPure`).
+  const bool Pure = Record.readInt();
   FD->setHasInheritedPrototype(Record.readInt());
   FD->setHasWrittenPrototype(Record.readInt());
   FD->setDeletedAsWritten(Record.readInt());
   FD->setTrivial(Record.readInt());
   FD->setTrivialForCall(Record.readInt());
   FD->setDefaulted(Record.readInt());
   FD->setExplicitlyDefaulted(Record.readInt());
   FD->setHasImplicitReturnZero(Record.readInt());
case FunctionDecl::TK_DependentFunctionTemplateSpecialization: {
     FD->setDependentTemplateSpecialization(Reader.getContext(),
                                            TemplDecls, TemplArgs);
     // These are not merged; we don't need to merge redeclarations of dependent
     // template friends.
     break;
   }
   }
 
+  // Defer calling `setPure` until merging above has guaranteed we've set
+  // `DefinitionData` (as this will need to access it).
+  FD->setPure(Pure);
+
   // Read in the parameters.
   unsigned NumParams = Record.readInt();
   SmallVector<ParmVarDecl *, 16> Params;
   Params.reserve(NumParams);
   for (unsigned I = 0; I != NumParams; ++I)
     Params.push_back(readDeclAs<ParmVarDecl>());
   FD->setParams(Reader.getContext(), Params);
 }

clang/test/Modules/Inputs/set-pure-crash/a.h

+#pragma once
+
+struct Tag {};
+
+template <typename T>
+class Base {
+public:
+  virtual void func() = 0;
+};
+
+Base<Tag> bar();

clang/test/Modules/Inputs/set-pure-crash/b.h

+#pragma once
+
+#include "a.h"
+#include "c.h"
+
+template <typename Fun, typename = simple<Fun>>
+void foo(Fun) {}
+
+class Child : public Base<Tag> {
+public:
+  void func() {
+    foo([]() {});
+  }
+};

clang/test/Modules/Inputs/set-pure-crash/c.h

+#pragma once
+
+template <typename>
+struct simple {
+};

clang/test/Modules/Inputs/set-pure-crash/module.modulemap

+module a {
+  header "a.h"
+}
+
+module b {
+  header "b.h"
+}
+
+module c {
+  header "c.h"
+}

clang/test/Modules/set-pure-crash.cpp

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x c++ -I %S/Inputs/set-pure-crash -verify %s -o %t
+
+// expected-no-diagnostics
+
+#include "b.h"
+#include "c.h"
+
+auto t = simple<const char *>();