[ELF] Introduce a flag to parse response file according to windows rules

[davide]

Summary

Without this, paths in response file which contain backslashes are not escaped correctly.

example:

[davide@localhost bin]$ ./ld.lld @patatino.rsp
cannot open c:foogoopatatello: No such file or directory
[davide@localhost bin]$ cat patatino.rsp 
c:\foo\goo\patatello

[silvas]

Slight bikeshed, but Clang already has an option --rsp-quoting=windows --rsp-quoting=posix. Can we use that instead? In fact, you can probably just implement --rsp-quoting=windows by changing F<"use-windows-rsp"> to F<"rsp-quoting=windows">.

Other than that (which I don't feel strongly about) it seems fine to me.

[• rafael]

In addition to Sean suggestion, I think the default value should be
host dependent, so

• --rsp-quoting=windows -> always use windows style
• --rsp-quoting=posix -> always use posix style
• --rsp-quoting not passed, windows if running on win32 and posix otherwise.

Other than that LGTM.

Cheers,
Rafael

[ruiu]

Please upload a new patch after implementing Sean/Rafael's suggestions.

ELF/DriverUtils.cpp

81

Remove ().

[davide]

Attempt to address reviewers comments. This is pretty much what clang does.

[• rafael]

ELF/Config.h

62 ↗

(On Diff #62926)

Why do you need this to be in Config.h? It is only used in one function, no?

ELF/DriverUtils.cpp

88

you can drop the ().

[davide]

Follow clang and remove error checking.

[davide@localhost bin]$ ./clang --rsp-quoting=blah
clang-3.9: error: no input files

[davide]

Rewrote to take in account:

• Rafael's comments
• proper error checking

Also added a test. I'll try to port the error checking logic to clang once Rui is happy with this.

[• rafael]

I think this is fine with a nit, but wait for Rui to check it too.

ELF/DriverUtils.cpp

81

StringRef has startswith.

[• rafael]

Just another small issue.

ELF/DriverUtils.cpp

81

You probably want F.startswith("--rsp-quoting=") so that the following find('=') is guaranteed to find a '='.

[ruiu]

ELF/DriverUtils.cpp

79

Move new code to a new function, say getQuoteStyle, to keep this function concise.

I think we want to handle edge cases such as single quote and double quotes or with/without '='. But I think we don't want to handle that by hand. So, how about calling this->Parse twice; once for --rsp-quoting and the other for all other options.

[davide]

Rui's comments.
I think we might end up moving getString() and related helpers to an header so that they can be shared by Driver and DriverUtils eventually, but I'd like to get this in first.

[ruiu]

ELF/DriverUtils.cpp

53

Return either cl::TokenizeWindowsCommandLine or cl::TokenizeGNUCommandLine.

55–56

if (auto *Arg = Args.getLastArg(OPT_rsp_quoting)) {
  StringRef S = Arg->getValue();
  if (S != "windows" && S != "posix")
    error("invalid response file quoting: " + S);
  if (S == "windows")
    return cl::TokenizeWindowsCommandLine;
  return cl::TokenizeGNUCommandLine;
}

58–60

if (Triple(sys::getProcessTriple()).getOS() == Triple::Win32)
  return cl::TokenizeWindowsCommandLine;
return cl::TokenizeGNUCommandLine;

72–76

Please move this code inside getQuotingStyle.

[davide]

ELF/DriverUtils.cpp

72–76

This requires duplicating the declaration of MissingIndex, MissingCount, Vec and making getQuotingStyle a member function or passing this` to the function. I think it's not a very good compromise.

[silvas]

Rui, these are good cleanup suggestions, but I feel like it would be more efficient if you applied the suggested changes yourself in a follow-up commit. That way we don't have to bounce back and forth like this over style nits.

[davide]

Addressed all comments but one (I disagree). See previous comment for details.

[ruiu]

LGTM

OK, I'll make a follow-up patch for this, but I'd like to say that it is important to keep code consistent in style and taste, and code review is a way to achieve it, so I'm not nitpicking.

[davide]

In D22015#475966, @ruiu wrote:

LGTM

OK, I'll make a follow-up patch for this, but I'd like to say that it is important to keep code consistent in style and taste, and code review is a way to achieve it, so I'm not nitpicking.

In D22015#475966, @ruiu wrote:

LGTM

OK, I'll make a follow-up patch for this, but I'd like to say that it is important to keep code consistent in style and taste, and code review is a way to achieve it, so I'm not nitpicking.

For this reason some projects have an official style guide, e.g. https://www.freebsd.org/cgi/man.cgi?query=style&sektion=9

Also, please note that LLVM is not always consistent with itself, and lld is a clear example.
Two differences that come to my mind are:

1. Lto instead of LTO, Elf instead of ELF
2. Not using auto unless it's really obvious from the context

So having a full set of "rules" or recommendations somewhere will be ideal, and I hope you can contribute those as it seem you care about the most. We have codingstandards but it doesn't seem to cover everything you want.

Thanks!

[ruiu]

By "in style", I meant style in general and did not mean the documented
coding style. Code is easy to read and understand if it is written
consistently and in the same taste throughout its code base, so I want new
code looks and feels similar to the existing code. Following the documented
coding style is the minimum standard to achieve it, but it's not
everything. I think that's what we were talking about.

[davide]

committed upstream

ELF/DriverUtils.cpp

 // Driver.cpp less cluttered.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Driver.h"
 #include "Error.h"
 #include "lld/Config/Version.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/StringSaver.h"
 
 using namespace llvm;
 using namespace llvm::sys;
X1, X2, X9, X10, OPT_##ID, opt::Option::KIND##Class, X8, X7, OPT_##GROUP,  \
         OPT_##ALIAS, X6                                                        \
   },
 #include "Options.inc"
 #undef OPTION
 };
 
 ELFOptTable::ELFOptTable() : OptTable(OptInfo) {}
 
+static cl::TokenizerCallback getQuotingStyle(opt::InputArgList &Args) {
+  if (auto *Arg = Args.getLastArg(OPT_rsp_quoting)) {
+    StringRef S = Arg->getValue();
+    if (S != "windows" && S != "posix")
+      error("invalid response file quoting: " + S);
+    if (S == "windows")
+      return cl::TokenizeWindowsCommandLine;
+    return cl::TokenizeGNUCommandLine;
+  }
+   if (Triple(sys::getProcessTriple()).getOS() == Triple::Win32)
+    return cl::TokenizeWindowsCommandLine;
+  return cl::TokenizeGNUCommandLine;
+}
+
 // Parses a given list of options.
 opt::InputArgList ELFOptTable::parse(ArrayRef<const char *> Argv) {
   // Make InputArgList from string vectors.
   unsigned MissingIndex;
   unsigned MissingCount;
+  SmallVector<const char *, 256> Vec(Argv.data(), Argv.data() + Argv.size());
+
+  // We need to get the quoting style for response files before parsing all
+  // options so we parse here before and ignore all the options but
+  // --rsp-quoting.
+  opt::InputArgList Args = this->ParseArgs(Vec, MissingIndex, MissingCount);
 
   // Expand response files. '@<filename>' is replaced by the file's contents.
-  SmallVector<const char *, 256> Vec(Argv.data(), Argv.data() + Argv.size());
   StringSaver Saver(Alloc);
-  cl::ExpandResponseFiles(Saver, cl::TokenizeGNUCommandLine, Vec);
+  cl::ExpandResponseFiles(Saver, getQuotingStyle(Args), Vec);
 
   // Parse options and then do error checking.
-  opt::InputArgList Args = this->ParseArgs(Vec, MissingIndex, MissingCount);
+  Args = this->ParseArgs(Vec, MissingIndex, MissingCount);
   if (MissingCount)
     error(Twine("missing arg value for \"") + Args.getArgString(MissingIndex) +
           "\", expected " + Twine(MissingCount) +
           (MissingCount == 1 ? " argument.\n" : " arguments"));
 
   iterator_range<opt::arg_iterator> Unknowns = Args.filtered(OPT_UNKNOWN);
   for (auto *Arg : Unknowns)
     warning("warning: unknown argument: " + Arg->getSpelling());
   if (Unknowns.begin() != Unknowns.end())
     error("unknown argument(s) found");
   return Args;
 }

ELF/Options.td

 def trace_symbol : J<"trace-symbol=">, HelpText<"Trace references to symbols">;
 
 def undefined: J<"undefined=">,
   HelpText<"Force undefined symbol during linking">;
 
 def unresolved_symbols: J<"unresolved-symbols=">,
   HelpText<"Determine how to handle unresolved symbols">;
 
+def rsp_quoting: J<"rsp-quoting=">,
+  HelpText<"Quoting style for response files. Values supported: windows|posix">;
+
 def verbose: F<"verbose">, HelpText<"Verbose mode">;
 
 def version: F<"version">, HelpText<"Display the version number">;
 
 def version_script: S<"version-script">,
   HelpText<"Read a version script">;
 
 def warn_common: F<"warn-common">,

test/ELF/basic.s

 # CHECK-NEXT:     Flags [
 # CHECK-NEXT:       PF_R
 # CHECK-NEXT:       PF_W
 # CHECK-NEXT:     ]
 # CHECK-NEXT:     Alignment: 0
 # CHECK-NEXT:   }
 # CHECK-NEXT: ]
 
-# Test for the response file
+# Test for the response file (POSIX quoting style)
 # RUN: echo " -o %t2" > %t.responsefile
-# RUN: ld.lld %t @%t.responsefile
+# RUN: ld.lld %t --rsp-quoting=posix @%t.responsefile
 # RUN: llvm-readobj -file-headers -sections -program-headers -symbols %t2 \
 # RUN:   | FileCheck %s
 
+# Test for the response file (Windows quoting style)
+# RUN: echo " c:\blah\foo" > %t.responsefile
+# RUN: not ld.lld --rsp-quoting=windows %t @%t.responsefile 2>&1 | FileCheck \
+# RUN:   %s --check-prefix=WINRSP
+# WINRSP: cannot open c:\blah\foo
+
+# Test for the response file (invalid quoting style)
+# RUN: not ld.lld --rsp-quoting=patatino %t 2>&1 | FileCheck %s \
+# RUN:   --check-prefix=INVRSP
+# INVRSP: invalid response file quoting: patatino
+
 # RUN: not ld.lld %t.foo -o %t2 2>&1 | \
 # RUN:  FileCheck --check-prefix=MISSING %s
 # MISSING: cannot open {{.*}}.foo: {{[Nn]}}o such file or directory
 
 # RUN: not ld.lld -o %t2 2>&1 | \
 # RUN:  FileCheck --check-prefix=NO_INPUT %s
 # NO_INPUT: no input files.