[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

61

Remove ().

[davide]

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

[• rafael]

ELF/Config.h

62

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

ELF/DriverUtils.cpp

67

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

60

StringRef has startswith.

[• rafael]

Just another small issue.

ELF/DriverUtils.cpp

60

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

[ruiu]

ELF/DriverUtils.cpp

58

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

52

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

54–55

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;
}

57–59

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

57–61

Please move this code inside getQuotingStyle.

[davide]

ELF/DriverUtils.cpp

57–61

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/Config.h

struct Configuration {
   llvm::StringRef DynamicLinker;
   llvm::StringRef Entry;
   llvm::StringRef Emulation;
   llvm::StringRef Fini;
   llvm::StringRef Init;
   llvm::StringRef LtoAAPipeline;
   llvm::StringRef LtoNewPmPasses;
   llvm::StringRef OutputFile;
   llvm::StringRef SoName;
   llvm::StringRef Sysroot;
   llvm::StringSet<> TraceSymbol;
   std::string RPath;
   std::vector<Version> SymbolVersions;
   std::vector<llvm::StringRef> DynamicList;
   std::vector<llvm::StringRef> SearchPaths;
   std::vector<llvm::StringRef> Undefined;
   std::vector<llvm::StringRef> VersionScriptGlobals;
struct Configuration {
   bool SaveTemps;
   bool Shared;
   bool Static = false;
   bool StripAll;
   bool StripDebug;
   bool SysvHash = true;
   bool Threads;
   bool Trace;
+  bool UseWindowsRsp;
   bool Verbose;
   bool VersionScriptGlobalByDefault = true;
   bool WarnCommon;
   bool ZCombreloc;
   bool ZExecStack;
   bool ZNodelete;
   bool ZNow;
   bool ZOrigin;

ELF/Driver.cpp

void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->PrintGcSections = Args.hasArg(OPT_print_gc_sections);
   Config->Relocatable = Args.hasArg(OPT_relocatable);
   Config->SaveTemps = Args.hasArg(OPT_save_temps);
   Config->Shared = Args.hasArg(OPT_shared);
   Config->StripAll = Args.hasArg(OPT_strip_all);
   Config->StripDebug = Args.hasArg(OPT_strip_debug);
   Config->Threads = Args.hasArg(OPT_threads);
   Config->Trace = Args.hasArg(OPT_trace);
+  Config->UseWindowsRsp = Args.hasArg(OPT_use_windows_rsp);
   Config->Verbose = Args.hasArg(OPT_verbose);
   Config->WarnCommon = Args.hasArg(OPT_warn_common);
 
   Config->DynamicLinker = getString(Args, OPT_dynamic_linker);
   Config->Entry = getString(Args, OPT_entry);
   Config->Fini = getString(Args, OPT_fini, "_fini");
   Config->Init = getString(Args, OPT_init, "_init");
   Config->LtoAAPipeline = getString(Args, OPT_lto_aa_pipeline);

ELF/DriverUtils.cpp

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) {}
 
 // Parses a given list of options.
 opt::InputArgList ELFOptTable::parse(ArrayRef<const char *> Argv) {
   // Make InputArgList from string vectors.
   unsigned MissingIndex;
   unsigned 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::TokenizerCallback TC = (Config->UseWindowsRsp)
+                                 ? cl::TokenizeWindowsCommandLine
+                                 : cl::TokenizeGNUCommandLine;
+  cl::ExpandResponseFiles(Saver, TC, Vec);
 
   // Parse options and then do error checking.
   opt::InputArgList 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());

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 use_windows_rsp: F<"use-windows-rsp">,
+  HelpText<"Use Windows rules for parsing response files">;
+
 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: ]
 
 # Test for the response file
 # RUN: echo " -o %t2" > %t.responsefile
 # RUN: ld.lld %t @%t.responsefile
 # RUN: llvm-readobj -file-headers -sections -program-headers -symbols %t2 \
 # RUN:   | FileCheck %s
 
+# Test for response file (Windows rules)
+# RUN: echo " c:\blah\foo" > %t.responsefile
+# RUN: not ld.lld --use-windows-rsp %t @%t.responsefile 2>&1 | FileCheck %s --check-prefix=WINRSP
+# WINRSP: cannot open c:\blah\foo
+
 # 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.