[clang] ed07fe7 - [clang][modules][deps] Transitive module maps are not affecting

[Jan Svoboda via cfe-commits]

Author: Jan Svoboda
Date: 2022-12-01T20:16:17-08:00
New Revision: ed07fe71d7bbdd13b113a2073f47e701e41a1001

URL: https://github.com/llvm/llvm-project/commit/ed07fe71d7bbdd13b113a2073f47e701e41a1001
DIFF: https://github.com/llvm/llvm-project/commit/ed07fe71d7bbdd13b113a2073f47e701e41a1001.diff

LOG: [clang][modules][deps] Transitive module maps are not affecting

Currently, the algorithm for gathering affecting module maps includes those defining transitive dependencies. This seems like an over-approximation, since those don't change the semantics of current module build.

(With this patch, `ModulesToProcess` only ever holds modules whose headers will be serialized into the current PCM.)

Reviewed By: Bigcheese

Differential Revision: https://reviews.llvm.org/D137197

Added: 
    clang/test/ClangScanDeps/modules-transitive.c

Modified: 
    clang/lib/Serialization/ASTWriter.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index f1dab7e051e2..80930357a6d1 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -190,30 +190,20 @@ std::set<const FileEntry *> GetAffectingModuleMaps(const HeaderSearch &HS,
     }
   }
 
-  while (!ModulesToProcess.empty()) {
-    auto *CurrentModule = ModulesToProcess.pop_back_val();
-    ProcessedModules.insert(CurrentModule);
+  const ModuleMap &MM = HS.getModuleMap();
 
-    Optional<FileEntryRef> ModuleMapFile =
-        HS.getModuleMap().getModuleMapFileForUniquing(CurrentModule);
-    if (!ModuleMapFile) {
-      continue;
-    }
-
-    ModuleMaps.insert(*ModuleMapFile);
-
-    for (auto *ImportedModule : (CurrentModule)->Imports) {
-      if (!ImportedModule ||
-          ProcessedModules.find(ImportedModule) != ProcessedModules.end()) {
-        continue;
-      }
-      ModulesToProcess.push_back(ImportedModule);
-    }
+  auto ProcessModuleOnce = [&](const Module *Mod) {
+    if (ProcessedModules.insert(Mod).second)
+      if (auto ModuleMapFile = MM.getModuleMapFileForUniquing(Mod))
+        ModuleMaps.insert(*ModuleMapFile);
+  };
 
+  for (const Module *CurrentModule : ModulesToProcess) {
+    ProcessModuleOnce(CurrentModule);
+    for (const Module *ImportedModule : CurrentModule->Imports)
+      ProcessModuleOnce(ImportedModule);
     for (const Module *UndeclaredModule : CurrentModule->UndeclaredUses)
-      if (UndeclaredModule &&
-          ProcessedModules.find(UndeclaredModule) == ProcessedModules.end())
-        ModulesToProcess.push_back(UndeclaredModule);
+      ProcessModuleOnce(UndeclaredModule);
   }
 
   return ModuleMaps;

diff  --git a/clang/test/ClangScanDeps/modules-transitive.c b/clang/test/ClangScanDeps/modules-transitive.c
new file mode 100644
index 000000000000..9ae1554d9811
--- /dev/null
+++ b/clang/test/ClangScanDeps/modules-transitive.c
@@ -0,0 +1,58 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- tu.m
+#include "first.h"
+
+//--- first/module.modulemap
+module first { header "first.h" }
+//--- first/first.h
+#include "second.h"
+
+//--- second/module.modulemap
+module second { header "second.h" }
+//--- second/second.h
+#include "third.h"
+
+//--- third/module.modulemap
+module third { header "third.h" }
+//--- third/third.h
+// empty
+
+//--- cdb.json.template
+[{
+  "file": "DIR/tu.c",
+  "directory": "DIR",
+  "command": "clang -I DIR/first -I DIR/second -I DIR/third -fmodules -fmodules-cache-path=DIR/cache -c DIR/tu.m -o DIR/tu.o"
+}]
+
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/result.json
+// RUN: cat %t/result.json | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t
+
+// CHECK:      {
+// CHECK-NEXT:   "modules": [
+// CHECK-NEXT:     {
+// CHECK-NEXT:       "clang-module-deps": [
+// CHECK-NEXT:         {
+// CHECK-NEXT:           "context-hash": "{{.*}}",
+// CHECK-NEXT:           "module-name": "second"
+// CHECK-NEXT:         }
+// CHECK-NEXT:       ],
+// CHECK-NEXT:       "clang-modulemap-file": "[[PREFIX]]/first/module.modulemap",
+// CHECK-NEXT:       "command-line": [
+// CHECK-NEXT:         "-cc1",
+// CHECK-NOT:          "-fmodule-map-file=[[PREFIX]]/third/module.modulemap"
+// CHECK:              "-fmodule-map-file=[[PREFIX]]/second/module.modulemap"
+// CHECK-NOT:          "-fmodule-map-file=[[PREFIX]]/third/module.modulemap"
+// CHECK:            ],
+// CHECK-NEXT:       "context-hash": "{{.*}}",
+// CHECK-NEXT:       "file-deps": [
+// CHECK-NEXT:         "[[PREFIX]]/first/first.h"
+// CHECK-NEXT:         "[[PREFIX]]/first/module.modulemap"
+// CHECK-NEXT:         "[[PREFIX]]/second/module.modulemap"
+// CHECK-NEXT:       ],
+// CHECK-NEXT:       "name": "first"
+// CHECK-NEXT:     }
+// CHECK:        ]
+// CHECK:      }