[PATCH] D53021: lld-link: Use /pdbsourcepath: for more places when present.

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

Thanks for the fast review! All done.



================
Comment at: COFF/PDB.cpp:87
 
+void pdb_make_absolute(SmallVectorImpl<char> &FileName) {
+  if (Config->PDBSourcePath.empty()) {
----------------
ruiu wrote:
> pdbMakeAbsolute
> 
> Add `static`?
Renamed. It was in a unnamed namespace already -- moved it out and down to all the static helper functions and made it static.


================
Comment at: COFF/PDB.cpp:88
+void pdb_make_absolute(SmallVectorImpl<char> &FileName) {
+  if (Config->PDBSourcePath.empty()) {
+    sys::fs::make_absolute(FileName);
----------------
ruiu wrote:
> You are checking if this variable is empty both in this function and before calling this function. Is that necessary?
I check in one calling place, to maintain the old behavior. For symbols in debug info, we would only prepend /pdbsourcepath: if it was present, but not call make_absolute() if it wasn't and the path was relative; the check before calling maintains that behavior.

I'm guessing changing the behavior for simpler code should be safe though: The only way to get relative symbols in the compiler output is with -Xclang flags which kind of force you to use /pdbsourcepath: to get a functional pdb already. So, done.

Oh, looks like it made a test fail:

```
FAIL: lld :: COFF/pdb-file-static.test (183 of 1897)
******************** TEST 'lld :: COFF/pdb-file-static.test' FAILED ********************
Script:
--
: 'RUN: at line 1';   /Users/thakis/src/llvm-build-goma/bin/yaml2obj /Users/thakis/src/llvm-rw/tools/lld/test/COFF/Inputs/pdb-file-statics-a.yaml > /Users/thakis/src/llvm-build-goma/tools/lld/test/COFF/Output/pdb-file-static.test.tmp.a.obj
: 'RUN: at line 2';   /Users/thakis/src/llvm-build-goma/bin/yaml2obj /Users/thakis/src/llvm-rw/tools/lld/test/COFF/Inputs/pdb-file-statics-b.yaml > /Users/thakis/src/llvm-build-goma/tools/lld/test/COFF/Output/pdb-file-static.test.tmp.b.obj
: 'RUN: at line 3';   /Users/thakis/src/llvm-build-goma/bin/lld-link /Users/thakis/src/llvm-build-goma/tools/lld/test/COFF/Output/pdb-file-static.test.tmp.a.obj /Users/thakis/src/llvm-build-goma/tools/lld/test/COFF/Output/pdb-file-static.test.tmp.b.obj /nodefaultlib /entry:main /debug /pdb:/Users/thakis/src/llvm-build-goma/tools/lld/test/COFF/Output/pdb-file-static.test.tmp.pdb
: 'RUN: at line 4';   /Users/thakis/src/llvm-build-goma/bin/llvm-pdbutil dump -symbols /Users/thakis/src/llvm-build-goma/tools/lld/test/COFF/Output/pdb-file-static.test.tmp.pdb | /Users/thakis/src/llvm-build-goma/bin/FileCheck /Users/thakis/src/llvm-rw/tools/lld/test/COFF/pdb-file-static.test
--
Exit Code: 1

Command Output (stderr):
--
/Users/thakis/src/llvm-rw/tools/lld/test/COFF/pdb-file-static.test:49:15: error: CHECK-NEXT: expected string not found in input
# CHECK-NEXT: type = 0x0074 (int), file name = 74 (D:\src\llvmbuild\cl\Debug\x64\b.obj), flags = enreg global | enreg static
              ^
<stdin>:103:2: note: scanning from here
 type = 0x0074 (int), file name = 128 (D:\src\llvmbuild\cl\Debug\x64\b.obj), flags = enreg global | enreg static
 ^

--

********************
Testing Time: 21.75s
********************
Failing Tests (1):
    lld :: COFF/pdb-file-static.test
```

It's because of this:

```
$ /Users/thakis/src/llvm-build-goma/bin/llvm-pdbutil pdb2yaml -string-table /Users/thakis/src/llvm-build-goma/tools/lld/test/COFF/Output/pdb-file-static.test.tmp.pdb 
---
MSF:             
  SuperBlock:      
    BlockSize:       4096
    FreeBlockMap:    2
    NumBlocks:       20
    NumDirectoryBytes: 128
    Unknown1:        0
    BlockMapAddr:    3
  NumDirectoryBlocks: 1
  DirectoryBlocks: [ 19 ]
  NumStreams:      0
  FileSize:        81920
StringTable:     
  - 'D:\src\llvmbuild\cl\Debug\x64\b.obj'
  - '/Users/thakis/src/llvm-build-goma/tools/lld/test/COFF/d:\src\llvmbuild\cl\debug\x64\b.cpp'
  - 'D:\src\llvmbuild\cl\Debug\x64\a.obj'
  - '/Users/thakis/src/llvm-build-goma/tools/lld/test/COFF/d:\src\llvmbuild\cl\debug\x64\a.cpp'
```

Note the weird posix/win mixed path. Looks like `make_absolute()` doesn't have a style parameter. I can mostly work around this by explicitly passing style to is_absolute and checking that first in the function, so did that and added a FIXME.


https://reviews.llvm.org/D53021