Skip to content

Commit 4551e50

Browse files
authored
[clang] Reset FileID based diag state mappings (#143695)
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.
1 parent ce62104 commit 4551e50

File tree

4 files changed

+67
-4
lines changed

4 files changed

+67
-4
lines changed

clang/include/clang/Basic/Diagnostic.h

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -424,10 +424,13 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
424424
bool empty() const { return Files.empty(); }
425425

426426
/// Clear out this map.
427-
void clear() {
427+
void clear(bool Soft) {
428+
// Just clear the cache when in soft mode.
428429
Files.clear();
429-
FirstDiagState = CurDiagState = nullptr;
430-
CurDiagStateLoc = SourceLocation();
430+
if (!Soft) {
431+
FirstDiagState = CurDiagState = nullptr;
432+
CurDiagStateLoc = SourceLocation();
433+
}
431434
}
432435

433436
/// Produce a debugging dump of the diagnostic state.
@@ -920,6 +923,10 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> {
920923
/// Reset the state of the diagnostic object to its initial configuration.
921924
/// \param[in] soft - if true, doesn't reset the diagnostic mappings and state
922925
void Reset(bool soft = false);
926+
/// We keep a cache of FileIDs for diagnostics mapped by pragmas. These might
927+
/// get invalidated when diagnostics engine is shared across different
928+
/// compilations. Provide users with a way to reset that.
929+
void ResetPragmas();
923930

924931
//===--------------------------------------------------------------------===//
925932
// DiagnosticsEngine classification and reporting interfaces.

clang/lib/Basic/Diagnostic.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,8 @@ bool DiagnosticsEngine::popMappings(SourceLocation Loc) {
119119
return true;
120120
}
121121

122+
void DiagnosticsEngine::ResetPragmas() { DiagStatesByLoc.clear(/*Soft=*/true); }
123+
122124
void DiagnosticsEngine::Reset(bool soft /*=false*/) {
123125
ErrorOccurred = false;
124126
UncompilableErrorOccurred = false;
@@ -135,7 +137,7 @@ void DiagnosticsEngine::Reset(bool soft /*=false*/) {
135137
if (!soft) {
136138
// Clear state related to #pragma diagnostic.
137139
DiagStates.clear();
138-
DiagStatesByLoc.clear();
140+
DiagStatesByLoc.clear(false);
139141
DiagStateOnPushStack.clear();
140142

141143
// Create a DiagState and DiagStatePoint representing diagnostic changes

clang/lib/Basic/SourceManager.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,9 @@ void SourceManager::clearIDTables() {
344344
NextLocalOffset = 0;
345345
CurrentLoadedOffset = MaxLoadedOffset;
346346
createExpansionLoc(SourceLocation(), SourceLocation(), SourceLocation(), 1);
347+
// Diagnostics engine keeps some references to fileids, mostly for dealing
348+
// with diagnostic pragmas, make sure they're reset as well.
349+
Diag.ResetPragmas();
347350
}
348351

349352
bool SourceManager::isMainFile(const FileEntry &SourceFile) {

clang/unittests/Frontend/CompilerInstanceTest.cpp

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,12 @@
99
#include "clang/Frontend/CompilerInstance.h"
1010
#include "clang/Basic/FileManager.h"
1111
#include "clang/Frontend/CompilerInvocation.h"
12+
#include "clang/Frontend/FrontendActions.h"
1213
#include "clang/Frontend/TextDiagnosticPrinter.h"
14+
#include "llvm/ADT/IntrusiveRefCntPtr.h"
1315
#include "llvm/Support/FileSystem.h"
1416
#include "llvm/Support/Format.h"
17+
#include "llvm/Support/MemoryBuffer.h"
1518
#include "llvm/Support/ToolOutputFile.h"
1619
#include "llvm/Support/VirtualFileSystem.h"
1720
#include "gtest/gtest.h"
@@ -97,4 +100,52 @@ TEST(CompilerInstance, AllowDiagnosticLogWithUnownedDiagnosticConsumer) {
97100
ASSERT_EQ(DiagnosticOutput, "error: expected no crash\n");
98101
}
99102

103+
TEST(CompilerInstance, MultipleInputsCleansFileIDs) {
104+
auto VFS = makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
105+
VFS->addFile("a.cc", /*ModificationTime=*/{},
106+
MemoryBuffer::getMemBuffer(R"cpp(
107+
#include "a.h"
108+
)cpp"));
109+
// Paddings of `void foo();` in the sources below are "important". We're
110+
// testing against source locations from previous compilations colliding.
111+
// Hence the `unused` variable in `b.h` needs to be within `#pragma clang
112+
// diagnostic` block from `a.h`.
113+
VFS->addFile("a.h", /*ModificationTime=*/{}, MemoryBuffer::getMemBuffer(R"cpp(
114+
#include "b.h"
115+
#pragma clang diagnostic push
116+
#pragma clang diagnostic warning "-Wunused"
117+
void foo();
118+
#pragma clang diagnostic pop
119+
)cpp"));
120+
VFS->addFile("b.h", /*ModificationTime=*/{}, MemoryBuffer::getMemBuffer(R"cpp(
121+
void foo(); void foo(); void foo(); void foo();
122+
inline void foo() { int unused = 2; }
123+
)cpp"));
124+
125+
DiagnosticOptions DiagOpts;
126+
IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
127+
CompilerInstance::createDiagnostics(*VFS, DiagOpts);
128+
129+
CreateInvocationOptions CIOpts;
130+
CIOpts.Diags = Diags;
131+
132+
const char *Args[] = {"clang", "-xc++", "a.cc"};
133+
std::shared_ptr<CompilerInvocation> CInvok =
134+
createInvocation(Args, std::move(CIOpts));
135+
ASSERT_TRUE(CInvok) << "could not create compiler invocation";
136+
137+
CompilerInstance Instance(std::move(CInvok));
138+
Instance.setDiagnostics(Diags.get());
139+
Instance.createFileManager(VFS);
140+
141+
// Run once for `a.cc` and then for `a.h`. This makes sure we get the same
142+
// file ID for `b.h` in the second run as `a.h` from first run.
143+
const auto &OrigInputKind = Instance.getFrontendOpts().Inputs[0].getKind();
144+
Instance.getFrontendOpts().Inputs.emplace_back("a.h", OrigInputKind);
145+
146+
SyntaxOnlyAction Act;
147+
EXPECT_TRUE(Instance.ExecuteAction(Act)) << "Failed to execute action";
148+
EXPECT_FALSE(Diags->hasErrorOccurred());
149+
EXPECT_EQ(Diags->getNumWarnings(), 0u);
150+
}
100151
} // anonymous namespace

0 commit comments

Comments
 (0)