[clang][deps] Set -disable-free for module compilations

[benlangmuir]

Summary

The command-line arguments for module builds are cc1 commands, so they do not implicitly set -disable-free like a driver invocation, and Tooling will disable it for the scanning instance itself. Set -disable-free explicitly so that separate invocations for building modules will not pay for freeing memory unnecessarily.

[benlangmuir]

Attempt to fix Windows path issue in test.

[jansvoboda11]

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

338

I see the driver is adding -disable-free conditionally:

if (!C.isForDiagnostics())
  CmdArgs.push_back("-disable-free");

Does that change anything for this patch?

338

If this is always true for our purposes, is there a reason for passing this argument into DependencyScanningAction instead of just hard-coding it there?

[benlangmuir]

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

338

C.isForDiagnostics() is only true in Driver::generateCompilationDiagnostics, which didn't seem relevant to this tool.

If this is always true for our purposes, is there a reason for passing this argument into DependencyScanningAction instead of just hard-coding it there?

My thinking here was that making this an explicit option forces us to think about it if we add another way to trigger dependency scanning that doesn't use a driver invocation directly - for example, in our downstream branch experimental/cas/main, we have another path through this code that starts from a cc1 command-line, in which case I would think we should not add -disable-free unless the original command-line set it, since it's explicit in cc1. Obviously we don't need to bend over backwards to accommodate out-of-tree code here, but it seemed like a good indication this should be explicit, since it's easy to do that. WDYT?

[jansvoboda11]

LGTM!

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

338

That makes sense, thanks!

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

 
 /// A clang tool that runs the preprocessor in a mode that's optimized for
 /// dependency scanning for the given compiler invocation.
 class DependencyScanningAction : public tooling::ToolAction {
 public:
   DependencyScanningAction(
       StringRef WorkingDirectory, DependencyConsumer &Consumer,
       llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> DepFS,
-      ScanningOutputFormat Format, bool OptimizeArgs,
+      ScanningOutputFormat Format, bool OptimizeArgs, bool DisableFree,
       llvm::Optional<StringRef> ModuleName = None)
       : WorkingDirectory(WorkingDirectory), Consumer(Consumer),
         DepFS(std::move(DepFS)), Format(Format), OptimizeArgs(OptimizeArgs),
-        ModuleName(ModuleName) {}
+        DisableFree(DisableFree), ModuleName(ModuleName) {}
 
   bool runInvocation(std::shared_ptr<CompilerInvocation> Invocation,
                      FileManager *FileMgr,
                      std::shared_ptr<PCHContainerOperations> PCHContainerOps,
                      DiagnosticConsumer *DiagConsumer) override {
     // Make a deep copy of the original Clang invocation.
     CompilerInvocation OriginalInvocation(*Invocation);
+    // Restore the value of DisableFree, which may be modified by Tooling.
+    OriginalInvocation.getFrontendOpts().DisableFree = DisableFree;
 
     // Create a compiler instance to handle the actual work.
     CompilerInstance ScanInstance(std::move(PCHContainerOps));
     ScanInstance.setInvocation(std::move(Invocation));
 
     // Create the compiler's actual diagnostics engine.
     sanitizeDiagOpts(ScanInstance.getDiagnosticOpts());
     ScanInstance.createDiagnostics(DiagConsumer, /*ShouldOwnClient=*/false);
public:
   }
 
 private:
   StringRef WorkingDirectory;
   DependencyConsumer &Consumer;
   llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> DepFS;
   ScanningOutputFormat Format;
   bool OptimizeArgs;
+  bool DisableFree;
   llvm::Optional<StringRef> ModuleName;
 };
 
 } // end anonymous namespace
 
 DependencyScanningWorker::DependencyScanningWorker(
     DependencyScanningService &Service)
     : Format(Service.getFormat()), OptimizeArgs(Service.canOptimizeArgs()) {
const std::vector<std::string> &FinalCommandLine =
       ModifiedCommandLine ? *ModifiedCommandLine : CommandLine;
 
   std::vector<const char *> FinalCCommandLine(CommandLine.size(), nullptr);
   llvm::transform(CommandLine, FinalCCommandLine.begin(),
                   [](const std::string &Str) { return Str.c_str(); });
 
   return runWithDiags(CreateAndPopulateDiagOpts(FinalCCommandLine).release(),
                       [&](DiagnosticConsumer &DC, DiagnosticOptions &DiagOpts) {
+                        // DisableFree is modified by Tooling for running
+                        // in-process; preserve the original value, which is
+                        // always true for a driver invocation.
+                        bool DisableFree = true;
                         DependencyScanningAction Action(
                             WorkingDirectory, Consumer, DepFS, Format,
-                            OptimizeArgs, ModuleName);
+                            OptimizeArgs, DisableFree, ModuleName);
                         // Create an invocation that uses the underlying file
                         // system to ensure that any file system requests that
                         // are made by the driver do not go through the
                         // dependency scanning filesystem.
                         ToolInvocation Invocation(FinalCommandLine, &Action,
                                                   CurrentFiles.get(),
                                                   PCHContainerOps);
                         Invocation.setDiagnosticConsumer(&DC);
                         Invocation.setDiagnosticOptions(&DiagOpts);
                         return Invocation.run();
                       });
 }

clang/test/ClangScanDeps/modules-disable-free.c

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/compile-commands.json.in > %t/compile-commands.json
+
+// RUN: clang-scan-deps -compilation-database %t/compile-commands.json -j 1 -format experimental-full \
+// RUN:   -mode preprocess-dependency-directives -generate-modules-path-args > %t/output
+// RUN: FileCheck %s < %t/output
+
+// CHECK: "-disable-free",
+
+//--- compile-commands.json.in
+
+[{
+  "directory": "DIR",
+  "command": "clang -c DIR/main.c -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps",
+  "file": "DIR/main.c"
+}]
+
+//--- module.modulemap
+
+module A {
+  header "a.h"
+}
+
+//--- a.h
+
+void a(void);
+
+//--- main.c
+
+#include "a.h"
+void m() {
+  a();
+}