[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:53:20 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/3] [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/3] 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/3] 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;



More information about the cfe-commits mailing list