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

[aganea]

Summary

Linker command-line arguments containing spaces were being stored without quoting the arguments. This resulted in ambiguous argument strings and prevented reliable parsing. MSVC simply wraps arguments containing spaces in quotes, so we do the same.

[ruiu]

COFF/PDB.cpp

1068

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.)

[zturner]

COFF/PDB.cpp

1075–1076

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?)

[lantictac]

COFF/PDB.cpp

1068

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).

1075–1076

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.

[lantictac]

Extracted implementation to a quote() function.

[amccarth]

COFF/PDB.cpp

1075–1076

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

[ruiu]

COFF/PDB.cpp

1068

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.

[lantictac]

COFF/PDB.cpp

1068

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.

1075–1076

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?

[lantictac]

Hi All, What's the consensus on the additional checks? I can try and add support for the theoretical corner cases but without being able to construct test cases I'm not sure how useful it'd be.

[ruiu]

COFF/PDB.cpp

1068

It is possible to pass a single double-quote to the command and as a result the double-quote character shows up in Config->Args. Here is an example:

> lld-link "Hello ""World"""
C:\src\b\bin\lld-link.exe: error: could not open Hello "World": invalid argument

[lantictac]

COFF/PDB.cpp

1068

Thanks for clarifying. I'll see if I can get it to to repro on a Windows build with a Windows specific test case.

[pcc]

It looks like MSVC does not bother to quote quotation marks correctly.

/mnt/c/src/tmp$ cat quote.yaml
--- !COFF
header:
  Machine:         IMAGE_FILE_MACHINE_I386
  Characteristics: [  ]  
sections:
  - Name:            '.text'
    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_EXECUTE ]
    Alignment:       1   
    SectionData:     00  
symbols:
  - Name:            '"'
    Value:           0
    SectionNumber:   1
    SimpleType:      IMAGE_SYM_TYPE_NULL
    ComplexType:     IMAGE_SYM_DTYPE_NULL
    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
...
/mnt/c/src/tmp$ ../l/ra/bin/yaml2obj.exe quote.yaml  > quote.obj

C:\src\tmp>link /debug "/entry:""" /subsystem:console quote.obj
Microsoft (R) Incremental Linker Version 14.12.25835.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/mnt/c/src/tmp$ hd quote.pdb | grep -2 entry
000060d0  65 78 65 00 70 64 62 00  43 3a 5c 73 72 63 5c 74  |exe.pdb.C:\src\t|
000060e0  6d 70 5c 71 75 6f 74 65  2e 70 64 62 00 63 6d 64  |mp\quote.pdb.cmd|
000060f0  00 20 2f 64 65 62 75 67  20 2f 65 6e 74 72 79 3a  |. /debug /entry:|
00006100  22 20 2f 73 75 62 73 79  73 74 65 6d 3a 63 6f 6e  |" /subsystem:con|
00006110  73 6f 6c 65 00 00 00 00  1a 00 36 11 01 00 0c 00  |sole......6.....|

[lantictac]

A few issues. On the DOS command-line I can shoehorn in some escaped double-quotes using /manifestuac:"level='asInvoker' uiAccess=""false""" and still generate a PDB. However:

1. I can't figure out a way to get the test runner to pass the escaped double-quotes during testing. It either mangles the command-line or the double-quotes are ignored. Only directly running under cmd.exe seems to work.
2. I'm still not convinced anyone would ever encounter an escaped double-quote in a link.exe command-line IRL. Although, feel free to come up with a realistic example I haven't thought of.
3. I'm not sure if escaping double-quotes as "" would be the best method of escaping the quote since this appears to be specific to the the Command Prompt parsing rules (and MSVC offers no standard approach).
4. As pcc points out MSVC link.exe doesn't escape these inner double-quotes anyhow.
5. The link command-line stored in the PDB unlikely to matter to the vast majority of users.

In short, I can't really afford much more time on this unless anyone can resolve/answer/workaround a few of the issues above. Any thoughts?

[lantictac]

Also: Just noticed that response files aren't being expanded inline in the stored command-line (as per MSVC link.exe). I'll open a bug for it.

[aganea]

@ruiu what is the consensus on this? What would be a practical use-case for escaping double-quotes?

File names can already be ruled out on Windows, as double quotes are reserved.

That could happen if one is linking a COFF target on Linux system? However in that case the 'cmd' field in the PDB will not be reusable afterwards on a Windows system, which is the whole purpose of this change. If that happens, we should rather warn if reserved Windows characters are used in the 'cmd' field?

As a counter-example, /manifestuac uses single quotes to prevent such issues: /manifestuac:"level='asInvoker' uiAccess='false'"

[stefan_reinalter]

In D46427#1102066, @lantictac wrote:

Also: Just noticed that response files aren't being expanded inline in the stored command-line (as per MSVC link.exe). I'll open a bug for it.

Second that.
Would be nice to expand response files. Did you open a bug for this/are you working on this?

[lantictac]

In D46427#1165040, @stefan_reinalter wrote:

In D46427#1102066, @lantictac wrote:

Also: Just noticed that response files aren't being expanded inline in the stored command-line (as per MSVC link.exe). I'll open a bug for it.

Second that.
Would be nice to expand response files. Did you open a bug for this/are you working on this?

The bug is here: 38085. I started looking into it and it looks like it ought to be fairly simple to implement since LLD already has to generate the expanded arguments. The only challenge is passing them back to the PDB generation code. Probably won't get to it this week however.

[aganea]

@lantictac The following works in the test:

RUN: lld-link %S/a.obj /entry:"1 "'"'hello'"'" 2"

Outputs as expected:

lld-link: error: <root>: undefined symbol: 1 "hello" 2

Could you please update the diff to handle this case?

[aganea]

Commited as rL348001: [PDB] Quote linker arguments containing spaces (mimic MSVC)

COFF/PDB.cpp

static void addCommonLinkerModuleSymbols(StringRef Path,
   CS.VersionFrontendBuild = 0;
   CS.VersionFrontendMajor = 0;
   CS.VersionFrontendMinor = 0;
   CS.VersionFrontendQFE = 0;
   CS.Version = "LLVM Linker";
   CS.setLanguage(SourceLanguage::Link);
 
   ArrayRef<StringRef> Args = makeArrayRef(Config->Argv).drop_front();
-  std::string ArgStr = llvm::join(Args, " ");
+
+  // MSVC surrounds arguments containing spaces with quotes, so we do the same.
+  SmallString<256> ArgStr;
+  for (StringRef Arg : Args) {
+    if (!ArgStr.empty())
+      ArgStr.push_back(' ');
+
+    bool HasWS = Arg.find(' ') != StringRef::npos;
+    if (HasWS)
+      ArgStr.push_back('"');
+    ArgStr.append(Arg);
+    if (HasWS)
+      ArgStr.push_back('"');
+  }
+
   EBS.Fields.push_back("cwd");
   SmallString<64> cwd;
   sys::fs::current_path(cwd);
   EBS.Fields.push_back(cwd);
   EBS.Fields.push_back("exe");
   SmallString<64> exe = Config->Argv[0];
   llvm::sys::fs::make_absolute(exe);
   EBS.Fields.push_back(exe);

test/COFF/pdb-linker-module.test

-RUN: lld-link /debug /pdb:%t.pdb /nodefaultlib /entry:main %S/Inputs/pdb-diff.obj
+RUN: lld-link /debug /pdb:%t.pdb /nodefaultlib /entry:main /manifestuac:"level='asInvoker' uiAccess='false'" %S/Inputs/pdb-diff.obj
 RUN: llvm-pdbutil dump -modules %t.pdb | FileCheck --check-prefix=MODS %s
 RUN: llvm-pdbutil dump -symbols %t.pdb | FileCheck --check-prefix=SYMS %s
 
 MODS:      Mod 0001 | `* Linker *`
 MODS-NEXT:             Obj: ``:
 MODS-NEXT:             debug stream: 12, # files: 0, has ec info: false
 MODS-NEXT:             pdb file ni: 1 `{{.*}}pdb-linker-module.test.tmp.pdb`, src file ni: 0 ``
 
 SYMS:      Mod 0001 | `* Linker *`
 SYMS-NEXT:  4 | S_OBJNAME [size = 20] sig=0, `* Linker *`
 SYMS-NEXT: 24 | S_COMPILE3 [size = 40]
 SYMS-NEXT:      machine = intel 80386, Ver = LLVM Linker, language = link
 SYMS-NEXT:      frontend = 0.0.0.0, backend = 14.10.25019.0
 SYMS-NEXT:      flags = none
 SYMS-NEXT: 64 | S_ENVBLOCK
 SYMS-NEXT: - cwd
 SYMS-NEXT: -
 SYMS-NEXT: - exe
 SYMS-NEXT: - {{.*}}lld-link
 SYMS-NEXT: - pdb
 SYMS-NEXT: - {{.*}}pdb-linker-module{{.*}}pdb
 SYMS-NEXT: - cmd
-SYMS-NEXT: - /debug /pdb:{{.*}}pdb-linker-module{{.*}}pdb /nodefaultlib /entry:main {{.*}}pdb-diff.obj
+SYMS-NEXT: - /debug /pdb:{{.*}}pdb-linker-module{{.*}}pdb /nodefaultlib /entry:main "/manifestuac:level='asInvoker' uiAccess='false'" {{.*}}pdb-diff.obj