[LLD] [Driver] Look for -m in response files as well

[mstorsjo]

Summary

This should fix PR42135.

[rnk]

For links that use response files (i.e. most of them) this effectively leaks a second copy of the entire response file. Every argument needs to be put in the string saver so it can be null terminated. Response files tend to be large enough that I'd prefer it if we didn't do that.

One way we could address this is to try to calculate the flavor twice:

• First, use the unexpanded arguments to guess the flavor
• Use the guessed flavor to expand response files: Windows tokenization for lld-link, GNU for everyone else
• Re-parse the flavor using the expanded arguments, and call the appropriate entry point with those arguments
• Remove existing rsp expansion code

This is kind of what clang does:
https://github.com/llvm/llvm-project/blob/master/clang/tools/driver/driver.cpp#L334

[mstorsjo]

In D63024#1534763, @rnk wrote:

For links that use response files (i.e. most of them) this effectively leaks a second copy of the entire response file. Every argument needs to be put in the string saver so it can be null terminated. Response files tend to be large enough that I'd prefer it if we didn't do that.

One way we could address this is to try to calculate the flavor twice:

• First, use the unexpanded arguments to guess the flavor
• Use the guessed flavor to expand response files: Windows tokenization for lld-link, GNU for everyone else
• Re-parse the flavor using the expanded arguments, and call the appropriate entry point with those arguments
• Remove existing rsp expansion code

This sounds sensible in general. But there's one gotcha that I'm a bit unsure about how to handle best; the COFF and ELF linkers support the option rsp-quoting (-rsp-quoting val, --rsp-quoting val, -rsp-quoting=val or --rsp-quoting=val for the ELF one, --rsp-quoting=val for COFF). So after getting the unexpanded arguments and guessing flavor based on that, we'd need to interpret the arguments to handle the rsp-quoting option. For the flavor option, we have only one spelling and it's handled without the tablegen options. Should I try to do some similar handwritten parsing of rsp-quoting for the ELF syntax (which is a supserset of the variants accepted for COFF) and use that expanding response files at that stage?

For the -flavor option, we remove it from the args we pass on to the individual linkers - should we do that for rsp-quoting as well , or keep it, along with the rsp-quoting options in the individual linkers for printing in the help listings (even if they aren't actually used at that level)?

[ruiu]

Do you have to support --rsp-quoting for the use case that you want to support? I believe if you are using MinGW, you are always using Unix-style quoting in response files, so if the following two conditions are met, you can conclude that a user is trying to use mingw driver, can't you?

1. Response files are correctly expanded using the Unix-style quoting scheme
2. There's a -m foo where foo implies mingw

[mstorsjo]

In D63024#1535891, @ruiu wrote:

Do you have to support --rsp-quoting for the use case that you want to support?

Not for my use case, no. But Reid wanted to move all expansion of response files out from the individual linkers here, to avoid reading (and allocating memory for) the (potentially large) response file twice. And in that case, we must handle --rsp-quoting perfectly here.

I believe if you are using MinGW, you are always using Unix-style quoting in response files

At least the default in ELF/DriverUtils.cpp is to use windows quoting style, if the linker binary runs on windows, FWIW.

[ruiu]

In D63024#1535918, @mstorsjo wrote:

In D63024#1535891, @ruiu wrote:

Do you have to support --rsp-quoting for the use case that you want to support?

Not for my use case, no. But Reid wanted to move all expansion of response files out from the individual linkers here, to avoid reading (and allocating memory for) the (potentially large) response file twice. And in that case, we must handle --rsp-quoting perfectly here.

Each flavor's link() function is a public interface, and each function needs to do what the command would do when the same argument strings are given. That means we need to expand response files when the linker is invoked by lld::{coff,elf,wasm,..}::link().

I believe if you are using MinGW, you are always using Unix-style quoting in response files

At least the default in ELF/DriverUtils.cpp is to use windows quoting style, if the linker binary runs on windows, FWIW.

IIRC lld works that way because that's what ninja does, but I may be wrong.

[mstorsjo]

In D63024#1535947, @ruiu wrote:

In D63024#1535918, @mstorsjo wrote:

In D63024#1535891, @ruiu wrote:

Do you have to support --rsp-quoting for the use case that you want to support?

Not for my use case, no. But Reid wanted to move all expansion of response files out from the individual linkers here, to avoid reading (and allocating memory for) the (potentially large) response file twice. And in that case, we must handle --rsp-quoting perfectly here.

Each flavor's link() function is a public interface, and each function needs to do what the command would do when the same argument strings are given. That means we need to expand response files when the linker is invoked by lld::{coff,elf,wasm,..}::link().

Ah, right. So we at least must keep the handling there as well.

What's your opinion on Reid's performance/memory concern? Do you agree we should expand here and pass the expanded args to the individual linkers, to avoid double expansion? And how should we handle rsp-quoting then?

Or do you suggest to ignore the performance concern and expand once here just for peeking at the -m flag, and let the individual linkers expand it again, properly - like this patch?

[ruiu]

I'm not particularly worried about the performance impact of doubly-expanding response files. If it is proved to be too slow, we need to do something, but until then I wouldn't optimize it.

But at least we can expand response files only for GNU flavor. For COFF and WASM linkers, we don't have to expand them at this stage.

[mstorsjo]

In D63024#1535958, @ruiu wrote:

I'm not particularly worried about the performance impact of doubly-expanding response files. If it is proved to be too slow, we need to do something, but until then I wouldn't optimize it.

But at least we can expand response files only for GNU flavor. For COFF and WASM linkers, we don't have to expand them at this stage.

Ok, that makes sense. Then we can't handle the -flavor option in a response file, but there's no need for that either.

I'll update the patch that way. And I think I'll default the quoting style to the host platform native (like the ELF linker); I think that'll cause fewer issues down the line. (Currently the MinGW driver doesn't handle response files at all, but they fall through to the COFF linker.)

[mstorsjo]

Localized the extra expansion to the ELF case, use the host platform native quoting style.

[ruiu]

tools/lld/lld.cpp

141–143 ↗

(On Diff #203797)

If you factor this piece of code to a new function, we can discard SmallVector, saving a small amount of memory.

145 ↗

(On Diff #203797)

Is it intentional that you call mingw::link with ExpandedArgs?

[mstorsjo]

tools/lld/lld.cpp

141–143 ↗

(On Diff #203797)

Could do that, but...

145 ↗

(On Diff #203797)

The MinGW driver does no expansion of its own right now; I guess it would be better to do that there instead of passing the expanded args here - for consistency?

[ruiu]

tools/lld/lld.cpp

145 ↗

(On Diff #203797)

Yeah I think so. lld::mingw::link's contract should be the same as other link() functions.

[mstorsjo]

Expand only in isPETarget, expand in the mingw driver as well.

[mstorsjo]

Avoid expanding unless no -m option was found.

[ruiu]

LGTM

tools/lld/lld.cpp

89 ↗

(On Diff #203807)

I'd avoid function overloading if I can simply give a different name. At first this looked like a recursive function call.

[rnk]

lgtm

tools/lld/lld.cpp

82 ↗

(On Diff #203807)

Typo on expand

lld/trunk/MinGW/Driver.cpp

 // ld/MinGW implements a few GNU-specific features. Such features are directly
 // implemented in lld/COFF and enabled only when the linker is running in MinGW
 // mode.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lld/Common/Driver.h"
 #include "lld/Common/ErrorHandler.h"
+#include "lld/Common/Memory.h"
 #include "lld/Common/Version.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 
 #if !defined(_MSC_VER) && !defined(__MINGW32__)

 
 static void printHelp(const char *Argv0) {
   MinGWOptTable().PrintHelp(
       outs(), (std::string(Argv0) + " [options] file...").c_str(), "lld",
       false /*ShowHidden*/, true /*ShowAllAliases*/);
   outs() << "\n";
 }
 
+static cl::TokenizerCallback getQuotingStyle() {
+  if (Triple(sys::getProcessTriple()).getOS() == Triple::Win32)
+    return cl::TokenizeWindowsCommandLine;
+  return cl::TokenizeGNUCommandLine;
+}
+
 opt::InputArgList MinGWOptTable::parse(ArrayRef<const char *> Argv) {
   unsigned MissingIndex;
   unsigned MissingCount;
 
   SmallVector<const char *, 256> Vec(Argv.data(), Argv.data() + Argv.size());
+  cl::ExpandResponseFiles(Saver, getQuotingStyle(), Vec);
   opt::InputArgList Args = this->ParseArgs(Vec, MissingIndex, MissingCount);
 
   if (MissingCount)
     fatal(StringRef(Args.getArgString(MissingIndex)) + ": missing argument");
   for (auto *Arg : Args.filtered(OPT_UNKNOWN))
     fatal("unknown argument: " + Arg->getSpelling());
   return Args;
 }

lld/trunk/test/MinGW/driver.test

 RUN: ld.lld -### foo.o -m i386pe | FileCheck -check-prefix=X86 %s
 X86:      -out:a.exe
 X86-SAME: -machine:x86
 X86-SAME: -alternatename:__image_base__=___ImageBase
 X86-SAME: foo.o
 
+RUN: echo "-### foo.o -m i386pe" > %t.rsp
+RUN: ld.lld @%t.rsp | FileCheck -check-prefix=X86 %s
+
 RUN: ld.lld -### foo.o -m i386pep | FileCheck -check-prefix=X64 %s
 X64:      -out:a.exe
 X64-SAME: -machine:x64
 X64-SAME: -alternatename:__image_base__=__ImageBase
 X64-SAME: foo.o
 
 RUN: ld.lld -### foo.o -m thumb2pe | FileCheck -check-prefix=ARM %s
 ARM:      -out:a.exe

lld/trunk/tools/lld/lld.cpp

 //  - ld-wasm:   WebAssembly
 //
 // lld can be invoked as "lld" along with "-flavor" option. This is for
 // backward compatibility and not recommended.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lld/Common/Driver.h"
+#include "lld/Common/Memory.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/ADT/Twine.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Path.h"
 #include <cstdlib>
 
 using namespace lld;
 using namespace llvm;
 using namespace llvm::sys;
 
static Flavor getFlavor(StringRef S) {
   return StringSwitch<Flavor>(S)
       .CasesLower("ld", "ld.lld", "gnu", Gnu)
       .CasesLower("wasm", "ld-wasm", Wasm)
       .CaseLower("link", WinLink)
       .CasesLower("ld64", "ld64.lld", "darwin", Darwin)
       .Default(Invalid);
 }
 
-static bool isPETarget(const std::vector<const char *> &V) {
+static cl::TokenizerCallback getDefaultQuotingStyle() {
+  if (Triple(sys::getProcessTriple()).getOS() == Triple::Win32)
+    return cl::TokenizeWindowsCommandLine;
+  return cl::TokenizeGNUCommandLine;
+}
+
+static bool isPETargetName(StringRef S) {
+  return S == "i386pe" || S == "i386pep" || S == "thumb2pe" || S == "arm64pe";
+}
+
+static bool isPETarget(std::vector<const char *> &V) {
   for (auto It = V.begin(); It + 1 != V.end(); ++It) {
     if (StringRef(*It) != "-m")
       continue;
-    StringRef S = *(It + 1);
-    return S == "i386pe" || S == "i386pep" || S == "thumb2pe" || S == "arm64pe";
+    return isPETargetName(*(It + 1));
+  }
+  // Expand response files (arguments in the form of @<filename>)
+  // to allow detecting the -m argument from arguments in them.
+  SmallVector<const char *, 256> ExpandedArgs(V.data(), V.data() + V.size());
+  cl::ExpandResponseFiles(Saver, getDefaultQuotingStyle(), ExpandedArgs);
+  for (auto It = ExpandedArgs.begin(); It + 1 != ExpandedArgs.end(); ++It) {
+    if (StringRef(*It) != "-m")
+      continue;
+    return isPETargetName(*(It + 1));
   }
   return false;
 }
 
 static Flavor parseProgname(StringRef Progname) {
 #if __APPLE__
   // Use Darwin driver for "ld" on Darwin.
   if (Progname == "ld")