[clang][Sema] Always clear UndefinedButUsed

[hahnjo]

Before, it was only cleared if there were undefined entities. This is important for Clang's incremental parsing as used by clang-repl that might receive multiple calls to Sema.ActOnEndOfTranslationUnit.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Jonas Hahnfeld (hahnjo)

Changes

Before, it was only cleared if there were undefined entities. This is important for Clang's incremental parsing as used by clang-repl that might receive multiple calls to Sema.ActOnEndOfTranslationUnit.

---

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

1 Files Affected:

• (modified) clang/lib/Sema/Sema.cpp (+1-2)

diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 9771aaa2f3b0371..d08f8cd56b39bde 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -870,6 +870,7 @@ static void checkUndefinedButUsed(Sema &S) {
   // Collect all the still-undefined entities with internal linkage.
   SmallVector<std::pair<NamedDecl *, SourceLocation>, 16> Undefined;
   S.getUndefinedButUsed(Undefined);
+  S.UndefinedButUsed.clear();
   if (Undefined.empty()) return;
 
   for (const auto &Undef : Undefined) {
@@ -923,8 +924,6 @@ static void checkUndefinedButUsed(Sema &S) {
     if (UseLoc.isValid())
       S.Diag(UseLoc, diag::note_used_here);
   }
-
-  S.UndefinedButUsed.clear();
 }
 
 void Sema::LoadExternalWeakUndeclaredIdentifiers() {

[vgvassilev]

Can you add a test?

[hahnjo]

I will try, but observing the consequences of this depends on unloading: Basically it happens if a declaration in UndefinedButUsed thas was previously defined is unloaded, which makes it undefined. For now, it's possible that for the upstream cases it's only an optimization because the data structure doesn't grow as much.

[vgvassilev]

I think I understand. @AaronBallman from what concerns me that change seems fine it'd be hard to add a test for it right now. Do you have any concerns?

[AaronBallman]

I'm okay landing these changes without test coverage, though tests are always preferred. LGTM

[hahnjo]

I tried to craft a test here, but declaration unloading in clang-repl is not powerful enough (yet) to show a (user-visible) consequence of the problem.

On a high level, the problem should be triggered by:

clang-repl> template <typename T> struct A { void f() { } };
clang-repl> A<int>().f();
clang-repl> %undo
clang-repl> A<int>().f();

In principle, the %undo should remove the implicit template instantiation of f which is then subsequently re-instantiated. This is currently not implemented in clang-repl (we just re-use the first instantiation, which is wrong in case there are more lines in between that could influence the instantiation). With debug statements, I can verify that f is in UndefinedButUsed, but getUndefinedButUsed filters it out.

The slightly more complex

clang-repl> template <typename T> struct A { void f() { } };
clang-repl> A<int>().f();
clang-repl> %undo
clang-repl> %undo
clang-repl> template <typename T> struct A { void f() { } };
clang-repl> A<int>().f();

ie unloading the entire class doesn't work either for the same reason, we never actually treat the instantiated member function. (Subsequently this leads to the entire clang-repl crashing after printing definition with same mangled name '_ZN1AIiE1fEv' as another definition...)

[vgvassilev]

In principle we could add this test as an xfail to make sure we have this scenario when unloading support is up to that stage.