[PATCH] D46427: [PDB] Quote linker arguments containing spaces (mimic MSVC)

[Will Wilson via Phabricator via llvm-commits]

lantictac added inline comments.


================
Comment at: COFF/PDB.cpp:1068
+
+  // MSVC surrounds arguments containing spaces with quotes, so we do the same.
+  SmallString<256> ArgStr;
----------------
ruiu wrote:
> lantictac wrote:
> > ruiu wrote:
> > > Can you factor this code as `std::string quote(ArayRef<StringRef>)`?
> > > 
> > > I believe you also need to quote a double-quote as double double-quotes as well (but please check what the MS linker behavior is first.)
> > I'm not quite sure what you meant by "quote a double-quote as double double-quotes", or at least I couldn't find or reproduce anything to support this way of escaping a double-quote. Any further clues?
> > 
> > Other than that, I've factored out the code into a quote() function as requested (patch pending).
> I don't know if it is applicable to this field, but in Windows command line, the way to escape a single double-quote in a double-quoted string is to replace it with two double-quote characters.
> 
> For example, to get `hello "world"`, one needs to quote it as `"hello ""world"""`.
> 
> At least the way you escape a string seems ambiguous when a string contains a double-quote character.
It doesn't seem applicable in this case. At least I'm finding it impossible to get a single quote to show up in the args however I specify the command-line.

So as far as I can ascertain it shouldn't be a concern for this code.


================
Comment at: COFF/PDB.cpp:1075-1076
+    bool HasWS = Arg.find(' ') != StringRef::npos;
+    if (HasWS)
+      ArgStr.push_back('"');
+    ArgStr.append(Arg);
----------------
amccarth wrote:
> lantictac wrote:
> > zturner wrote:
> > > In addition to what rui suggested, can you only do this if the string is not already quoted?  Furthermore, you may need to escape embedded quotes (is that something we need to worry about?)
> > The arguments always appear to have all double-quotes removed. This applies for both the MS linker and lld-link (by the time this code sees the arguments anyhow).
> > 
> > I've also tried escaping embedded double-quotes but they seem to be eaten by the argument parser and never appear in the tokenized args. Not to mention that there's really no place embedded quotes could be in any valid linker command-line (to the best of my knowledge).
> > 
> > So I can't see either case being worth worrying about.
> Since quotation marks are not valid in file names, I believe lantictac is correct that this is not worth worrying about escaping quotation marks within an argument (for linker commands).
> 
> The rules for escaping quotation marks and backslashes in the Windows command line are arcane.  Worse, they differ slightly depending on whether the program relies on the CRT to parse the command into a main-style argument vector of whether it calls CommandLineToArgvW to parse it explicitly.
> 
> This Raymond Chen post touches on the some of the complexity, but the juiciest bits are in the comments:  
> https://blogs.msdn.microsoft.com/oldnewthing/20100917-00/?p=12833
> 
To add to that, without a decent and likely test case it's going be hard to implement the functionality required to handle embedded double-quotations. How about we implement it if and when it's ever discovered in the wild?



https://reviews.llvm.org/D46427