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

[jansvoboda11]

Summary

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.)

[jansvoboda11]

Extract lambda, replace while with for

[Bigcheese]

Yeah, I agree with this. lgtm.

clang/lib/Serialization/ASTWriter.cpp

for (unsigned UID = 0, LastUID = FilesByUID.size(); UID != LastUID; ++UID) {
 
     for (const auto &KH : HS.findAllModulesForHeader(File)) {
       if (!KH.getModule())
         continue;
       ModulesToProcess.push_back(KH.getModule());
     }
   }
 
-  while (!ModulesToProcess.empty()) {
-    auto *CurrentModule = ModulesToProcess.pop_back_val();
-    ProcessedModules.insert(CurrentModule);
-
-    Optional<FileEntryRef> ModuleMapFile =
-        HS.getModuleMap().getModuleMapFileForUniquing(CurrentModule);
-    if (!ModuleMapFile) {
-      continue;
-    }
+  const ModuleMap &MM = HS.getModuleMap();
 
+  auto ProcessModuleOnce = [&](const Module *Mod) {
+    if (ProcessedModules.insert(Mod).second)
+      if (auto ModuleMapFile = MM.getModuleMapFileForUniquing(Mod))
         ModuleMaps.insert(*ModuleMapFile);
+  };
 
-    for (auto *ImportedModule : (CurrentModule)->Imports) {
-      if (!ImportedModule ||
-          ProcessedModules.find(ImportedModule) != ProcessedModules.end()) {
-        continue;
-      }
-      ModulesToProcess.push_back(ImportedModule);
-    }
-
+  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;
 }
 
 class ASTTypeWriter {
   ASTWriter &Writer;
   ASTWriter::RecordData Record;

clang/test/ClangScanDeps/modules-transitive.c

+// 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:      }