[clang-scan-deps] Expand response files before the argument adjuster

[aganea]

Previously, since response (.rsp) files weren't expanded at the very beginning of clang-scan-deps, we only parsed the command-line as provided in the Clang .cdb file. Unfortunately, when using Unreal Engine, arguments are always generated in a .rsp file (ie. /path/to/clang-cl.exe @/path/to/filename_args.rsp).

After this patch, /Fo can be parsed and added to the final command-line. Without this option, the make targets that are emitted are made up from the input file name alone. We have some cases where the same input in the project generates several output files, so we end up with duplicate make targets in the scan-deps emitted dependency file.

[llvmbot]

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Alexandre Ganea (aganea)

Changes

Previously, since response (.rsp) files weren't expanded at the very beginning of clang-scan-deps, we only parsed the command-line as provided in the Clang .cdb file. Unfortunately, when using Unreal Engine, arguments are always generated in a .rsp file (ie. /path/to/clang-cl.exe @/path/to/filename_args.rsp).

After this patch, /Fo can be parsed and added to the final command-line. Without this option, the make targets that are emitted are made up from the input file name alone. We have some cases where the same input in the project generates several output files, so we end up with duplicate make targets in the scan-deps emitted dependency file.

---

Full diff: https://github.com/llvm/llvm-project/pull/89950.diff

1 Files Affected:

• (modified) clang/tools/clang-scan-deps/ClangScanDeps.cpp (+6-1)

diff --git a/clang/tools/clang-scan-deps/ClangScanDeps.cpp b/clang/tools/clang-scan-deps/ClangScanDeps.cpp
index f42af7e330e17a..7b7f10c4be7421 100644
--- a/clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ b/clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -792,10 +792,15 @@ int clang_scan_deps_main(int argc, char **argv, const llvm::ToolContext &) {
 
   llvm::cl::PrintOptionValues();
 
+  // Expand response files in advance, so that we can "see" all the arguments
+  // when adjusting below.
+  auto ResponseExpander = expandResponseFiles(std::move(Compilations),
+                                              llvm::vfs::getRealFileSystem());
+
   // The command options are rewritten to run Clang in preprocessor only mode.
   auto AdjustingCompilations =
       std::make_unique<tooling::ArgumentsAdjustingCompilations>(
-          std::move(Compilations));
+          std::move(ResponseExpander));
   ResourceDirectoryCache ResourceDirCache;
 
   AdjustingCompilations->appendArgumentsAdjuster(

[aganea]

This is missing a test, I will add one.

[jansvoboda11]

This change makes sense to me, but I forget what's the purpose of the whole output path deducing code. Even for clang-cl command lines we should go through the driver and get a CompilerInvocation that will have the output file set (if the action does produce an output file). Shouldn't that get propagated to the dependency consumer? Maybe the issue is that the DependencyOptions we pass have no target set, since the original command line doesn't have -MD (or something like that)?

[jansvoboda11]

Maybe the issue is that the DependencyOptions we pass have no target set, since the original command line doesn't have -MD (or something like that)?

No, even then we should deduce the output path:

llvm-project/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

Lines 378 to 400 in a45eb62

	// Create the dependency collector that will collect the produced
	// dependencies.
	//
	// This also moves the existing dependency output options from the
	// invocation to the collector. The options in the invocation are reset,
	// which ensures that the compiler won't create new dependency collectors,
	// and thus won't write out the extra '.d' files to disk.
	auto Opts = std::make_unique<DependencyOutputOptions>();
	std::swap(*Opts, ScanInstance.getInvocation().getDependencyOutputOpts());
	// We need at least one -MT equivalent for the generator of make dependency
	// files to work.
	if (Opts->Targets.empty())
	Opts->Targets = {
	deduceDepTarget(ScanInstance.getFrontendOpts().OutputFile,
	ScanInstance.getFrontendOpts().Inputs)};
	Opts->IncludeSystemHeaders = true;
	switch (Format) {
	case ScanningOutputFormat::Make:
	ScanInstance.addDependencyCollector(
	std::make_shared<DependencyConsumerForwarder>(
	std::move(Opts), WorkingDirectory, Consumer));
	break;

[aganea]

The reason of this /Fo output path fiddling code is this: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L1072-L1082
It's because that highlighted code doesn't handle the clang-cl case, it doesn't consider /Fo. Probably because most people don't use -MT/-MF with clang-cl (it has to be escaped on the command-line such as /clang:-MT /clang:-MF.... I can fix it there (in Clang.cpp) instead, and remove all the related code in clang_scan_deps_main().

[jansvoboda11]

Hmm, that driver code is only executed if the dependency file was requested on the Clang command line, though. clang-scan-deps should be able to generate the dependency file even if the original command line doesn't have -MD and friends. That's why we call deduceDepTarget() in DependencyScanningWorker.cpp.

In general yes, I'd prefer to solve this wherever we have access to properly parsed command line (or CompilerInvocation) and removing the ad-hoc parsing in clang-scan-deps.

[aganea]

@jansvoboda11 PTAL.

I've added handling of /Fo in the driver, and that solves my case without the .rsp expansion in ClangScanDeps.cpp. However if we pass /E to command-lines in the CDB, we need to keep the adjuster code. This is because Driver::GetNamedOutputPath() returns "-" here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/Driver.cpp#L5871

@sylvain-audi worked around that by adding -o file.o from the adjuster callback, which happens to bypass that condition a bit earlier, here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/Driver.cpp#L5853

I'm not sure that's right either, ideally we should pass -MT from the adjuster but currently that doesn't work. This would probably need to be revisited later.

[jansvoboda11]

I tried playing around with response files and they don't play well with newlines. I think that instead of appending to the response files with echo, we should use split-file and create two separate files. That will also be more declarative and will make it obvious that one contains /c and the other /E.

[aganea]

Can you elaborate please on "I tried playing around with response files and they don't play well with newlines"?
Things were working on my end (empty lines in .rsp files). Also, Unreal Engine generates .rsp files with each argument on a new line, so if you're seeing any issues there we shall fix them.

[jansvoboda11]

I recall running %clang_cl with whatever arguments and response files this test generated and seeing Clang's argument parser hit an assertion while working with the expanded arguments.

[aganea]

@jansvoboda11 Do you see any further changes for this PR? Can I land it?

[jansvoboda11]

LGTM, thanks!