Skip to content

Commit 67963d3

Browse files
authored
[include-cleaner] Fix a race issue when editing multiple files. (#76960)
We have a previous fix be861b6, which snapshots all processing files. It works most of times, the snapshot (InMemoryFileSystem) is based on the file path. The file-path-based lookup can fail in a subtle way for some tricky cases (we encounter it internally), which will result in reading a corrupted file. This is a different fix, we don't modify files on the fly, instead, we write files when the tool finishes for all files.
1 parent 86ef039 commit 67963d3

File tree

1 file changed

+31
-27
lines changed

1 file changed

+31
-27
lines changed

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

Lines changed: 31 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,10 @@
1616
#include "clang/Tooling/Tooling.h"
1717
#include "llvm/ADT/STLFunctionalExtras.h"
1818
#include "llvm/ADT/SmallVector.h"
19+
#include "llvm/ADT/StringMap.h"
1920
#include "llvm/ADT/StringRef.h"
2021
#include "llvm/Support/CommandLine.h"
2122
#include "llvm/Support/FormatVariadic.h"
22-
#include "llvm/Support/MemoryBuffer.h"
2323
#include "llvm/Support/Regex.h"
2424
#include "llvm/Support/Signals.h"
2525
#include "llvm/Support/raw_ostream.h"
@@ -110,14 +110,16 @@ format::FormatStyle getStyle(llvm::StringRef Filename) {
110110

111111
class Action : public clang::ASTFrontendAction {
112112
public:
113-
Action(llvm::function_ref<bool(llvm::StringRef)> HeaderFilter)
114-
: HeaderFilter(HeaderFilter){};
113+
Action(llvm::function_ref<bool(llvm::StringRef)> HeaderFilter,
114+
llvm::StringMap<std::string> &EditedFiles)
115+
: HeaderFilter(HeaderFilter), EditedFiles(EditedFiles) {}
115116

116117
private:
117118
RecordedAST AST;
118119
RecordedPP PP;
119120
PragmaIncludes PI;
120121
llvm::function_ref<bool(llvm::StringRef)> HeaderFilter;
122+
llvm::StringMap<std::string> &EditedFiles;
121123

122124
bool BeginInvocation(CompilerInstance &CI) override {
123125
// We only perform include-cleaner analysis. So we disable diagnostics that
@@ -181,17 +183,8 @@ class Action : public clang::ASTFrontendAction {
181183
}
182184
}
183185

184-
if (Edit && (!Results.Missing.empty() || !Results.Unused.empty())) {
185-
if (auto Err = llvm::writeToOutput(
186-
Path, [&](llvm::raw_ostream &OS) -> llvm::Error {
187-
OS << Final;
188-
return llvm::Error::success();
189-
})) {
190-
llvm::errs() << "Failed to apply edits to " << Path << ": "
191-
<< toString(std::move(Err)) << "\n";
192-
++Errors;
193-
}
194-
}
186+
if (!Results.Missing.empty() || !Results.Unused.empty())
187+
EditedFiles.try_emplace(Path, Final);
195188
}
196189

197190
void writeHTML() {
@@ -215,11 +208,17 @@ class ActionFactory : public tooling::FrontendActionFactory {
215208
: HeaderFilter(HeaderFilter) {}
216209

217210
std::unique_ptr<clang::FrontendAction> create() override {
218-
return std::make_unique<Action>(HeaderFilter);
211+
return std::make_unique<Action>(HeaderFilter, EditedFiles);
212+
}
213+
214+
const llvm::StringMap<std::string> &editedFiles() const {
215+
return EditedFiles;
219216
}
220217

221218
private:
222219
llvm::function_ref<bool(llvm::StringRef)> HeaderFilter;
220+
// Map from file name to final code with the include edits applied.
221+
llvm::StringMap<std::string> EditedFiles;
223222
};
224223

225224
std::function<bool(llvm::StringRef)> headerFilter() {
@@ -274,21 +273,26 @@ int main(int argc, const char **argv) {
274273

275274
clang::tooling::ClangTool Tool(OptionsParser->getCompilations(),
276275
OptionsParser->getSourcePathList());
277-
std::vector<std::unique_ptr<llvm::MemoryBuffer>> Buffers;
278-
for (const auto &File : OptionsParser->getSourcePathList()) {
279-
auto Content = llvm::MemoryBuffer::getFile(File);
280-
if (!Content) {
281-
llvm::errs() << "Error: can't read file '" << File
282-
<< "': " << Content.getError().message() << "\n";
283-
return 1;
284-
}
285-
Buffers.push_back(std::move(Content.get()));
286-
Tool.mapVirtualFile(File, Buffers.back()->getBuffer());
287-
}
288276

289277
auto HeaderFilter = headerFilter();
290278
if (!HeaderFilter)
291279
return 1; // error already reported.
292280
ActionFactory Factory(HeaderFilter);
293-
return Tool.run(&Factory) || Errors != 0;
281+
auto ErrorCode = Tool.run(&Factory);
282+
if (Edit) {
283+
for (const auto &NameAndContent : Factory.editedFiles()) {
284+
llvm::StringRef FileName = NameAndContent.first();
285+
const std::string &FinalCode = NameAndContent.second;
286+
if (auto Err = llvm::writeToOutput(
287+
FileName, [&](llvm::raw_ostream &OS) -> llvm::Error {
288+
OS << FinalCode;
289+
return llvm::Error::success();
290+
})) {
291+
llvm::errs() << "Failed to apply edits to " << FileName << ": "
292+
<< toString(std::move(Err)) << "\n";
293+
++Errors;
294+
}
295+
}
296+
}
297+
return ErrorCode || Errors != 0;
294298
}

0 commit comments

Comments
 (0)