Switch to copying source files only after the indexing process

Indexer-PiperOrigin-RevId: 779213411

Author: OSS-Fuzz Team

infra/indexer/index/file_copier.cc

@@ -17,7 +17,6 @@
 #include <filesystem>  // NOLINT
 #include <string>
 #include <system_error>  // NOLINT
-#include <utility>
 #include <vector>
 
 #include "absl/container/flat_hash_set.h"
@@ -42,12 +41,10 @@ void PreparePath(std::string& path) {
 
 FileCopier::FileCopier(absl::string_view base_path,
                        absl::string_view index_path,
-                       const std::vector<std::string>& extra_paths,
-                       bool dry_run)
+                       const std::vector<std::string>& extra_paths)
     : base_path_(base_path),
       extra_paths_(extra_paths),
-      index_path_(index_path),
-      dry_run_(dry_run) {
+      index_path_(index_path) {
   PreparePath(base_path_);
   for (std::string& extra_path : extra_paths_) {
     PreparePath(extra_path);
@@ -74,49 +71,49 @@ std::string FileCopier::ToIndexPath(absl::string_view path) const {
   return result;
 }
 
-void FileCopier::CopyFileIfNecessary(absl::string_view index_path) {
+void FileCopier::RegisterIndexedFile(absl::string_view index_path) {
   if (index_path.empty() || index_path.starts_with('<')) {
     // Built-in header or a location lacking the filename.
     return;
   }
 
-  std::filesystem::path src_path;
-  std::filesystem::path dst_path;
-  src_path = std::filesystem::path(index_path);
-  if (src_path.is_absolute()) {
-    dst_path =
-        std::filesystem::path(index_path_) / "absolute" / index_path.substr(1);
-  } else {
-    src_path = std::filesystem::path(base_path_) / index_path;
-    dst_path = std::filesystem::path(index_path_) / "relative" / index_path;
+  {
+    absl::MutexLock lock(&mutex_);
+    indexed_files_.emplace(index_path);
   }
+}
 
-  DLOG(INFO) << "From: " << src_path << "\n  To: " << dst_path << "\n";
+void FileCopier::CopyIndexedFiles() {
+  absl::MutexLock lock(&mutex_);
+
+  for (const std::string& indexed_path : indexed_files_) {
+    std::filesystem::path src_path = indexed_path;
+    std::filesystem::path dst_path;
+    if (src_path.is_absolute()) {
+      dst_path = std::filesystem::path(index_path_) / "absolute" /
+                 indexed_path.substr(1);
+    } else {
+      src_path = std::filesystem::path(base_path_) / indexed_path;
+      dst_path = std::filesystem::path(index_path_) / "relative" / indexed_path;
+    }
 
-  CHECK(std::filesystem::exists(src_path))
-      << "Source file does not exist: " << index_path;
+    DLOG(INFO) << "\nFrom: " << src_path << "\n  To: " << dst_path << "\n";
 
-  bool should_copy = false;
-  {
-    absl::MutexLock lock(&mutex_);
-    should_copy = indexed_files_.insert(dst_path).second;
-  }
+    QCHECK(std::filesystem::exists(src_path))
+        << "Source file does not exist: " << src_path;
 
-  if (should_copy && !dry_run_) {
     std::error_code error_code;
-    // We can race on creating the destination directory structure, so silently
-    // ignore errors here.
+    // The destination directory may already exist, but report other errors.
     (void)std::filesystem::create_directories(dst_path.parent_path(),
                                               error_code);
+    QCHECK(!error_code) << "Failed to create directory: "
+                        << dst_path.parent_path()
+                        << " (error: " << error_code.message() << ")";
 
-    // We cannot race on creating the same destination file.
-    QCHECK(std::filesystem::copy_file(
-        src_path, dst_path, std::filesystem::copy_options::overwrite_existing,
-        error_code))
-        << "Failed to copy file: " << src_path
+    QCHECK(std::filesystem::copy_file(src_path, dst_path, error_code))
+        << "Failed to copy file " << src_path << " to " << dst_path
         << " (error: " << error_code.message() << ")";
   }
 }
-
 }  // namespace indexer
 }  // namespace oss_fuzz

infra/indexer/index/file_copier.h

@@ -33,25 +33,26 @@ namespace indexer {
 class FileCopier {
  public:
   FileCopier(absl::string_view base_path, absl::string_view index_path,
-             const std::vector<std::string>& extra_paths, bool dry_run = false);
+             const std::vector<std::string>& extra_paths);
   FileCopier(const FileCopier&) = delete;
 
   // Rewrites a path into the representation it will have in the index
   // (relative if within the source tree and absolute otherwise).
   std::string ToIndexPath(absl::string_view path) const;
 
   // `index_path` is expected to be produced by `ToIndexPath`.
-  void CopyFileIfNecessary(absl::string_view index_path);
+  void RegisterIndexedFile(absl::string_view index_path);
+
+  // Single-threaded.
+  void CopyIndexedFiles();
 
  private:
   std::string base_path_;
   std::vector<std::string> extra_paths_;
   const std::filesystem::path index_path_;
-  bool dry_run_;
 
   absl::Mutex mutex_;
-  absl::flat_hash_set<std::filesystem::path> indexed_files_
-      ABSL_GUARDED_BY(mutex_);
+  absl::flat_hash_set<std::string> indexed_files_ ABSL_GUARDED_BY(mutex_);
 };
 
 }  // namespace indexer

infra/indexer/index/file_copier_unittest.cc

@@ -64,7 +64,7 @@ TEST(FileCopierTest, ToIndexPathOutside) {
   EXPECT_DEATH(file_copier.ToIndexPath("/a/b/c/d.cc"), "/a/b/c/d.cc");
 }
 
-TEST(FileCopierTest, CopyFileIfNecessary) {
+TEST(FileCopierTest, FileCopying) {
   auto tmp_dir_path = std::filesystem::path(::testing::TempDir());
   auto base_path = tmp_dir_path / "src";
   auto index_path = tmp_dir_path / "idx";
@@ -85,18 +85,15 @@ TEST(FileCopierTest, CopyFileIfNecessary) {
   auto file_c_copy = index_path / "absolute" /
                      sysroot_path.lexically_relative("/") / "y/z/c.cc";
 
-  file_copier.CopyFileIfNecessary(file_copier.ToIndexPath(file_a.string()));
-  file_copier.CopyFileIfNecessary(file_copier.ToIndexPath(file_b.string()));
-  file_copier.CopyFileIfNecessary(file_copier.ToIndexPath(file_c.string()));
+  file_copier.RegisterIndexedFile(file_copier.ToIndexPath(file_a.string()));
+  file_copier.RegisterIndexedFile(file_copier.ToIndexPath(file_b.string()));
+  file_copier.RegisterIndexedFile(file_copier.ToIndexPath(file_c.string()));
+
+  file_copier.CopyIndexedFiles();
 
   EXPECT_EQ(GetFileContents(file_a_copy).value_or(""), "a.cc");
   EXPECT_EQ(GetFileContents(file_b_copy).value_or(""), "b.cc");
   EXPECT_EQ(GetFileContents(file_c_copy).value_or(""), "c.cc");
-
-  // Delete `file_a` and check that it's not copied more than once.
-  std::filesystem::remove(file_a_copy);
-  file_copier.CopyFileIfNecessary(file_copier.ToIndexPath(file_a.string()));
-  EXPECT_FALSE(std::filesystem::exists(file_a_copy));
 }
 
 }  // namespace indexer

infra/indexer/index/in_memory_index.cc

@@ -280,7 +280,7 @@ LocationId InMemoryIndex::GetLocationId(const Location& location) {
   auto [iter, inserted] = locations_.insert({new_location, next_location_id_});
   if (inserted) {
     next_location_id_++;
-    file_copier_.CopyFileIfNecessary(new_location.path());
+    file_copier_.RegisterIndexedFile(new_location.path());
   }
 
   return iter->second;

infra/indexer/main.cc

@@ -17,6 +17,7 @@
 #include <filesystem>  // NOLINT
 #include <memory>
 #include <string>
+#include <system_error>  // NOLINT
 #include <utility>
 #include <vector>
 
@@ -37,7 +38,8 @@
 
 ABSL_FLAG(std::string, source_dir, "", "Source directory");
 ABSL_FLAG(std::string, build_dir, "", "Build directory");
-ABSL_FLAG(std::string, index_dir, "", "Output index file directory");
+ABSL_FLAG(std::string, index_dir, "",
+          "Output index file directory (should be empty if it exists)");
 ABSL_FLAG(std::vector<std::string>, extra_dirs, {"/"},
           "Additional source directory/-ies (comma-separated)");
 ABSL_FLAG(std::string, dry_run_regex, "",
@@ -84,8 +86,7 @@ int main(int argc, char** argv) {
     clang::tooling::Filter.setValue(dry_run_regex);
   }
 
-  oss_fuzz::indexer::FileCopier file_copier(source_dir, index_dir, extra_dirs,
-                                            /*dry_run=*/!dry_run_regex.empty());
+  oss_fuzz::indexer::FileCopier file_copier(source_dir, index_dir, extra_dirs);
 
   std::unique_ptr<MergeQueue> merge_queue = MergeQueue::Create(
       absl::GetFlag(FLAGS_merge_queues), absl::GetFlag(FLAGS_merge_queue_size));
@@ -99,7 +100,9 @@ int main(int argc, char** argv) {
                << llvm::toString(std::move(index_error));
     if (!absl::GetFlag(FLAGS_ignore_indexing_errors)) {
       merge_queue->Cancel();
-      std::filesystem::remove_all(std::filesystem::path(index_dir));
+      std::error_code ignored_error_code;
+      std::filesystem::remove_all(std::filesystem::path(index_dir),
+                                  ignored_error_code);
       return 1;
     }
   }
@@ -108,14 +111,19 @@ int main(int argc, char** argv) {
   auto index = merge_queue->TakeIndex();
   if (!index) {
     LOG(ERROR) << "Failed to create index";
-    std::filesystem::remove_all(std::filesystem::path(index_dir));
+    std::error_code ignored_error_code;
+    std::filesystem::remove_all(std::filesystem::path(index_dir),
+                                ignored_error_code);
     return 1;
   }
 
   if (!dry_run_regex.empty()) {
     return 0;
   }
 
+  LOG(INFO) << "copying files";
+  file_copier.CopyIndexedFiles();
+
   LOG(INFO) << "exporting index";
   auto flat_index = index->Export();