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

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

<details>
<summary>Changes</summary>

This fixes crashes with `-dump-deserialized-decls`. 

The `DeserializedDeclsDumper` implementation has two issues:

1. The `DelegatingDeserializationListener` does not fully implement all interfaces. As a result, the `Previous` listener object misses some critical state updates, leading to inconsistencies and eventual crashes;
3. Print declaration names in the middle of deserialization is not safe, because the decl may still be incomplete; we should do it after the deserialization is finished;

No unittest is included  -- crash was discovered by @<!-- -->VitaNuo  in an internal isolated test which is still large, depending on the stl/absl modules. The issue no longer occurs with this change.

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


4 Files Affected:

- (modified) clang/docs/ReleaseNotes.rst (+1) 
- (modified) clang/include/clang/Serialization/ASTDeserializationListener.h (+4) 
- (modified) clang/lib/Frontend/FrontendAction.cpp (+38-6) 
- (modified) clang/lib/Serialization/ASTReader.cpp (+2) 


``````````diff
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 04ec2cfef679c..9ae29d7e58626 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -316,6 +316,7 @@ Bug Fixes in This Version
 - Fixed a modules crash where exception specifications were not propagated properly (#GH121245, relanded in #GH129982)
 - Fixed a problematic case with recursive deserialization within ``FinishedDeserializing()`` where
   ``PassInterestingDeclsToConsumer()`` was called before the declarations were safe to be passed. (#GH129982)
+- Fixed a module crash with `-dump-deserialized-decls`
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Serialization/ASTDeserializationListener.h b/clang/include/clang/Serialization/ASTDeserializationListener.h
index ea96faa07c191..ccbaae93f9048 100644
--- a/clang/include/clang/Serialization/ASTDeserializationListener.h
+++ b/clang/include/clang/Serialization/ASTDeserializationListener.h
@@ -27,6 +27,8 @@ class MacroInfo;
 class Module;
 class SourceLocation;
 
+// IMPORTANT: when you add a new interface to this class, please update the
+// DelegatingDeserializationListener in FrontendAction.cpp
 class ASTDeserializationListener {
 public:
   virtual ~ASTDeserializationListener();
@@ -57,6 +59,8 @@ class ASTDeserializationListener {
   /// A module import was read from the AST file.
   virtual void ModuleImportRead(serialization::SubmoduleID ID,
                                 SourceLocation ImportLoc) {}
+  /// The deserialization of the AST file was finished.
+  virtual void FinishedDeserializing() {}
 };
 }
 
diff --git a/clang/lib/Frontend/FrontendAction.cpp b/clang/lib/Frontend/FrontendAction.cpp
index 9f789f093f55d..416ee50593e74 100644
--- a/clang/lib/Frontend/FrontendAction.cpp
+++ b/clang/lib/Frontend/FrontendAction.cpp
@@ -76,6 +76,10 @@ class DelegatingDeserializationListener : public ASTDeserializationListener {
     if (Previous)
       Previous->IdentifierRead(ID, II);
   }
+  void MacroRead(serialization::MacroID ID, MacroInfo *MI) override {
+    if (Previous)
+      Previous->MacroRead(ID, MI);
+  }
   void TypeRead(serialization::TypeIdx Idx, QualType T) override {
     if (Previous)
       Previous->TypeRead(Idx, T);
@@ -93,6 +97,19 @@ class DelegatingDeserializationListener : public ASTDeserializationListener {
     if (Previous)
       Previous->MacroDefinitionRead(PPID, MD);
   }
+  void ModuleRead(serialization::SubmoduleID ID, Module *Mod) override {
+    if (Previous)
+      Previous->ModuleRead(ID, Mod);
+  }
+  void ModuleImportRead(serialization::SubmoduleID ID,
+                        SourceLocation ImportLoc) override {
+    if (Previous)
+      Previous->ModuleImportRead(ID, ImportLoc);
+  }
+  void FinishedDeserializing() override {
+    if (Previous)
+      Previous->FinishedDeserializing();
+  }
 };
 
 /// Dumps deserialized declarations.
@@ -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()) {
+      llvm::errs() << "Deserialized more decls while printing, total of "
+                   << PendingDecls.size() << "\n";
+      PendingDecls.clear();
+    }
+    DelegatingDeserializationListener::FinishedDeserializing();
   }
+
+private:
+  std::vector<const Decl *> PendingDecls;
 };
 
 /// Checks deserialized declarations and emits error if a name
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 0cd2cedb48dd9..9e3712226491a 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -10877,6 +10877,8 @@ void ASTReader::FinishedDeserializing() {
     // decls to the consumer.
     if (Consumer)
       PassInterestingDeclsToConsumer();
+    if (DeserializationListener)
+      DeserializationListener->FinishedDeserializing();
   }
 }
 

``````````

</details>


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