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

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir created this revision.
benlangmuir added reviewers: jansvoboda11, Bigcheese.
Herald added a subscriber: mgrang.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This fixes the underlying module dependencies, which had a non-deterministic order, which was also visible in the order of calls to DependencyConsumer methods. This was not directly observable in the clang-scan-deps utility, because it was previously seeing a sorted order from std::map in DependencyScanningTool. However, the underlying API previously created a likely issue for any other clients. Note: if you only apply the change from DependencyScanningTool, you can see the issue in clang-scan-deps, and existing tests will fail non-deterministicaly.


Repository:
  rG LLVM Github Monorepo

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
@@ -133,7 +133,9 @@
     }
 
     void handleModuleDependency(ModuleDeps MD) override {
-      ClangModuleDeps[MD.ID.ContextHash + MD.ID.ModuleName] = std::move(MD);
+      ClangModuleDeps.insert(
+          std::make_pair(MD.ID.ContextHash + MD.ID.ModuleName,
+                         std::make_unique<ModuleDeps>(std::move(MD))));
     }
 
     void handleContextHash(std::string Hash) override {
@@ -152,7 +154,7 @@
       FD.FileDeps.assign(Dependencies.begin(), Dependencies.end());
 
       for (auto &&M : ClangModuleDeps) {
-        auto &MD = M.second;
+        auto &MD = *M.second;
         if (MD.ImportedByMainFile)
           FD.ClangModuleDeps.push_back(MD.ID);
       }
@@ -166,7 +168,7 @@
         //   we've already seen.
         if (AlreadySeen.count(M.first))
           continue;
-        FDR.DiscoveredModules.push_back(std::move(M.second));
+        FDR.DiscoveredModules.push_back(std::move(*M.second));
       }
 
       FDR.FullDeps = std::move(FD);
@@ -176,7 +178,9 @@
   private:
     std::vector<std::string> Dependencies;
     std::vector<PrebuiltModuleDep> PrebuiltModuleDeps;
-    std::map<std::string, ModuleDeps> ClangModuleDeps;
+    llvm::MapVector<std::string, std::unique_ptr<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.434913.patch
Type: text/x-patch
Size: 4142 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220607/a8ad933a/attachment-0001.bin>