[PDB] Remove dots and canonicalize slashes when using /PDBSOURCEPATH

[zturner]

Summary

In a previous patch, I made changes so that PDBs which were
generated on non-Windows platforms contained sensical paths
for the host.  While this is an esoteric use case, we need
it to be supported for certain cross compilation scenarios
especially with LLDB, which can debug things on non-Windows
platforms.

However, this regressed a case where you specify /PDBSOURCEPATH
and use a windows-style path.  Previously, we would still remove
dots and canonicalize slashes to backslashes, but since my
change intentionally tried to support non-backslash paths, this
was broken.

This patch fixes the situation by trying to guess which path
style the user is specifying when /PDBSOURCEPATH is passed.
It is intentionally conservative, erring on the side of a
Windows path style unless absolutely certain.  All dots are
removed and slashes canonicalized to whatever the deduced
path style is after appending the file path to the /PDBSOURCEPATH
argument.

[thakis]

Do you understand why D53149 changed behavior on a Windows host?

I thought we had 2 issues:

1. Your patch wasn't supposed to change behavior on Windows hosts, yet it did. Understand why and fix. This shouldn't need a change like this.

2. In cross builds, it'd be nice if there was a way to get the Windows behavior, e.g. by looking for ':' (what this patch does).

Oh, I see http://reviews.llvm.org/rL344377 is different from https://reviews.llvm.org/D53149 . New in the non-reviewed bit is:

// Only apply native and dot removal to the relative file path.  We want to
// leave the path the user specified untouched since we assume they specified
// it for a reason.

Is that necessary for anything? That doesn't look like the behavior we want -- we do want /pdbsourcepath:c:\foo\bar and a file with a relative path of ..\..\blah to become c:\blah. Can we undo that bit to fix (1)?

[zturner]

In D57769#1385789, @thakis wrote:

// Only apply native and dot removal to the relative file path.  We want to
// leave the path the user specified untouched since we assume they specified
// it for a reason.

Is that necessary for anything? That doesn't look like the behavior we want -- we do want /pdbsourcepath:c:\foo\bar and a file with a relative path of ..\..\blah to become c:\blah. Can we undo that bit to fix (1)?

It's possible I'm misunderstanding your comment, but I actually did remove that in this patch?

[thakis]

Ah, I see.

It feels a bit weird to use the native path functions only to then manually replace slashes. What do you think about:

sys::path::Style guessed_style = Config->PDBSourcePath.startswith("/")
                                    ? sys::path::Style::posix
                                    : sys::path::Style::windows;

after the first return and then pass that to all the sys::path function calls in this function instead?

[zturner]

In D57769#1385861, @thakis wrote:

Ah, I see.

It feels a bit weird to use the native path functions only to then manually replace slashes. What do you think about:

sys::path::Style guessed_style = Config->PDBSourcePath.startswith("/")
                                    ? sys::path::Style::posix
                                    : sys::path::Style::windows;

after the first return and then pass that to all the sys::path function calls in this function instead?

Well, the second return (which gets entered if user did not specify /PDBSOURCEPATH) handles the case where it actually *must be* on the local file system, so I think those calls should still use the native path syntax. But it would probably be fine to use the sys::path functions everywhere else. The reason I didn't do it is because they deal with std::string, and it seemed silly to have to marshal this SmallString<> through a separate type just to satisfy the API. I could also have added an overload of convert_to_slash() that just dealt with SmallString, but ultimately they just boil down to a std::replace() anyway, so I just inlined it.

[thakis]

lgtm either way, but below seems simpler to me.

(Also, isn't it GuessedStyle instead of guessed_style in llvm style?

lld/COFF/PDB.cpp

300 ↗

(On Diff #185345)

It's already in a SmallString here though. Wouldn't it just be

sys::path::append(AbsolutFileName, guessed_style, FileName);
sys::path::native(AbsoluteFileName, guessed_style);
sys::path::remove_dots(AbsoluteFileName, /*remove_dot_dots=*/true,
                                        guessed_style);

?

(and put the sys::path::native(FileName) call above into the PDBSourcePath.empyt() if)

COFF/PDB.cpp

static void pdbMakeAbsolute(SmallVectorImpl<char> &FileName) {
   // passed in as the gospel and not normalize, canonicalize it.
   if (sys::path::is_absolute(FileName, sys::path::Style::windows) ||
       sys::path::is_absolute(FileName, sys::path::Style::posix))
     return;
 
   // It's not absolute in any path syntax.  Relative paths necessarily refer to
   // the local file system, so we can make it native without ending up with a
   // nonsensical path.
-  sys::path::native(FileName);
   if (Config->PDBSourcePath.empty()) {
+    sys::path::native(FileName);
     sys::fs::make_absolute(FileName);
     return;
   }
-  // Only apply native and dot removal to the relative file path.  We want to
-  // leave the path the user specified untouched since we assume they specified
-  // it for a reason.
-  sys::path::remove_dots(FileName, /*remove_dot_dots=*/true);
 
+  // Try to guess whether /PDBSOURCEPATH is a unix path or a windows path.
+  // Since PDB's are more of a Windows thing, we make this conservative and only
+  // decide that it's a unix path if we're fairly certain.  Specifically, if
+  // it starts with a forward slash.
   SmallString<128> AbsoluteFileName = Config->PDBSourcePath;
-  sys::path::append(AbsoluteFileName, FileName);
+  sys::path::Style GuessedStyle = AbsoluteFileName.startswith("/")
+                                      ? sys::path::Style::posix
+                                      : sys::path::Style::windows;
+  sys::path::append(AbsoluteFileName, GuessedStyle, FileName);
+  sys::path::native(AbsoluteFileName, GuessedStyle);
+  sys::path::remove_dots(AbsoluteFileName, true, GuessedStyle);
+
   FileName = std::move(AbsoluteFileName);
 }
 
 static SectionChunk *findByName(ArrayRef<SectionChunk *> Sections,
                                 StringRef Name) {
   for (SectionChunk *C : Sections)
     if (C->getSectionName() == Name)
       return C;

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

 
 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
 RUN: ./lld-link -debug "-pdbsourcepath:/usr/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 --check-prefix=POSIX %s
 
-CHECK-LABEL:  - Module:          'c:\src{{[\\/]}}pdb_lines_1_relative.obj'
-CHECK-NEXT:     ObjFile:         'c:\src{{[\\/]}}pdb_lines_1_relative.obj'
+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-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:                - 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:                - FileName:        'c:\src\pdb_lines_1.c'
+CHECK:                - FileName:        'c:\src\foo.h'
 
-CHECK-LABEL:  - Module:          'c:\src{{[\\/]}}pdb_lines_2_relative.obj'
-CHECK-NEXT:     ObjFile:         'c:\src{{[\\/]}}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-NEXT:       - 'c:\src\pdb_lines_2.c'
 CHECK:          Subsections:
-CHECK:                - FileName:        'c:\src{{[\\/]}}pdb_lines_2.c'
+CHECK:                - FileName:        'c:\src\pdb_lines_2.c'
 CHECK:            - !FileChecksums
-CHECK:                - FileName:        'c:\src{{[\\/]}}pdb_lines_2.c'
+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:         - 'c:\src\lld-link'
 CHECK-NEXT:         - pdb 
-CHECK-NEXT:         - 'c:\src{{[\\/]}}out.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'
 
 
-POSIX-LABEL:  - Module:          '/usr/src{{[\\/]}}pdb_lines_1_relative.obj'
-POSIX-NEXT:     ObjFile:         '/usr/src{{[\\/]}}pdb_lines_1_relative.obj'
+POSIX-LABEL:  - Module:          '/usr/src/pdb_lines_1_relative.obj'
+POSIX-NEXT:     ObjFile:         '/usr/src/pdb_lines_1_relative.obj'
 POSIX:          SourceFiles:
-POSIX-NEXT:       - '/usr/src{{[\\/]}}pdb_lines_1.c'
-POSIX-NEXT:       - '/usr/src{{[\\/]}}foo.h'
+POSIX-NEXT:       - '/usr/src/pdb_lines_1.c'
+POSIX-NEXT:       - '/usr/src/foo.h'
 POSIX:          Subsections:
-POSIX:                - FileName:        '/usr/src{{[\\/]}}pdb_lines_1.c'
-POSIX:                - FileName:        '/usr/src{{[\\/]}}foo.h'
+POSIX:                - FileName:        '/usr/src/pdb_lines_1.c'
+POSIX:                - FileName:        '/usr/src/foo.h'
 POSIX:            - !FileChecksums
-POSIX:                - FileName:        '/usr/src{{[\\/]}}pdb_lines_1.c'
-POSIX:                - FileName:        '/usr/src{{[\\/]}}foo.h'
+POSIX:                - FileName:        '/usr/src/pdb_lines_1.c'
+POSIX:                - FileName:        '/usr/src/foo.h'
 
-POSIX-LABEL:  - Module:          '/usr/src{{[\\/]}}pdb_lines_2_relative.obj'
-POSIX-NEXT:     ObjFile:         '/usr/src{{[\\/]}}pdb_lines_2_relative.obj'
+POSIX-LABEL:  - Module:          '/usr/src/pdb_lines_2_relative.obj'
+POSIX-NEXT:     ObjFile:         '/usr/src/pdb_lines_2_relative.obj'
 POSIX:          SourceFiles:
-POSIX-NEXT:       - '/usr/src{{[\\/]}}pdb_lines_2.c'
+POSIX-NEXT:       - '/usr/src/pdb_lines_2.c'
 POSIX:          Subsections:
-POSIX:                - FileName:        '/usr/src{{[\\/]}}pdb_lines_2.c'
+POSIX:                - FileName:        '/usr/src/pdb_lines_2.c'
 POSIX:            - !FileChecksums
-POSIX:                - FileName:        '/usr/src{{[\\/]}}pdb_lines_2.c'
+POSIX:                - FileName:        '/usr/src/pdb_lines_2.c'
 
 POSIX-LABEL:  - Kind:            S_ENVBLOCK
 POSIX-NEXT:     EnvBlockSym:     
 POSIX-NEXT:       Entries:     
 POSIX-NEXT:         - cwd
 POSIX-NEXT:         - '/usr/src'
 POSIX-NEXT:         - exe
-POSIX-NEXT:         - '/usr/src{{[\\/]}}lld-link'
+POSIX-NEXT:         - '/usr/src/lld-link'
 POSIX-NEXT:         - pdb 
-POSIX-NEXT:         - '/usr/src{{[\\/]}}out.pdb'
+POSIX-NEXT:         - '/usr/src/out.pdb'
 POSIX-NEXT:         - cmd
 POSIX-NEXT:         - '-debug -pdbsourcepath:/usr/src -entry:main -nodefaultlib -out:out.exe -pdb:out.pdb pdb_lines_1_relative.obj pdb_lines_2_relative.obj'