Page MenuHomePhabricator

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

Authored by davide on Jul 5 2016, 1:49 PM.

Details

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

Diff Detail

Event Timeline

davide updated this revision to Diff 62786.Jul 5 2016, 1:49 PM
davide retitled this revision from to [ELF] Introduce a flag to parse response file according to windows rules.
davide updated this object.
davide added reviewers: rafael, filcab.
davide added a subscriber: llvm-commits.
silvas added a subscriber: silvas.Jul 5 2016, 5:57 PM

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 edited edge metadata.Jul 6 2016, 5:32 AM
rafael added a subscriber: 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 added a subscriber: ruiu.Jul 6 2016, 10:14 AM

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

ELF/DriverUtils.cpp
81

Remove ().

davide updated this revision to Diff 62926.Jul 6 2016, 11:46 AM
davide added a reviewer: ruiu.
davide removed a subscriber: ruiu.

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

rafael added inline comments.Jul 6 2016, 11:48 AM
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 updated this revision to Diff 62928.Jul 6 2016, 11:50 AM

Follow clang and remove error checking.

[davide@localhost bin]$ ./clang --rsp-quoting=blah
clang-3.9: error: no input files
davide updated this revision to Diff 62940.Jul 6 2016, 12:46 PM

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.

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

ELF/DriverUtils.cpp
81

StringRef has startswith.

davide updated this revision to Diff 62944.Jul 6 2016, 12:54 PM

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 added inline comments.Jul 6 2016, 1:02 PM
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 updated this revision to Diff 62957.Jul 6 2016, 1:35 PM

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 added inline comments.Jul 6 2016, 1:47 PM
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 added inline comments.Jul 6 2016, 2:03 PM
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 added a comment.Jul 6 2016, 2:08 PM

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 updated this revision to Diff 62970.Jul 6 2016, 2:23 PM

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

ruiu accepted this revision.Jul 6 2016, 2:27 PM
ruiu edited edge metadata.

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.

This revision is now accepted and ready to land.Jul 6 2016, 2:27 PM
davide added a comment.Jul 6 2016, 2:37 PM
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 added a subscriber: ruiu.Jul 6 2016, 3:04 PM

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 closed this revision.Jul 6 2016, 4:06 PM

committed upstream