[clang-repl] Fix assertion failure in CleanUpPTU()

[weliveindetail]

This assertion failure is one (fortunate) symptom:

clang/include/clang/AST/DeclBase.h:1337: reference clang::DeclListNode::iterator::operator*() const: Assertion `Ptr && "dereferencing end() iterator"' failed.

Using remove() from DeclContext::lookup_result list invalidates iterators. It can't be used while iterating.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Stefan Gränitz (weliveindetail)

Changes

This assertion failure is one (fortunate) symptom:

clang/include/clang/AST/DeclBase.h:1337: reference clang::DeclListNode::iterator::operator*() const: Assertion `Ptr && "dereferencing end() iterator"' failed.

Using remove() from DeclContext::lookup_result list invalidates iterators. It can't be used while iterating.

---

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

1 Files Affected:

• (modified) clang/lib/Interpreter/IncrementalParser.cpp (+13-6)

diff --git a/clang/lib/Interpreter/IncrementalParser.cpp b/clang/lib/Interpreter/IncrementalParser.cpp
index 370bcbfee8b014..fdb7b686ecfdcc 100644
--- a/clang/lib/Interpreter/IncrementalParser.cpp
+++ b/clang/lib/Interpreter/IncrementalParser.cpp
@@ -13,6 +13,7 @@
 #include "IncrementalParser.h"
 
 #include "clang/AST/DeclContextInternals.h"
+#include "clang/AST/DeclarationName.h"
 #include "clang/CodeGen/BackendUtil.h"
 #include "clang/CodeGen/CodeGenAction.h"
 #include "clang/CodeGen/ModuleBuilder.h"
@@ -375,16 +376,22 @@ void IncrementalParser::CleanUpPTU(PartialTranslationUnit &PTU) {
   TranslationUnitDecl *MostRecentTU = PTU.TUPart;
   TranslationUnitDecl *FirstTU = MostRecentTU->getFirstDecl();
   if (StoredDeclsMap *Map = FirstTU->getPrimaryContext()->getLookupPtr()) {
-    for (auto I = Map->begin(); I != Map->end(); ++I) {
-      StoredDeclsList &List = I->second;
+    for (auto &&[Key, List] : *Map) {
       DeclContextLookupResult R = List.getLookupResult();
+      std::vector<NamedDecl *> NamedDeclsToRemove;
+      bool RemoveAll = true;
       for (NamedDecl *D : R) {
-        if (D->getTranslationUnitDecl() == MostRecentTU) {
+        if (D->getTranslationUnitDecl() == MostRecentTU)
+          NamedDeclsToRemove.push_back(D);
+        else
+          RemoveAll = false;
+      }
+      if (LLVM_LIKELY(RemoveAll)) {
+        Map->erase(Key);
+      } else {
+        for (NamedDecl *D : NamedDeclsToRemove)
           List.remove(D);
-        }
       }
-      if (List.isNull())
-        Map->erase(I);
     }
   }
 }

[vgvassilev]

Nice catch! Any chance for having a test?

[weliveindetail]

I can reproduce it downstream in the implicit Undo after including a header that causes a lot of parse errors in nested headers. I am afraid it's unpractical to emulate in the test suite upstream.

The code is exercised in all Undo tests. They don't reproduce the issue right now, but it's just a matter of likeliness. I think this fix is fine without an additional test.

[vgvassilev]

Okay, sounds fair.

[vgvassilev]

Thank you!

[weliveindetail]

Two minor remarks

[weliveindetail]

BTW it would have been nice to use llvm::remove_if, but the iterator doesn't support assignment unfortunately.

[weliveindetail]

This is not an array (otherwise we could have done erase-remove)