[clang] Reset FileID based diag state mappings #143695
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When sharing same compiler instance for multiple compilations, we reset source manager's file id tables in between runs. Diagnostics engine keeps a cache based on these file ids, that became dangling references across compilations. This patch makes sure we reset those whenever sourcemanager is trashing its FileIDs.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: kadir çetinkaya (kadircet) ChangesWhen sharing same compiler instance for multiple compilations, we reset This patch makes sure we reset those whenever sourcemanager is trashing Full diff: https://github.com/llvm/llvm-project/pull/143695.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index e9c54c3c487c9..b5cbda9e31049 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -420,10 +420,13 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
bool empty() const { return Files.empty(); }
/// Clear out this map.
- void clear() {
+ void clear(bool Soft) {
+ // Just clear the cache when in soft mode.
Files.clear();
- FirstDiagState = CurDiagState = nullptr;
- CurDiagStateLoc = SourceLocation();
+ if (!Soft) {
+ FirstDiagState = CurDiagState = nullptr;
+ CurDiagStateLoc = SourceLocation();
+ }
}
/// Produce a debugging dump of the diagnostic state.
@@ -918,6 +921,10 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
/// Reset the state of the diagnostic object to its initial configuration.
/// \param[in] soft - if true, doesn't reset the diagnostic mappings and state
void Reset(bool soft = false);
+ /// We keep a cache of FileIDs for diagnostics mapped by pragmas. These might
+ /// get invalidated when diagnostics engine is shared across different
+ /// compilations. Provide users with a way to reset that.
+ void ResetPragmas();
//===--------------------------------------------------------------------===//
// DiagnosticsEngine classification and reporting interfaces.
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index 694224071347a..6d4f2ba68072f 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -119,6 +119,8 @@ bool DiagnosticsEngine::popMappings(SourceLocation Loc) {
return true;
}
+void DiagnosticsEngine::ResetPragmas() { DiagStatesByLoc.clear(/*Soft=*/true); }
+
void DiagnosticsEngine::Reset(bool soft /*=false*/) {
ErrorOccurred = false;
UncompilableErrorOccurred = false;
@@ -135,7 +137,7 @@ void DiagnosticsEngine::Reset(bool soft /*=false*/) {
if (!soft) {
// Clear state related to #pragma diagnostic.
DiagStates.clear();
- DiagStatesByLoc.clear();
+ DiagStatesByLoc.clear(false);
DiagStateOnPushStack.clear();
// Create a DiagState and DiagStatePoint representing diagnostic changes
diff --git a/clang/lib/Basic/SourceManager.cpp b/clang/lib/Basic/SourceManager.cpp
index 09e5c6547fb51..053e82683a4a6 100644
--- a/clang/lib/Basic/SourceManager.cpp
+++ b/clang/lib/Basic/SourceManager.cpp
@@ -344,6 +344,9 @@ void SourceManager::clearIDTables() {
NextLocalOffset = 0;
CurrentLoadedOffset = MaxLoadedOffset;
createExpansionLoc(SourceLocation(), SourceLocation(), SourceLocation(), 1);
+ // Diagnostics engine keeps some references to fileids, mostly for dealing
+ // with diagnostic pragmas, make sure they're reset as well.
+ Diag.ResetPragmas();
}
bool SourceManager::isMainFile(const FileEntry &SourceFile) {
diff --git a/clang/unittests/Frontend/CompilerInstanceTest.cpp b/clang/unittests/Frontend/CompilerInstanceTest.cpp
index a7b258d5e537e..459a3864887e1 100644
--- a/clang/unittests/Frontend/CompilerInstanceTest.cpp
+++ b/clang/unittests/Frontend/CompilerInstanceTest.cpp
@@ -9,9 +9,12 @@
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Basic/FileManager.h"
#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/FrontendActions.h"
#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Format.h"
+#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/ToolOutputFile.h"
#include "llvm/Support/VirtualFileSystem.h"
#include "gtest/gtest.h"
@@ -97,4 +100,52 @@ TEST(CompilerInstance, AllowDiagnosticLogWithUnownedDiagnosticConsumer) {
ASSERT_EQ(DiagnosticOutput, "error: expected no crash\n");
}
+TEST(CompilerInstance, MultipleInputsCleansFileIDs) {
+ auto VFS = makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+ VFS->addFile("a.cc", /*ModificationTime=*/{},
+ MemoryBuffer::getMemBuffer(R"cpp(
+ #include "a.h"
+ )cpp"));
+ // Paddings of `void foo();` in the sources below are "important". We're
+ // testing against source locations from previous compilations colliding.
+ // Hence the `unused` variable in `b.h` needs to be within `#pragma clang
+ // diagnostic` block from `a.h`.
+ VFS->addFile("a.h", /*ModificationTime=*/{}, MemoryBuffer::getMemBuffer(R"cpp(
+ #include "b.h"
+ #pragma clang diagnostic push
+ #pragma clang diagnostic warning "-Wunused"
+ void foo();
+ #pragma clang diagnostic pop
+ )cpp"));
+ VFS->addFile("b.h", /*ModificationTime=*/{}, MemoryBuffer::getMemBuffer(R"cpp(
+ void foo(); void foo(); void foo(); void foo();
+ inline void foo() { int unused = 2; }
+ )cpp"));
+
+ DiagnosticOptions DiagOpts;
+ IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
+ CompilerInstance::createDiagnostics(*VFS, DiagOpts);
+
+ CreateInvocationOptions CIOpts;
+ CIOpts.Diags = Diags;
+
+ const char *Args[] = {"clang", "-xc++", "a.cc"};
+ std::shared_ptr<CompilerInvocation> CInvok =
+ createInvocation(Args, std::move(CIOpts));
+ ASSERT_TRUE(CInvok) << "could not create compiler invocation";
+
+ CompilerInstance Instance(std::move(CInvok));
+ Instance.setDiagnostics(Diags.get());
+ Instance.createFileManager(VFS);
+
+ // Run once for `a.cc` and then for `a.h`. This makes sure we get the same
+ // file ID for `b.h` in the second run as `a.h` from first run.
+ const auto &OrigInputKind = Instance.getFrontendOpts().Inputs[0].getKind();
+ Instance.getFrontendOpts().Inputs.emplace_back("a.h", OrigInputKind);
+
+ SyntaxOnlyAction Act;
+ EXPECT_TRUE(Instance.ExecuteAction(Act)) << "Failed to execute action";
+ EXPECT_FALSE(Diags->hasErrorOccurred());
+ EXPECT_EQ(Diags->getNumWarnings(), 0u);
+}
} // anonymous namespace
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/30582 Here is the relevant piece of the build log for the reference |
|
FWIW, I am seeing an assertion failure when running |
|
hi @nathanchance, I can't seem to reproduce the failure, and I couldn't find any buildbots with this failure (also the assertion you're seeing conceptually shouldn't be triggered by state in diagnostics engine). I am not sure how to proceed here but can you at least verify if the issue is not specific to your environment and reproduces elsewhere? |
|
ok so I did a little more investigation here, and tl;dr is:
going to mention this in the original commit to see what authors think. |
tomtor
pushed a commit
to tomtor/llvm-project
that referenced
this pull request
Jun 14, 2025
When sharing same compiler instance for multiple compilations, we reset source manager's file id tables in between runs. Diagnostics engine keeps a cache based on these file ids, that became dangling references across compilations. This patch makes sure we reset those whenever sourcemanager is trashing its FileIDs.
|
I came up with this: Registering just the targets without the MC info causes the CodeGenTests to fail deterministically because they can't produce assembly, as you mention in other comments. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When sharing same compiler instance for multiple compilations, we reset
source manager's file id tables in between runs. Diagnostics engine
keeps a cache based on these file ids, that became dangling references
across compilations.
This patch makes sure we reset those whenever sourcemanager is trashing
its FileIDs.