[clang-tools-extra] [include-cleaner] Fix a race issue when editing multiple files. (PR #76960)
Haojian Wu via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 5 00:58:18 PST 2024
https://github.com/hokein updated https://github.com/llvm/llvm-project/pull/76960
>From 6cc7141f1f182763ccec8a4801d3b866cc839324 Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Thu, 4 Jan 2024 14:59:22 +0100
Subject: [PATCH 1/4] [include-cleaner] Fix a race issue when editing multiple
files.
We have a previous fix https://github.com/llvm/llvm-project/commit/be861b64d94198230d8f9889b17280e3cd215a0a,
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 header.
This is a different fix, we don't modify files on the fly, instead,
we write files when the tool finishes analysises for all files.
---
.../include-cleaner/tool/IncludeCleaner.cpp | 54 +++++++++----------
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
index 30aaee29b9a397..eacdfab57b74f0 100644
--- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -16,10 +16,10 @@
#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/STLFunctionalExtras.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FormatVariadic.h"
-#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Regex.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/raw_ostream.h"
@@ -110,14 +110,16 @@ format::FormatStyle getStyle(llvm::StringRef Filename) {
class Action : public clang::ASTFrontendAction {
public:
- Action(llvm::function_ref<bool(llvm::StringRef)> HeaderFilter)
- : HeaderFilter(HeaderFilter){};
+ Action(llvm::function_ref<bool(llvm::StringRef)> HeaderFilter,
+ llvm::StringMap<std::string> &FileEdits)
+ : HeaderFilter(HeaderFilter), EditFiles(FileEdits) {}
private:
RecordedAST AST;
RecordedPP PP;
PragmaIncludes PI;
llvm::function_ref<bool(llvm::StringRef)> HeaderFilter;
+ llvm::StringMap<std::string>& EditFiles;
bool BeginInvocation(CompilerInstance &CI) override {
// We only perform include-cleaner analysis. So we disable diagnostics that
@@ -181,17 +183,8 @@ class Action : public clang::ASTFrontendAction {
}
}
- if (Edit && (!Results.Missing.empty() || !Results.Unused.empty())) {
- if (auto Err = llvm::writeToOutput(
- Path, [&](llvm::raw_ostream &OS) -> llvm::Error {
- OS << Final;
- return llvm::Error::success();
- })) {
- llvm::errs() << "Failed to apply edits to " << Path << ": "
- << toString(std::move(Err)) << "\n";
- ++Errors;
- }
- }
+ if (!Results.Missing.empty() || !Results.Unused.empty())
+ EditFiles.try_emplace(Path, Final);
}
void writeHTML() {
@@ -215,11 +208,15 @@ class ActionFactory : public tooling::FrontendActionFactory {
: HeaderFilter(HeaderFilter) {}
std::unique_ptr<clang::FrontendAction> create() override {
- return std::make_unique<Action>(HeaderFilter);
+ return std::make_unique<Action>(HeaderFilter, EditFiles);
}
+ const llvm::StringMap<std::string> &getEditFiles() const { return EditFiles; }
+
private:
llvm::function_ref<bool(llvm::StringRef)> HeaderFilter;
+ // Map from file name to final code with the include edits applied.
+ llvm::StringMap<std::string> EditFiles;
};
std::function<bool(llvm::StringRef)> headerFilter() {
@@ -274,21 +271,24 @@ int main(int argc, const char **argv) {
clang::tooling::ClangTool Tool(OptionsParser->getCompilations(),
OptionsParser->getSourcePathList());
- std::vector<std::unique_ptr<llvm::MemoryBuffer>> Buffers;
- for (const auto &File : OptionsParser->getSourcePathList()) {
- auto Content = llvm::MemoryBuffer::getFile(File);
- if (!Content) {
- llvm::errs() << "Error: can't read file '" << File
- << "': " << Content.getError().message() << "\n";
- return 1;
- }
- Buffers.push_back(std::move(Content.get()));
- Tool.mapVirtualFile(File, Buffers.back()->getBuffer());
- }
auto HeaderFilter = headerFilter();
if (!HeaderFilter)
return 1; // error already reported.
ActionFactory Factory(HeaderFilter);
- return Tool.run(&Factory) || Errors != 0;
+ auto ErrorCode = Tool.run(&Factory);
+ if (Edit) {
+ for (const auto &[FileName, FinalCode] : Factory.getEditFiles()) {
+ if (auto Err = llvm::writeToOutput(
+ FileName, [&](llvm::raw_ostream &OS) -> llvm::Error {
+ OS << FinalCode;
+ return llvm::Error::success();
+ })) {
+ llvm::errs() << "Failed to apply edits to " << FileName << ": "
+ << toString(std::move(Err)) << "\n";
+ ++Errors;
+ }
+ }
+ }
+ return ErrorCode || Errors != 0;
}
>From acace9a6753e8f95a718b8ec7464c465c7687d72 Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Fri, 5 Jan 2024 09:35:24 +0100
Subject: [PATCH 2/4] Add comment and fix clang-format.
---
.../include-cleaner/tool/IncludeCleaner.cpp | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
index eacdfab57b74f0..7df1b959a4ae0e 100644
--- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -111,15 +111,15 @@ format::FormatStyle getStyle(llvm::StringRef Filename) {
class Action : public clang::ASTFrontendAction {
public:
Action(llvm::function_ref<bool(llvm::StringRef)> HeaderFilter,
- llvm::StringMap<std::string> &FileEdits)
- : HeaderFilter(HeaderFilter), EditFiles(FileEdits) {}
+ llvm::StringMap<std::string> &EditedFiles)
+ : HeaderFilter(HeaderFilter), EditedFiles(EditedFiles) {}
private:
RecordedAST AST;
RecordedPP PP;
PragmaIncludes PI;
llvm::function_ref<bool(llvm::StringRef)> HeaderFilter;
- llvm::StringMap<std::string>& EditFiles;
+ llvm::StringMap<std::string> &EditedFiles;
bool BeginInvocation(CompilerInstance &CI) override {
// We only perform include-cleaner analysis. So we disable diagnostics that
@@ -184,7 +184,7 @@ class Action : public clang::ASTFrontendAction {
}
if (!Results.Missing.empty() || !Results.Unused.empty())
- EditFiles.try_emplace(Path, Final);
+ EditedFiles.try_emplace(Path, Final);
}
void writeHTML() {
@@ -208,15 +208,15 @@ class ActionFactory : public tooling::FrontendActionFactory {
: HeaderFilter(HeaderFilter) {}
std::unique_ptr<clang::FrontendAction> create() override {
- return std::make_unique<Action>(HeaderFilter, EditFiles);
+ return std::make_unique<Action>(HeaderFilter, EditedFiles);
}
- const llvm::StringMap<std::string> &getEditFiles() const { return EditFiles; }
+ const llvm::StringMap<std::string> &editedFiles() const { return EditedFiles; }
private:
llvm::function_ref<bool(llvm::StringRef)> HeaderFilter;
// Map from file name to final code with the include edits applied.
- llvm::StringMap<std::string> EditFiles;
+ llvm::StringMap<std::string> EditedFiles;
};
std::function<bool(llvm::StringRef)> headerFilter() {
@@ -278,7 +278,7 @@ int main(int argc, const char **argv) {
ActionFactory Factory(HeaderFilter);
auto ErrorCode = Tool.run(&Factory);
if (Edit) {
- for (const auto &[FileName, FinalCode] : Factory.getEditFiles()) {
+ for (const auto &[FileName, FinalCode] : Factory.editedFiles()) {
if (auto Err = llvm::writeToOutput(
FileName, [&](llvm::raw_ostream &OS) -> llvm::Error {
OS << FinalCode;
>From 62bf279a1cfebf6fc779f5fdabaf5eacabcb96d6 Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Fri, 5 Jan 2024 09:53:01 +0100
Subject: [PATCH 3/4] clang-format
---
clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
index 7df1b959a4ae0e..412b718a9c4660 100644
--- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -211,7 +211,9 @@ class ActionFactory : public tooling::FrontendActionFactory {
return std::make_unique<Action>(HeaderFilter, EditedFiles);
}
- const llvm::StringMap<std::string> &editedFiles() const { return EditedFiles; }
+ const llvm::StringMap<std::string> &editedFiles() const {
+ return EditedFiles;
+ }
private:
llvm::function_ref<bool(llvm::StringRef)> HeaderFilter;
@@ -278,7 +280,9 @@ int main(int argc, const char **argv) {
ActionFactory Factory(HeaderFilter);
auto ErrorCode = Tool.run(&Factory);
if (Edit) {
- for (const auto &[FileName, FinalCode] : Factory.editedFiles()) {
+ for (const auto & NameAndContent: Factory.editedFiles()) {
+ llvm::StringRef FileName = NameAndContent.first();
+ const std::string& FinalCode = NameAndContent.second;
if (auto Err = llvm::writeToOutput(
FileName, [&](llvm::raw_ostream &OS) -> llvm::Error {
OS << FinalCode;
>From a0fb5d7ba30d9cc0645470763641c5fcadc4dd7d Mon Sep 17 00:00:00 2001
From: Haojian Wu <hokein.wu at gmail.com>
Date: Fri, 5 Jan 2024 09:57:28 +0100
Subject: [PATCH 4/4] clang-format2
---
clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
index 412b718a9c4660..e078bfae66c3b5 100644
--- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -280,9 +280,9 @@ int main(int argc, const char **argv) {
ActionFactory Factory(HeaderFilter);
auto ErrorCode = Tool.run(&Factory);
if (Edit) {
- for (const auto & NameAndContent: Factory.editedFiles()) {
+ for (const auto &NameAndContent : Factory.editedFiles()) {
llvm::StringRef FileName = NameAndContent.first();
- const std::string& FinalCode = NameAndContent.second;
+ const std::string &FinalCode = NameAndContent.second;
if (auto Err = llvm::writeToOutput(
FileName, [&](llvm::raw_ostream &OS) -> llvm::Error {
OS << FinalCode;
More information about the cfe-commits
mailing list