[clang] [clang] Fix the crash when dumping deserialized decls (PR #133395)

[Haojian Wu via cfe-commits]

================
@@ -103,15 +120,30 @@ class DeserializedDeclsDumper : public DelegatingDeserializationListener {
       : DelegatingDeserializationListener(Previous, DeletePrevious) {}
 
   void DeclRead(GlobalDeclID ID, const Decl *D) override {
-    llvm::outs() << "PCH DECL: " << D->getDeclKindName();
-    if (const NamedDecl *ND = dyn_cast<NamedDecl>(D)) {
-      llvm::outs() << " - ";
-      ND->printQualifiedName(llvm::outs());
+    PendingDecls.push_back(D);
+    DelegatingDeserializationListener::DeclRead(ID, D);
+  }
+  void FinishedDeserializing() override {
+    auto Decls = std::move(PendingDecls);
+    for (const auto *D : Decls) {
+      llvm::outs() << "PCH DECL: " << D->getDeclKindName();
+      if (const NamedDecl *ND = dyn_cast<NamedDecl>(D)) {
+        llvm::outs() << " - ";
+        ND->printQualifiedName(llvm::outs());
+      }
+      llvm::outs() << "\n";
     }
-    llvm::outs() << "\n";
 
-    DelegatingDeserializationListener::DeclRead(ID, D);
+    if (!PendingDecls.empty()) {
----------------
hokein wrote:

> Our theory is that printQualifiedName can start deserializing more.

I tested it with the crash case, and `printQualifiedName` does not cause further deserialization with this change, I'm not certain now.

I think a broader question is: once deserialization is finished, can we safely assume that using a loaded declaration will never trigger additional deserialization?

Currently, the contract for `OurListener::FinishedDeserializing` states that it is called only after deserialization is fully completed. If the implementation of `OurListener::FinishedDeserializing` itself triggers further deserialization, that seems to break this contract. One possible solution is to disallow this behavior, e.g we could add an assertion in ASTReader.cpp.

https://github.com/llvm/llvm-project/pull/133395