Use native path syntax when writing PDB module name.

[zturner]

Summary

Previously lld-link would write module names containing forward slashes, even on Windows. This differs from what link does, which makes sure all paths are Windows-ized. This may or may not be necessary for Windows tools to work with clang-cl generated PDBs, but since we don't know, we need to do the same thing. This issue was also causing diff to think these were two different modules and not comparing their fields, so we now get better output from diff.

[rnk]

lld/COFF/PDB.cpp

248–249 ↗

(On Diff #105545)

We should explicitly pass sys::fs::style::windows to get the same behavior on Linux, right? Otherwise the test won't pass there.

lld/test/COFF/pdb-diff.test

175 ↗

(On Diff #105545)

This absolute path won't be the same on other people's systems.

[zturner]

lld/COFF/PDB.cpp

248–249 ↗

(On Diff #105545)

I actually think we should use native path syntax. We're already in uncharted waters dealing with cross-compilation because we'll be the first ones that have ever tried to do it, but since semantically the PDB file is supposed to contain "absolute path to the module on the file system where it was compiled", it seems like we need it to be a real path. A windows tool isn't going to understand a path to a different machine anyway (even if it uses windows path syntax), so we might as well write it in a way that at least *some* system can make sense of the path. We can fix the test by using a regex like `{{.*pdb-diff.obj}}

lld/test/COFF/pdb-diff.test

175 ↗

(On Diff #105545)

Fixed offline.

lld/trunk/COFF/PDB.cpp

static void addObjectsToPDB(BumpPtrAllocator &Alloc, SymbolTable *Symtab,
   for (ObjectFile *File : Symtab->ObjectFiles) {
     // 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);
+    sys::path::native(Path, llvm::sys::path::Style::windows);
     StringRef Name = InArchive ? File->getName() : StringRef(Path);
+
     File->ModuleDBI = &ExitOnErr(Builder.getDbiBuilder().addModuleInfo(Name));
     File->ModuleDBI->setObjFileName(Path);
 
     // Before we can process symbol substreams from .debug$S, we need to process
     // type information, file checksums, and the string table.  Add type info to
     // the PDB first, so that we can get the map from object file type and item
     // indices to PDB type and item indices.
     SmallVector<TypeIndex, 128> TypeIndexMap;
void coff::createPDB(StringRef Path, SymbolTable *Symtab,
 
   // Add an Info stream.
   auto &InfoBuilder = Builder.getInfoBuilder();
   InfoBuilder.setAge(DI ? DI->PDB70.Age : 0);
   InfoBuilder.setSignature(time(nullptr));
 
   llvm::SmallString<128> NativePath(Path.begin(), Path.end());
   llvm::sys::fs::make_absolute(NativePath);
-  llvm::sys::path::native(NativePath);
+  llvm::sys::path::native(NativePath, llvm::sys::path::Style::windows);
 
   pdb::PDB_UniqueId uuid{};
   if (DI)
     memcpy(&uuid, &DI->PDB70.Signature, sizeof(uuid));
   InfoBuilder.setGuid(uuid);
   // Should be the current time, but set 0 for reproducibilty.
   InfoBuilder.setSignature(0);
   InfoBuilder.setVersion(pdb::PdbRaw_ImplVer::PdbImplVC70);

lld/trunk/test/COFF/pdb-diff.test

 CHECK-NEXT:   |          Section Header Data | {{[EI]}} |
 CHECK-NEXT:   |------------------------------+---|
 CHECK-NEXT:   |        Named Stream "/names" | {{[EI]}} |
 CHECK-NEXT:   |------------------------------+---|
 CHECK-NEXT:   |     Named Stream "/LinkInfo" | {{[EI]}} |
 CHECK-NEXT:   |------------------------------+---|
 CHECK-NEXT:   | Named Stream "/src/heade..." | D |
 CHECK-NEXT:   |------------------------------+---|
-CHECK-NEXT:   | Module "{{.*}}pdb-diff.obj" | D |
+CHECK-NEXT:   | Module "{{.*}}\pdb-diff.obj" | {{[EI]}} |
 CHECK-NEXT:   |------------------------------+---|
 CHECK-NEXT:   |          Module "* Linker *" | {{[EI]}} |
 CHECK-NEXT:   |------------------------------+---|
 CHECK-NEXT:   |                     TPI Hash | {{[EI]}} |
 CHECK-NEXT:   |------------------------------+---|
 CHECK-NEXT:   |                     IPI Hash | {{[EI]}} |
 CHECK-NEXT:   |------------------------------+---|
 CHECK-NEXT:   |           Global Symbol Hash | D |
 CHECK-NEXT:   |------------------------------+---|
 CHECK-NEXT:   |           Public Symbol Hash | D |
 CHECK-NEXT:   |------------------------------+---|
 CHECK-NEXT:   |        Public Symbol Records | D |
 CHECK-NEXT:   |------------------------------+---|
-CHECK-NEXT:   | Module "{{.*}}pdb-diff.obj" | D |
-CHECK-NEXT:   |------------------------------+---|
 CHECK-NEXT:   |                 New FPO Data | D |
 CHECK-NEXT:   |------------------------------+---|
 CHECK-NEXT:   ------------------------------------
 CHECK-NEXT:   |           String Table           |
 CHECK-NEXT:   |------------------------------+---|
 CHECK-NEXT:   |                         File |   |
 CHECK-NEXT:   |------------------------------+---|
 CHECK-NEXT:   |            Number of Strings | D |

 CHECK-NEXT:   |                Is Incrementally Linked | D |
 CHECK-NEXT:   |----------------------------------------+---|
 CHECK-NEXT:   |                            Is Stripped | I |
 CHECK-NEXT:   |----------------------------------------+---|
 CHECK-NEXT:   |                           Module Count | I |
 CHECK-NEXT:   |----------------------------------------+---|
 CHECK-NEXT:   |                      Source File Count | I |
 CHECK-NEXT:   |----------------------------------------+---|
-CHECK-NEXT:   |Module "{{.*}}Inputs/pdb-diff.obj"|
+CHECK-NEXT:   |Module "{{.*}}\pdb-diff.obj"|
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                                 - Modi | I |
 CHECK-NEXT:   |----------------------------------------+---|
-CHECK-NEXT:   |                                 - Modi | D |
+CHECK-NEXT:   |                        - Obj File Name | {{[EI]}} |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                         - Debug Stream | {{[EI]}} |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                        - C11 Byte Size | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                        - C13 Byte Size | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                           - # of files | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                  - Pdb File Path Index | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |               - Source File Name Index | I |
+CHECK-NEXT:   |----------------------------------------+---|
+CHECK-NEXT:   |                     - Symbol Byte Size | D |
 CHECK-NEXT:   |----------------------------------------+---|
 CHECK-NEXT:   |            Module "* Linker *"             |
 CHECK-NEXT:   |----------------------------------------+---|
 CHECK-NEXT:   |                                 - Modi | I |
 CHECK-NEXT:   |----------------------------------------+---|
 CHECK-NEXT:   |                        - Obj File Name | I |
 CHECK-NEXT:   |----------------------------------------+---|
 CHECK-NEXT:   |                         - Debug Stream | {{[EI]}} |
 CHECK-NEXT:   |----------------------------------------+---|
 CHECK-NEXT:   |                        - C11 Byte Size | I |
 CHECK-NEXT:   |----------------------------------------+---|
 CHECK-NEXT:   |                        - C13 Byte Size | I |
 CHECK-NEXT:   |----------------------------------------+---|
 CHECK-NEXT:   |                           - # of files | I |
 CHECK-NEXT:   |----------------------------------------+---|
 CHECK-NEXT:   |                  - Pdb File Path Index | {{[EI]}} |
 CHECK-NEXT:   |----------------------------------------+---|
 CHECK-NEXT:   |               - Source File Name Index | {{[EI]}} |
 CHECK-NEXT:   |----------------------------------------+---|
 CHECK-NEXT:   |                     - Symbol Byte Size | D |
 CHECK-NEXT:   |----------------------------------------+---|
+
+

llvm/trunk/tools/llvm-pdbutil/Diff.cpp

 getModuleDescriptors(const DbiModuleList &ML) {
   std::vector<std::pair<uint32_t, DbiModuleDescriptor>> List;
   List.reserve(ML.getModuleCount());
   for (uint32_t I = 0; I < ML.getModuleCount(); ++I)
     List.emplace_back(I, ML.getModuleDescriptor(I));
   return List;
 }
 
+static void diffOneModule(
+    DiffPrinter &D, const std::pair<uint32_t, DbiModuleDescriptor> Item,
+    std::vector<std::pair<uint32_t, DbiModuleDescriptor>> &Other, bool Invert) {
+  D.printFullRow(
+      truncateQuotedNameFront("Module", Item.second.getModuleName(), 70));
+
+  auto Iter = llvm::find_if(
+      Other, [&Item](const std::pair<uint32_t, DbiModuleDescriptor> &Other) {
+        return Other.second.getModuleName().equals_lower(
+            Item.second.getModuleName());
+      });
+  if (Iter == Other.end()) {
+    // We didn't find this module at all on the other side.  Just print one row
+    // and continue.
+    D.print<ModiProvider>("- Modi", Item.first, None);
+    return;
+  }
+
+  // We did find this module.  Go through and compare each field.
+  const auto *L = &Item;
+  const auto *R = &*Iter;
+  if (Invert)
+    std::swap(L, R);
+
+  D.print<ModiProvider>("- Modi", L->first, R->first);
+  D.print<StringProvider>("- Obj File Name",
+                          shortFilePath(L->second.getObjFileName(), 28),
+                          shortFilePath(R->second.getObjFileName(), 28));
+  D.print<StreamNumberProvider>("- Debug Stream",
+                                L->second.getModuleStreamIndex(),
+                                R->second.getModuleStreamIndex());
+  D.print("- C11 Byte Size", L->second.getC11LineInfoByteSize(),
+          R->second.getC11LineInfoByteSize());
+  D.print("- C13 Byte Size", L->second.getC13LineInfoByteSize(),
+          R->second.getC13LineInfoByteSize());
+  D.print("- # of files", L->second.getNumberOfFiles(),
+          R->second.getNumberOfFiles());
+  D.print("- Pdb File Path Index", L->second.getPdbFilePathNameIndex(),
+          R->second.getPdbFilePathNameIndex());
+  D.print("- Source File Name Index", L->second.getSourceFileNameIndex(),
+          R->second.getSourceFileNameIndex());
+  D.print("- Symbol Byte Size", L->second.getSymbolDebugInfoByteSize(),
+          R->second.getSymbolDebugInfoByteSize());
+  Other.erase(Iter);
+}
+
 Error DiffStyle::diffDbiStream() {
   DiffPrinter D(2, "DBI Stream", 40, 30, opts::diff::PrintResultColumn,
                 opts::diff::PrintValueColumns, outs());
   D.printExplicit("File", DiffResult::UNSPECIFIED,
                   shortFilePath(File1.getFilePath(), 28),
                   shortFilePath(File2.getFilePath(), 28));
 
   auto ExpectedDbi1 = File1.getPDBDbiStream();
Error DiffStyle::diffDbiStream() {
   const DbiModuleList &MR = DR.modules();
   D.print("Module Count", ML.getModuleCount(), MR.getModuleCount());
   D.print("Source File Count", ML.getSourceFileCount(),
           MR.getSourceFileCount());
   auto MDL = getModuleDescriptors(ML);
   auto MDR = getModuleDescriptors(MR);
   // Scan all module descriptors from the left, and look for corresponding
   // module descriptors on the right.
-  for (const auto &L : MDL) {
-    D.printFullRow(
-        truncateQuotedNameFront("Module", L.second.getModuleName(), 70));
-
-    auto Iter = llvm::find_if(
-        MDR, [&L](const std::pair<uint32_t, DbiModuleDescriptor> &R) {
-          return R.second.getModuleName().equals_lower(
-              L.second.getModuleName());
-        });
-    if (Iter == MDR.end()) {
-      // We didn't find this module at all on the right.  Just print one row
-      // and continue.
-      D.print<ModiProvider>("- Modi", L.first, None);
-      continue;
-    }
+  for (const auto &L : MDL)
+    diffOneModule(D, L, MDR, false);
 
-    // We did find this module.  Go through and compare each field.
-    const auto &R = *Iter;
-    D.print<ModiProvider>("- Modi", L.first, R.first);
-    D.print<StringProvider>("- Obj File Name",
-                            shortFilePath(L.second.getObjFileName(), 28),
-                            shortFilePath(R.second.getObjFileName(), 28));
-    D.print<StreamNumberProvider>("- Debug Stream",
-                                  L.second.getModuleStreamIndex(),
-                                  R.second.getModuleStreamIndex());
-    D.print("- C11 Byte Size", L.second.getC11LineInfoByteSize(),
-            R.second.getC11LineInfoByteSize());
-    D.print("- C13 Byte Size", L.second.getC13LineInfoByteSize(),
-            R.second.getC13LineInfoByteSize());
-    D.print("- # of files", L.second.getNumberOfFiles(),
-            R.second.getNumberOfFiles());
-    D.print("- Pdb File Path Index", L.second.getPdbFilePathNameIndex(),
-            R.second.getPdbFilePathNameIndex());
-    D.print("- Source File Name Index", L.second.getSourceFileNameIndex(),
-            R.second.getSourceFileNameIndex());
-    D.print("- Symbol Byte Size", L.second.getSymbolDebugInfoByteSize(),
-            R.second.getSymbolDebugInfoByteSize());
-    MDR.erase(Iter);
-  }
+  for (const auto &R : MDR)
+    diffOneModule(D, R, MDL, true);
 
   return Error::success();
 }
 
 Error DiffStyle::diffSectionContribs() { return Error::success(); }
 
 Error DiffStyle::diffSectionMap() { return Error::success(); }
 

llvm/trunk/tools/llvm-pdbutil/DiffPrinter.cpp

case DiffResult::EQUIVALENT:
     printValue("E", Result, AlignStyle::Center, 3, true);
     break;
   case DiffResult::IDENTICAL:
     printValue("I", Result, AlignStyle::Center, 3, true);
     break;
   case DiffResult::UNSPECIFIED:
     printValue(" ", Result, AlignStyle::Center, 3, true);
     break;
-  default:
-    llvm_unreachable("unreachable!");
   }
 }
 
 void DiffPrinter::printValue(StringRef Value, DiffResult C, AlignStyle Style,
                              uint32_t Width, bool Force) {
   if (!Force && !PrintValues)
     return;