Support finding pdb files from outputpath #94153

GkvJwa · 2024-06-02T08:55:16Z

Fixes #94152

llvmbot · 2024-06-02T08:55:44Z

@llvm/pr-subscribers-lld-coff

@llvm/pr-subscribers-lld

Author: None (GkvJwa)

Changes

Fixes #94152

Full diff: https://github.com/llvm/llvm-project/pull/94153.diff

1 Files Affected:

(modified) lld/COFF/InputFiles.cpp (+26-12)

diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 037fae45242c6..01e47dc45fe9a 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -735,8 +735,8 @@ void ObjFile::initializeFlags() {
             (cs.Flags & CompileSym3Flags::HotPatch) != CompileSym3Flags::None;
       }
       if (sym->kind() == SymbolKind::S_OBJNAME) {
-        auto objName = cantFail(SymbolDeserializer::deserializeAs<ObjNameSym>(
-            sym.get()));
+        auto objName =
+            cantFail(SymbolDeserializer::deserializeAs<ObjNameSym>(sym.get()));
         if (objName.Signature)
           pchSignature = objName.Signature;
       }
@@ -818,11 +818,8 @@ void ObjFile::initializeDependencies() {
   debugTypesObj = makeTpiSource(ctx, this);
 }
 
-// Make a PDB path assuming the PDB is in the same folder as the OBJ
-static std::string getPdbBaseName(ObjFile *file, StringRef tSPath) {
-  StringRef localPath =
-      !file->parentName.empty() ? file->parentName : file->getName();
-  SmallString<128> path = sys::path::parent_path(localPath);
+static std::string appendPdbPath(StringRef filePath, StringRef tSPath) {
+  SmallString<128> path = sys::path::parent_path(filePath);
 
   // Currently, type server PDBs are only created by MSVC cl, which only runs
   // on Windows, so we can assume type server paths are Windows style.
@@ -831,6 +828,20 @@ static std::string getPdbBaseName(ObjFile *file, StringRef tSPath) {
   return std::string(path);
 }
 
+// Make a PDB path assuming the PDB is in the same folder as the OBJ
+static std::string getPdbBaseNameFromObj(ObjFile *file, StringRef tSPath) {
+  StringRef localPath =
+      !file->parentName.empty() ? file->parentName : file->getName();
+
+  return appendPdbPath(localPath, tSPath);
+}
+
+// Make a PDB path assuming the PDB is in the same folder as the OutputFilePath
+static std::string getPdbBaseNameFromOutput(StringRef outputFilePath,
+                                            StringRef tSPath) {
+  return appendPdbPath(outputFilePath, tSPath);
+}
+
 // The casing of the PDB path stamped in the OBJ can differ from the actual path
 // on disk. With this, we ensure to always use lowercase as a key for the
 // pdbInputFileInstances map, at least on Windows.
@@ -843,15 +854,18 @@ static std::string normalizePdbPath(StringRef path) {
 }
 
 // If existing, return the actual PDB path on disk.
-static std::optional<std::string> findPdbPath(StringRef pdbPath,
-                                              ObjFile *dependentFile) {
+static std::optional<std::string>
+findPdbPath(StringRef pdbPath, ObjFile *dependentFile, StringRef outputFile) {
   // Ensure the file exists before anything else. In some cases, if the path
   // points to a removable device, Driver::enqueuePath() would fail with an
   // error (EAGAIN, "resource unavailable try again") which we want to skip
   // silently.
   if (llvm::sys::fs::exists(pdbPath))
     return normalizePdbPath(pdbPath);
-  std::string ret = getPdbBaseName(dependentFile, pdbPath);
+  std::string ret = getPdbBaseNameFromObj(dependentFile, pdbPath);
+  if (llvm::sys::fs::exists(ret))
+    return normalizePdbPath(ret);
+  ret = getPdbBaseNameFromOutput(outputFile, pdbPath);
   if (llvm::sys::fs::exists(ret))
     return normalizePdbPath(ret);
   return std::nullopt;
@@ -865,7 +879,7 @@ PDBInputFile::~PDBInputFile() = default;
 PDBInputFile *PDBInputFile::findFromRecordPath(const COFFLinkerContext &ctx,
                                                StringRef path,
                                                ObjFile *fromFile) {
-  auto p = findPdbPath(path.str(), fromFile);
+  auto p = findPdbPath(path.str(), fromFile, ctx.config.outputFile);
   if (!p)
     return nullptr;
   auto it = ctx.pdbInputFileInstances.find(*p);
@@ -931,7 +945,7 @@ std::optional<DILineInfo> ObjFile::getDILineInfo(uint32_t offset,
 }
 
 void ObjFile::enqueuePdbFile(StringRef path, ObjFile *fromFile) {
-  auto p = findPdbPath(path.str(), fromFile);
+  auto p = findPdbPath(path.str(), fromFile, ctx.config.outputFile);
   if (!p)
     return;
   auto it = ctx.pdbInputFileInstances.emplace(*p, nullptr);

llvmbot · 2024-06-02T08:55:44Z

@llvm/pr-subscribers-platform-windows

Author: None (GkvJwa)

Changes

Fixes #94152

Full diff: https://github.com/llvm/llvm-project/pull/94153.diff

1 Files Affected:

(modified) lld/COFF/InputFiles.cpp (+26-12)

diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 037fae45242c6..01e47dc45fe9a 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -735,8 +735,8 @@ void ObjFile::initializeFlags() {
             (cs.Flags & CompileSym3Flags::HotPatch) != CompileSym3Flags::None;
       }
       if (sym->kind() == SymbolKind::S_OBJNAME) {
-        auto objName = cantFail(SymbolDeserializer::deserializeAs<ObjNameSym>(
-            sym.get()));
+        auto objName =
+            cantFail(SymbolDeserializer::deserializeAs<ObjNameSym>(sym.get()));
         if (objName.Signature)
           pchSignature = objName.Signature;
       }
@@ -818,11 +818,8 @@ void ObjFile::initializeDependencies() {
   debugTypesObj = makeTpiSource(ctx, this);
 }
 
-// Make a PDB path assuming the PDB is in the same folder as the OBJ
-static std::string getPdbBaseName(ObjFile *file, StringRef tSPath) {
-  StringRef localPath =
-      !file->parentName.empty() ? file->parentName : file->getName();
-  SmallString<128> path = sys::path::parent_path(localPath);
+static std::string appendPdbPath(StringRef filePath, StringRef tSPath) {
+  SmallString<128> path = sys::path::parent_path(filePath);
 
   // Currently, type server PDBs are only created by MSVC cl, which only runs
   // on Windows, so we can assume type server paths are Windows style.
@@ -831,6 +828,20 @@ static std::string getPdbBaseName(ObjFile *file, StringRef tSPath) {
   return std::string(path);
 }
 
+// Make a PDB path assuming the PDB is in the same folder as the OBJ
+static std::string getPdbBaseNameFromObj(ObjFile *file, StringRef tSPath) {
+  StringRef localPath =
+      !file->parentName.empty() ? file->parentName : file->getName();
+
+  return appendPdbPath(localPath, tSPath);
+}
+
+// Make a PDB path assuming the PDB is in the same folder as the OutputFilePath
+static std::string getPdbBaseNameFromOutput(StringRef outputFilePath,
+                                            StringRef tSPath) {
+  return appendPdbPath(outputFilePath, tSPath);
+}
+
 // The casing of the PDB path stamped in the OBJ can differ from the actual path
 // on disk. With this, we ensure to always use lowercase as a key for the
 // pdbInputFileInstances map, at least on Windows.
@@ -843,15 +854,18 @@ static std::string normalizePdbPath(StringRef path) {
 }
 
 // If existing, return the actual PDB path on disk.
-static std::optional<std::string> findPdbPath(StringRef pdbPath,
-                                              ObjFile *dependentFile) {
+static std::optional<std::string>
+findPdbPath(StringRef pdbPath, ObjFile *dependentFile, StringRef outputFile) {
   // Ensure the file exists before anything else. In some cases, if the path
   // points to a removable device, Driver::enqueuePath() would fail with an
   // error (EAGAIN, "resource unavailable try again") which we want to skip
   // silently.
   if (llvm::sys::fs::exists(pdbPath))
     return normalizePdbPath(pdbPath);
-  std::string ret = getPdbBaseName(dependentFile, pdbPath);
+  std::string ret = getPdbBaseNameFromObj(dependentFile, pdbPath);
+  if (llvm::sys::fs::exists(ret))
+    return normalizePdbPath(ret);
+  ret = getPdbBaseNameFromOutput(outputFile, pdbPath);
   if (llvm::sys::fs::exists(ret))
     return normalizePdbPath(ret);
   return std::nullopt;
@@ -865,7 +879,7 @@ PDBInputFile::~PDBInputFile() = default;
 PDBInputFile *PDBInputFile::findFromRecordPath(const COFFLinkerContext &ctx,
                                                StringRef path,
                                                ObjFile *fromFile) {
-  auto p = findPdbPath(path.str(), fromFile);
+  auto p = findPdbPath(path.str(), fromFile, ctx.config.outputFile);
   if (!p)
     return nullptr;
   auto it = ctx.pdbInputFileInstances.find(*p);
@@ -931,7 +945,7 @@ std::optional<DILineInfo> ObjFile::getDILineInfo(uint32_t offset,
 }
 
 void ObjFile::enqueuePdbFile(StringRef path, ObjFile *fromFile) {
-  auto p = findPdbPath(path.str(), fromFile);
+  auto p = findPdbPath(path.str(), fromFile, ctx.config.outputFile);
   if (!p)
     return;
   auto it = ctx.pdbInputFileInstances.emplace(*p, nullptr);

GkvJwa · 2024-06-03T06:09:27Z

Hello @rnk, can you help me review it when you have time

tru · 2024-06-03T07:09:10Z

You are going to have to add a test for this as well. I think it's fine to always match the behaviour of link.exe for things like this.

GkvJwa · 2024-06-04T02:39:45Z

You are going to have to add a test for this as well. I think it's fine to always match the behaviour of link.exe for things like this.

Add test(Use the added pdb and obj files to compare before and after)

llvm-pdbutil dump -l t.pdb:

before:

                           Lines
============================================================
Mod 0000 | `{{.*}}pdb-from-outputpath-a.obj`:
Mod 0001 | `{{.*}}pdb-from-outputpath-b.obj`:
Mod 0002 | `* Linker *`:

--

after:
                           Lines
============================================================
Mod 0000 | `{{.*}}pdb-from-outputpath-a.obj`:
c:\src\llvm-project\build\a.c (SHA-256: 1112FA2413C56E03F4B5505E57C69AB45D701CFDAB923D4844A7D521FDACD940)
  0001:00000000-0000001B, line/addr entries = 4
     3 00000000      4 00000004      5 0000000C      6 00000016

Mod 0001 | `{{.*}}pdb-from-outputpath-b.obj`:
c:\src\llvm-project\build\b.c (SHA-256: D2547E2F2970ED80738146BB5A4D2B7C45C7BB71B73B541A4162F74A8EDDAF2C)
  0001:00000020-0000002D, line/addr entries = 1
     2 00000020

Mod 0002 | `* Linker *`:

GkvJwa · 2024-06-05T03:12:48Z

Hello @mstorsjo @aganea, can you help me review it

lld/COFF/InputFiles.cpp

aganea · 2024-06-05T13:08:23Z

lld/test/COFF/pdb-from-outputpath.test

+RUN: yaml2obj %S/Inputs/pdb-type-server-simple-b.yaml -o lib/b.obj
+RUN: llvm-pdbutil yaml2pdb %S/Inputs/pdb-type-server-simple-ts.yaml -pdb ts.pdb
+
+RUN: lld-link lib/a.obj lib/b.obj -debug -entry:main -nodefaultlib -out:t.exe


Can you rather extend the other test, pdb-type-server-simple.test? I think you could just cp the files where you need them in the %t folder. Also what you want to check here is the absence of warnings, FileCheck -check-prefix=OUTPUT and then OUTPUT-NOT: failed to load reference. The point is that you should run the test without your fix, and make sure it fails first. Then it should suceed with your fix applied.

The point is that you should run the test without your fix, and make sure it fails first. Then it should suceed with your fix applied.

Oh, so how to run two different lld-links in the test code

What I mean you should revert your code changes from your local git branch, and only keep the test. Compil & run, the test should fail. Then unstash or reapply your code changes, compil & run, the test should now pass.

Move to pdb-type-server-simple.test, use yaml2obj to generate an additional pdb, which can be modified to cp if necessary
Use existing checks to compare(the Summary is different)

no such file or directory: lld-link: Tpi record count: 0 lld-link: Ipi record count: 0 lld-link: warning: Cannot use debug info for 'lib/a.obj' [LNK4099] >>> failed to load reference 'C:\src\llvm-project\build\ts.pdb': no such file or directory lld-link: warning: Cannot use debug info for 'lib/b.obj' [LNK4099] >>> failed to load reference 'C:\src\llvm-project\build\ts.pdb': no such file or directory Summary -------------------------------------------------------------------------------- 2 Input OBJ files (expanded from all cmd-line inputs) 0 PDB type server dependencies 0 Precomp OBJ dependencies 0 Input type records 0 Input type records bytes 0 Merged TPI records 0 Merged IPI records 1 Output PDB strings 0 Global symbol records 0 Module symbol records 2 Public symbol records find pdb: lld-link: Tpi record count: 9 lld-link: Ipi record count: 16 Summary -------------------------------------------------------------------------------- 2 Input OBJ files (expanded from all cmd-line inputs) 1 PDB type server dependencies 0 Precomp OBJ dependencies 25 Input type records 868 Input type records bytes 9 Merged TPI records 16 Merged IPI records 3 Output PDB strings 4 Global symbol records 14 Module symbol records 2 Public symbol records

Can you rather leave the existing test unmodified and add another block after to run your test? I think you could create a %t2 folder and %t2/lib and cp the .obj and the .pdb files from the previous step? Then add another line with lld-link ... and FileCheck -check-prefix=OUTPUT and then OUTPUT-NOT: failed to load reference. You can take inspiration from other tests, such as pdb-natvis.test. Except that in your case you could pack all these steps together (mkdir(s), cp, lld-link and the OUTPUT line)

As suggested by comments, I made changes and reviewed again

lld/COFF/InputFiles.cpp

aganea · 2024-06-06T11:25:57Z

Apart from that, code changes look good, thanks!

aganea

LGTM, thanks again! Let's give it few days before merging, in case anyone has any further comments.

GkvJwa · 2024-06-07T14:17:13Z

LGTM, thanks again! Let's give it few days before merging, in case anyone has any further comments.

Thank you for your help, too

GkvJwa · 2024-06-17T14:14:32Z

LGTM, thanks again! Let's give it few days before merging, in case anyone has any further comments.

Hello, can you help me merge it recently

aganea · 2024-06-17T15:20:17Z

Merged, thanks!

llvmbot added lld lld:COFF platform:windows labels Jun 2, 2024

tru requested review from mstorsjo and aganea June 3, 2024 07:09

aganea requested changes Jun 5, 2024

View reviewed changes

aganea approved these changes Jun 7, 2024

View reviewed changes

Support finding pdb files from outputpath

38f2dda

aganea merged commit c11677e into llvm:main Jun 17, 2024
7 checks passed

Support finding pdb files from outputpath #94153

Support finding pdb files from outputpath #94153

Uh oh!

Conversation

GkvJwa commented Jun 2, 2024

Uh oh!

llvmbot commented Jun 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jun 2, 2024

Uh oh!

GkvJwa commented Jun 3, 2024

Uh oh!

tru commented Jun 3, 2024

Uh oh!

GkvJwa commented Jun 4, 2024

Uh oh!

GkvJwa commented Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aganea Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GkvJwa Jun 5, 2024

Choose a reason for hiding this comment

Uh oh!

aganea Jun 5, 2024

Choose a reason for hiding this comment

Uh oh!

GkvJwa Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aganea Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

GkvJwa Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aganea commented Jun 6, 2024

Uh oh!

aganea left a comment

Choose a reason for hiding this comment

Uh oh!

GkvJwa commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GkvJwa commented Jun 17, 2024

Uh oh!

Uh oh!

aganea commented Jun 17, 2024

Uh oh!

Uh oh!

llvmbot commented Jun 2, 2024 •

edited

Loading

GkvJwa commented Jun 5, 2024 •

edited

Loading

aganea Jun 5, 2024 •

edited

Loading

GkvJwa Jun 5, 2024 •

edited

Loading

GkvJwa commented Jun 7, 2024 •

edited

Loading