[PATCH] D120465: [clang][deps] Generate necessary "-fmodule-map-file=" arguments, disable implicit module maps

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:112
 
 void dependencies::detail::collectPCMAndModuleMapPaths(
     llvm::ArrayRef<ModuleID> Modules,
----------------
dexonsmith wrote:
> Should this be renamed?
Yes, that would make sense.


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:125-126
           PCMPaths.push_back(LookupPCMPath(MID).str());
-          if (!M.ClangModuleMapFile.empty())
-            ModMapPaths.push_back(M.ClangModuleMapFile);
         }
----------------
dexonsmith wrote:
> Can you clarify why this is safe to remove, even though sometimes we do need to add module map files?
This collects the module map files of transitive dependencies. We used it to generate `-fmodule-map-file=` arguments for builds of modules.

However, with explicit modules, it's not necessary to provide `-fmodule-map-file=` arguments of (transitive) dependencies at all. The module map semantics are basically serialized in the PCM files themselves. That's why this is safe to remove.

The new code below handles a special case (`[no_undeclared_includes]`) where we still need to generate some `-fmodule-map-file=` arguments. Semantics of such module maps will not be found in any PCM, since we don't import the modules they describe.

Note that caller of this function used to pass `FrontendOptions::ModuleMapFiles` member as `ModMapPaths`. The member is now being initialized in the new code below.


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:269-273
+  // However, some module maps loaded implicitly during the dependency scan can
+  // describe anti-dependencies. That happens when the current module is marked
+  // as '[no_undeclared_includes]', doesn't 'use' module from such module map,
+  // but tries to import it anyway. We need to tell the explicit build about
+  // such module map for it to have the same semantics as the implicit build.
----------------
dexonsmith wrote:
> Is there another long-term solution to this that could be pointed at with a FIXME? E.g., could the module map be encoded redundantly here? If so, what else would need to change to make that okay?
I'm not sure I understand why this would warrant "long-term solution" or a FIXME. This code handles an existing feature that just happens to be a corner case from the dependency scanning point of view. (You can read up on the feature [[ https://clang.llvm.org/docs/Modules.html | here ]].)

What do you mean by encoding the module map redundantly?


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:275-278
+    // We don't have a good way to determine which module map described the
+    // anti-dependency (let alone what's the corresponding top-level module
+    // map). We simply specify all the module maps in the order they were loaded
+    // during the implicit build during scan.
----------------
dexonsmith wrote:
> Is there a FIXME to leave behind for tracking the anti-dependencies?
That's a good idea. I left a FIXME in D120464 to specifically serialize the information on anti-dependency introducing module maps here: D120464, but it would make sense to mention it here too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120465