[PATCH] D127243: [clang][deps] Make order of module dependencies deterministic

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 435237.
benlangmuir added a comment.

Removed use of std::unique_ptr in DependencyScanningTool.cpp, per review feedback.

@jansvoboda11  Note: there is another map containing `std::unique_ptr<ModuleDeps>` in `ModuleDepCollector`, but that one is required for correctness.  The problem is that there is code that forms a `ModuleDeps &` lvalue and assumes it will be correct after mutating the containing `ModularDeps` map.  For example, the signature of `addAllSubmoduleDeps` requires this.  If we want to switch to using this as a value in the map, we would need to do the lookup in the map again after each mutation. This code worked previously with `unordered_map`, because it guarantees that keys and values are not mutated by unrelated mutations of the containing map, unlike `MapVector`.  In practice, this means there is no additional overhead from the unique_ptr, because it was already doing a separate allocation for each value in the unordered_map.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127243/new/

https://reviews.llvm.org/D127243

Files:
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp


Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===================================================================
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -199,7 +199,7 @@
   MDC.Consumer.handleDependencyOutputOpts(*MDC.Opts);
 
   for (auto &&I : MDC.ModularDeps)
-    MDC.Consumer.handleModuleDependency(I.second);
+    MDC.Consumer.handleModuleDependency(*I.second);
 
   for (auto &&I : MDC.FileDeps)
     MDC.Consumer.handleFileDependency(I);
@@ -212,11 +212,12 @@
   assert(M == M->getTopLevelModule() && "Expected top level module!");
 
   // If this module has been handled already, just return its ID.
-  auto ModI = MDC.ModularDeps.insert({M, ModuleDeps{}});
+  auto ModI = MDC.ModularDeps.insert({M, nullptr});
   if (!ModI.second)
-    return ModI.first->second.ID;
+    return ModI.first->second->ID;
 
-  ModuleDeps &MD = ModI.first->second;
+  ModI.first->second = std::make_unique<ModuleDeps>();
+  ModuleDeps &MD = *ModI.first->second;
 
   MD.ID.ModuleName = M->getFullModuleName();
   MD.ImportedByMainFile = DirectModularDeps.contains(M);
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
===================================================================
--- clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
@@ -176,7 +176,7 @@
   private:
     std::vector<std::string> Dependencies;
     std::vector<PrebuiltModuleDep> PrebuiltModuleDeps;
-    std::map<std::string, ModuleDeps> ClangModuleDeps;
+    llvm::MapVector<std::string, ModuleDeps, llvm::StringMap<unsigned>> ClangModuleDeps;
     std::string ContextHash;
     std::vector<std::string> OutputPaths;
     const llvm::StringSet<> &AlreadySeen;
Index: clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
===================================================================
--- clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
+++ clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
@@ -149,9 +149,9 @@
   /// The parent dependency collector.
   ModuleDepCollector &MDC;
   /// Working set of direct modular dependencies.
-  llvm::DenseSet<const Module *> DirectModularDeps;
+  llvm::SetVector<const Module *> DirectModularDeps;
   /// Working set of direct modular dependencies that have already been built.
-  llvm::DenseSet<const Module *> DirectPrebuiltModularDeps;
+  llvm::SetVector<const Module *> DirectPrebuiltModularDeps;
 
   void handleImport(const Module *Imported);
 
@@ -199,7 +199,7 @@
   /// textually included header files.
   std::vector<std::string> FileDeps;
   /// Direct and transitive modular dependencies of the main source file.
-  std::unordered_map<const Module *, ModuleDeps> ModularDeps;
+  llvm::MapVector<const Module *, std::unique_ptr<ModuleDeps>> ModularDeps;
   /// Options that control the dependency output generation.
   std::unique_ptr<DependencyOutputOptions> Opts;
   /// The original Clang invocation passed to dependency scanner.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D127243.435237.patch
Type: text/x-patch
Size: 3117 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220608/153a3359/attachment.bin>