lld-link: Use /pdbsourcepath: for more places when present.

[thakis]

Summary

/pdbsourcepath: was added in https://reviews.llvm.org/D48882 to make it
possible to have relative paths in the debug info that clang-cl writes.
lld-link then makes the paths absolute at link time, which debuggers require.
This way, clang-cl's output is independent of the absolute path of the build
directory, which is useful for cacheability in distcc-like systems.

This patch extends /pdbsourcepath: (if passed) to also be used for:

1. The "cwd" stored in the env block in the pdb is /pdbsourcepath: if present
2. The "exe" stored in the env block in the pdb is made absolute relative to /pdbsourcepath: instead of the cwd
3. The "pdb" stored in the env block in the pdb is made absolute relative to /pdbsourcepath: instead of the cwd
4. For making absolute paths to .obj files referenced from the pdb

/pdbsourcepath: is now useful in three scenarios (the first one already working
before this change):

1. When building with full debug info, passing the real build dir to /pdbsourcepath: allows having clang-cl's output to be independent of the build directory path. This patch effectively doesn't change behavior for this use case (assuming the cwd is the build dir).

2. When building without compile-time debug info but linking with /debug, a fake fixed /pdbsourcepath: can be passed to get symbolized stacks while making the pdb and exe independent of the current build dir. For this two work, lld-link needs to be invoked with relative paths for the lld-link invocation itself (for "exe"), for the pdb output name, the exe output name (for "pdb"), and the obj input files, and no absolute path must appear on the link command (for "cmd" in the pdb's env block). Since no full debug info is present, it doesn't matter that the absolute path doesn't exist on disk -- we only get symbols in stacks.

3. When building production builds with full debug info that don't have local changes, and that get source indexed and their pdbs get uploaded to a symbol server. /pdbsourcepath: again makes the build output independent of the current directory, and the fixed path passed to /pdbsourcepath: can be given the source indexing transform so that it gets mapped to a repository path. This has the same requirements as 2.

This patch also makes it possible to create PDB files containing Windows-style
absolute paths when cross-compiling on a POSIX system.

[ruiu]

COFF/PDB.cpp

87 ↗

(On Diff #168789)

pdbMakeAbsolute

Add static?

88 ↗

(On Diff #168789)

You are checking if this variable is empty both in this function and before calling this function. Is that necessary?

92 ↗

(On Diff #168789)

I'd flip the condition and return early.

[thakis]

address comments

[thakis]

Thanks for the fast review! All done.

COFF/PDB.cpp

87 ↗

(On Diff #168789)

Renamed. It was in a unnamed namespace already -- moved it out and down to all the static helper functions and made it static.

88 ↗

(On Diff #168789)

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.

[thakis]

COFF/PDB.cpp

226 ↗

(On Diff #168797)

Hm, thinking about this a bit more it's not clear what passing a style to make_absolute() should do I suppose :-) .pdb files should contain Windows paths. PDBs created on a non-Windows host without passing /pdbsourcepath: don't, but it's not clear what else we could do -- cwd on a non-Windows host is a non-Windows base path. Maybe I should update the comment that this is just weird in cross builds and that people should pass /pdbsourcepath: there.

[thakis]

update comments

[ruiu]

Personally, I found that the way how Go handles pathnames is better than other languages and libraries. In Go, pathname separator is always "/" even on Windows. That means you have to convert from/to native Windows paths by replacing "\" with "/" when you do IO, but having one internal representation makes coding much easier. For example, concatenating a directory name and a filename is as easy as Dir + "/" + Filename; you don't need anything like sys::path::append. Unfortunately, that's not how we handle pathnames in LLVM, though. I'm just saying.

COFF/PDB.cpp

221 ↗

(On Diff #168798)

Can you write a justification as a function comment as to why we want to embed a full path to a PDB?

[thakis]

add requested comment

[zturner]

COFF/PDB.cpp

226 ↗

(On Diff #168797)

Sounds reasonable. Just yesterday for example I uploaded a patch that relies on generating PDBs that contain UNIX paths. So I guess we should remove all Windows-isms from this and other places.

[ruiu]

LGTM

[thakis]

Thanks!

lld/trunk/COFF/PDB.cpp

public:
   DebugSHandler(PDBLinker &Linker, ObjFile &File, const CVIndexMap &IndexMap)
       : Linker(Linker), File(File), IndexMap(IndexMap) {}
 
   void handleDebugS(lld::coff::SectionChunk &DebugS);
   void finish();
 };
 }
 
+// Visual Studio's debugger requires absolute paths in various places in the
+// PDB to work without additional configuration:
+// https://docs.microsoft.com/en-us/visualstudio/debugger/debug-source-files-common-properties-solution-property-pages-dialog-box
+static void pdbMakeAbsolute(SmallVectorImpl<char> &FileName) {
+  if (sys::path::is_absolute(FileName, sys::path::Style::windows))
+    return;
+  if (Config->PDBSourcePath.empty()) {
+    // Debuggers generally want that PDB files contain absolute, Windows-style
+    // paths. On POSIX hosts, this here will produce an absolute POSIX-style
+    // path, which is weird -- but it's not clear what else to do.
+    // People doing cross builds should probably just always pass
+    // /pbdsourcepath: and make sure paths to input obj files and to lld-link
+    // itself are relative.
+    sys::fs::make_absolute(FileName);
+    return;
+  }
+  // Using /pdbsourcepath: with absolute POSIX paths will prepend
+  // PDBSourcePath to the absolute POSIX path. Since absolute POSIX paths
+  // don't make sense in PDB files anyways, this is gargabe-in-garbage-out.
+  SmallString<128> AbsoluteFileName = Config->PDBSourcePath;
+  sys::path::append(AbsoluteFileName, sys::path::Style::windows, FileName);
+  sys::path::native(AbsoluteFileName, sys::path::Style::windows);
+  sys::path::remove_dots(AbsoluteFileName, /*remove_dot_dots=*/true,
+                         sys::path::Style::windows);
+  FileName = std::move(AbsoluteFileName);
+}
+
 static SectionChunk *findByName(ArrayRef<SectionChunk *> Sections,
                                 StringRef Name) {
   for (SectionChunk *C : Sections)
     if (C->getSectionName() == Name)
       return C;
   return nullptr;
 }
 
void DebugSHandler::finish() {
   // Make a new file checksum table that refers to offsets in the PDB-wide
   // string table. Generally the string table subsection appears after the
   // checksum table, so we have to do this after looping over all the
   // subsections.
   auto NewChecksums = make_unique<DebugChecksumsSubsection>(Linker.PDBStrTab);
   for (FileChecksumEntry &FC : Checksums) {
     SmallString<128> FileName =
         ExitOnErr(CVStrTab.getString(FC.FileNameOffset));
-    if (!sys::path::is_absolute(FileName) && !Config->PDBSourcePath.empty()) {
-      SmallString<128> AbsoluteFileName = Config->PDBSourcePath;
-      sys::path::append(AbsoluteFileName, FileName);
-      sys::path::native(AbsoluteFileName);
-      sys::path::remove_dots(AbsoluteFileName, /*remove_dot_dots=*/true);
-      FileName = std::move(AbsoluteFileName);
-    }
+    pdbMakeAbsolute(FileName);
     ExitOnErr(Linker.Builder.getDbiBuilder().addModuleSourceFile(
         *File.ModuleDBI, FileName));
     NewChecksums->addChecksum(FileName, FC.Kind, FC.Checksum);
   }
   File.ModuleDBI->addDebugSubsection(std::move(NewChecksums));
 }
 
 void PDBLinker::addObjFile(ObjFile *File) {
   // Add a module descriptor for every object file. We need to put an absolute
   // path to the object into the PDB. If this is a plain object, we make its
   // path absolute. If it's an object in an archive, we make the archive path
   // absolute.
   bool InArchive = !File->ParentName.empty();
   SmallString<128> Path = InArchive ? File->ParentName : File->getName();
-  sys::fs::make_absolute(Path);
+  pdbMakeAbsolute(Path);
   sys::path::native(Path, sys::path::Style::windows);
   StringRef Name = InArchive ? File->getName() : StringRef(Path);
 
   pdb::DbiStreamBuilder &DbiBuilder = Builder.getDbiBuilder();
   File->ModuleDBI = &ExitOnErr(DbiBuilder.addModuleInfo(Name));
   File->ModuleDBI->setObjFileName(Path);
 
   auto Chunks = File->getChunks();
static void addCommonLinkerModuleSymbols(StringRef Path,
   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, " ");
   EBS.Fields.push_back("cwd");
   SmallString<64> cwd;
+  if (Config->PDBSourcePath.empty()) 
     sys::fs::current_path(cwd);
+  else
+    cwd = Config->PDBSourcePath;
   EBS.Fields.push_back(cwd);
   EBS.Fields.push_back("exe");
   SmallString<64> exe = Config->Argv[0];
-  llvm::sys::fs::make_absolute(exe);
+  pdbMakeAbsolute(exe);
   EBS.Fields.push_back(exe);
   EBS.Fields.push_back("pdb");
   EBS.Fields.push_back(Path);
   EBS.Fields.push_back("cmd");
   EBS.Fields.push_back(ArgStr);
   Mod.addSymbol(codeview::SymbolSerializer::writeOneSymbol(
       ONS, Allocator, CodeViewContainer::Pdb));
   Mod.addSymbol(codeview::SymbolSerializer::writeOneSymbol(
void PDBLinker::initialize(llvm::codeview::DebugInfo *BuildId) {
   DbiBuilder.setBuildNumber(14, 11);
 }
 
 void PDBLinker::addSections(ArrayRef<OutputSection *> OutputSections,
                             ArrayRef<uint8_t> SectionTable) {
   // It's not entirely clear what this is, but the * Linker * module uses it.
   pdb::DbiStreamBuilder &DbiBuilder = Builder.getDbiBuilder();
   NativePath = Config->PDBPath;
-  sys::fs::make_absolute(NativePath);
+  pdbMakeAbsolute(NativePath);
   sys::path::native(NativePath, sys::path::Style::windows);
   uint32_t PdbFilePathNI = DbiBuilder.addECName(NativePath);
   auto &LinkerModule = ExitOnErr(DbiBuilder.addModuleInfo("* Linker *"));
   LinkerModule.setPdbFilePathNI(PdbFilePathNI);
   addCommonLinkerModuleSymbols(NativePath, LinkerModule, Alloc);
 
   // Add section contributions. They must be ordered by ascending RVA.
   for (OutputSection *OS : OutputSections) {

lld/trunk/test/COFF/pdb-relative-source-lines.test

int main(void) {
   return 42;
 }
 ==> pdb_lines_2.c <==
 void bar(void) {
 }
 
 $ clang-cl -Xclang -fdebug-compilation-dir -Xclang . -c -Z7 pdb_lines*.c
 
-RUN: yaml2obj %S/Inputs/pdb_lines_1_relative.yaml -o %t.pdb_lines_1_relative.obj
-RUN: yaml2obj %S/Inputs/pdb_lines_2_relative.yaml -o %t.pdb_lines_2_relative.obj
-RUN: rm -f %t.exe %t.pdb
-RUN: lld-link -debug -pdbsourcepath:c:\\src -entry:main -nodefaultlib -out:%t.exe -pdb:%t.pdb %t.pdb_lines_1_relative.obj %t.pdb_lines_2_relative.obj
-RUN: llvm-pdbutil pdb2yaml -modules -module-files -subsections=lines,fc %t.pdb | FileCheck %s
+/pdbsourcepath: only sets the directory that relative paths are considered
+relative to, so this test needs to pass relative paths to lld-link for:
+1. The input obj files
+2. The /pdb: switch
+3. The lld-link invocation itself
+To achieve this, put all inputs of the lld-link invocation (including lld-link
+itself) in a temp directory that's cwd and then make sure to only use relative
+arguments when calling ./lld-link below.
+RUN: rm -rf %t
+RUN: mkdir %t
+RUN: cp lld-link %t/lld-link
+RUN: cd %t
 
-CHECK-LABEL:  - Module:          {{.*}}pdb_lines_1_relative.obj
-CHECK-NEXT:     ObjFile:         {{.*}}pdb_lines_1_relative.obj
+RUN: yaml2obj %S/Inputs/pdb_lines_1_relative.yaml -o %t/pdb_lines_1_relative.obj
+RUN: yaml2obj %S/Inputs/pdb_lines_2_relative.yaml -o %t/pdb_lines_2_relative.obj
+RUN: ./lld-link -debug -pdbsourcepath:c:\\src -entry:main -nodefaultlib -out:out.exe -pdb:out.pdb pdb_lines_1_relative.obj pdb_lines_2_relative.obj
+RUN: llvm-pdbutil pdb2yaml -modules -module-files -module-syms -subsections=lines,fc %t/out.pdb | FileCheck %s
+
+CHECK-LABEL:  - Module:          'c:\src\pdb_lines_1_relative.obj'
+CHECK-NEXT:     ObjFile:         'c:\src\pdb_lines_1_relative.obj'
 CHECK:          SourceFiles:
 CHECK-NEXT:       - 'c:{{[\\/]}}src{{[\\/]}}pdb_lines_1.c'
 CHECK-NEXT:       - 'c:{{[\\/]}}src{{[\\/]}}foo.h'
 CHECK:          Subsections:
 CHECK:                - FileName:        'c:{{[\\/]}}src{{[\\/]}}pdb_lines_1.c'
 CHECK:                - FileName:        'c:{{[\\/]}}src{{[\\/]}}foo.h'
 CHECK:            - !FileChecksums
 CHECK:                - FileName:        'c:{{[\\/]}}src{{[\\/]}}pdb_lines_1.c'
 CHECK:                - FileName:        'c:{{[\\/]}}src{{[\\/]}}foo.h'
 
-CHECK-LABEL:  - Module:          {{.*}}pdb_lines_2_relative.obj
-CHECK-NEXT:     ObjFile:         {{.*}}pdb_lines_2_relative.obj
+CHECK-LABEL:  - Module:          'c:\src\pdb_lines_2_relative.obj'
+CHECK-NEXT:     ObjFile:         'c:\src\pdb_lines_2_relative.obj'
 CHECK:          SourceFiles:
 CHECK-NEXT:       - 'c:{{[\\/]}}src{{[\\/]}}pdb_lines_2.c'
 CHECK:          Subsections:
 CHECK:                - FileName:        'c:{{[\\/]}}src{{[\\/]}}pdb_lines_2.c'
 CHECK:            - !FileChecksums
 CHECK:                - FileName:        'c:{{[\\/]}}src{{[\\/]}}pdb_lines_2.c'
+
+CHECK-LABEL:  - Kind:            S_ENVBLOCK
+CHECK-NEXT:     EnvBlockSym:     
+CHECK-NEXT:       Entries:     
+CHECK-NEXT:         - cwd
+CHECK-NEXT:         - 'c:\src'
+CHECK-NEXT:         - exe
+CHECK-NEXT:         - 'c:\src\lld-link'
+CHECK-NEXT:         - pdb 
+CHECK-NEXT:         - 'c:\src\out.pdb'
+CHECK-NEXT:         - cmd
+CHECK-NEXT:         - '-debug -pdbsourcepath:c:\src -entry:main -nodefaultlib -out:out.exe -pdb:out.pdb pdb_lines_1_relative.obj pdb_lines_2_relative.obj'