[clang][deps] Remove '-fmodules-cache-path=' arguments

[jansvoboda11]

Summary

With explicit modules build, the '-fmodules-cache-path=' argument is unused.

This patch removes the argument to avoid warnings or errors (with '-Werror') stemming from that.

Depends on D118915.

[dexonsmith]

clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp

36

Is that the only one to remove? (asking because you dropped the FIXME, which suggested it might only be one example)

Also, could this just be an assertion, since makeInvocationForModuleBuildWithoutPaths() is already clearing things?

• In other words, why do you need this in two places?
• And if you do need it in both, does the test cover both, or should there be another test or RUN line for the other code path?

[jansvoboda11]

clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp

36

Ah, there are some others (e.g. -fmodules-prune-interval=, -fmodules-prune-after=) so you're right dropping the FIXME might be a bit hasty. I'll update the fixme instead of removing it.

This cannot be an assertion unfortunately. makeInvocationForModuleBuildWithoutPaths() is clearing the modules cache path in the -cc1 command lines for building modules. This however, is dealing with the driver command line for building the original translation unit, which doesn't go through the CompilerInvocation machinery at all. We're relying on textual manipulation here at this moment. (TU command lines are driver command lines that might expand into multiple commands and therefore are not guaranteed to be representable by single CompilerInvocation.)

The test I touched covers only this change to the driver command line for building the TU. The error (-Wunused-command-line-argument) I test is only produced by the driver. This means the other change (clearing modules cache path in -cc1 CompilerInvocation) is just a preventive measure, not necessary to make the updated test pass and not testable.

[jansvoboda11]

Update TODO instead of removing it

[dexonsmith]

LGTM!

clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp

 std::vector<std::string>
 FullDependencies::getCommandLineWithoutModulePaths() const {
   std::vector<std::string> Args = OriginalCommandLine;
 
   std::vector<std::string> AdditionalArgs =
       getAdditionalArgsWithoutModulePaths();
   Args.insert(Args.end(), AdditionalArgs.begin(), AdditionalArgs.end());
 
-  // TODO: Filter out implicit modules leftovers (e.g. "-fmodules-cache-path=").
+  // This argument is unused in explicit compiles.
+  llvm::erase_if(Args, [](const std::string &Arg) {
+    return Arg.find("-fmodules-cache-path=") == 0;
+  });
+
+  // TODO: Filter out the remaining implicit modules leftovers
+  // (e.g. "-fmodules-prune-interval=" or "-fmodules-prune-after=").
 
   return Args;
 }
 
 std::vector<std::string>
 FullDependencies::getAdditionalArgsWithoutModulePaths() const {
   std::vector<std::string> Args{
       "-fno-implicit-modules",

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

CompilerInvocation ModuleDepCollector::makeInvocationForModuleBuildWithoutPaths(
   CI.getCodeGenOpts().DwarfDebugFlags.clear();
 
   CI.getFrontendOpts().ProgramAction = frontend::GenerateModule;
   CI.getLangOpts()->ModuleName = Deps.ID.ModuleName;
   CI.getFrontendOpts().IsSystemModule = Deps.IsSystem;
 
   CI.getLangOpts()->ImplicitModules = false;
   CI.getHeaderSearchOpts().ImplicitModuleMaps = false;
+  CI.getHeaderSearchOpts().ModuleCachePath.clear();
 
   // Report the prebuilt modules this module uses.
   for (const auto &PrebuiltModule : Deps.PrebuiltModuleDeps)
     CI.getFrontendOpts().ModuleFiles.push_back(PrebuiltModule.PCMFile);
 
   CI.getFrontendOpts().ModuleMapFiles = Deps.ModuleMapFileDeps;
 
   Optimize(CI);

clang/test/ClangScanDeps/modules-inferred-explicit-build.m

 // RUN: rm -rf %t.dir
 // RUN: rm -rf %t.cdb
 // RUN: mkdir -p %t.dir
 // RUN: cp %s %t.dir/modules_cdb_input.cpp
 // RUN: sed -e "s|DIR|%/t.dir|g" -e "s|FRAMEWORKS|%/S/Inputs/frameworks|g" -e "s|-E|-x objective-c -E|g" \
 // RUN:   %S/Inputs/modules_inferred_cdb.json > %t.cdb
 //
 // RUN: clang-scan-deps -compilation-database %t.cdb -j 1 -format experimental-full \
 // RUN:   -mode preprocess-minimized-sources -generate-modules-path-args > %t.db
 // RUN: %python %S/../../utils/module-deps-to-rsp.py %t.db --module-name=Inferred > %t.inferred.cc1.rsp
 // RUN: %python %S/../../utils/module-deps-to-rsp.py %t.db --module-name=System > %t.system.cc1.rsp
 // RUN: %python %S/../../utils/module-deps-to-rsp.py %t.db --tu-index=0 > %t.tu.rsp
 // RUN: %clang @%t.inferred.cc1.rsp -pedantic -Werror
 // RUN: %clang @%t.system.cc1.rsp -pedantic -Werror
-// RUN: %clang @%t.tu.rsp -pedantic -Werror -Wno-unused-command-line-argument
+// RUN: %clang @%t.tu.rsp -pedantic -Werror
 
 #include <Inferred/Inferred.h>
 #include <System/System.h>
 
 inferred a = bigger_than_int;

clang/tools/clang-scan-deps/ClangScanDeps.cpp

private:
 
   /// Construct a path for the explicitly built PCM.
   std::string constructPCMPath(ModuleID MID) const {
     auto MDIt = Modules.find(IndexedModuleID{MID, 0});
     assert(MDIt != Modules.end());
     const ModuleDeps &MD = MDIt->second;
 
     StringRef Filename = llvm::sys::path::filename(MD.ImplicitModulePCMPath);
+    StringRef ModuleCachePath = llvm::sys::path::parent_path(
+        llvm::sys::path::parent_path(MD.ImplicitModulePCMPath));
 
-    SmallString<256> ExplicitPCMPath(
-        !ModuleFilesDir.empty()
-            ? ModuleFilesDir
-            : MD.BuildInvocation.getHeaderSearchOpts().ModuleCachePath);
+    SmallString<256> ExplicitPCMPath(!ModuleFilesDir.empty() ? ModuleFilesDir
+                                                             : ModuleCachePath);
     llvm::sys::path::append(ExplicitPCMPath, MD.ID.ContextHash, Filename);
     return std::string(ExplicitPCMPath);
   }
 
   struct IndexedModuleID {
     ModuleID ID;
     mutable size_t InputIndex;