Skip to content

[LLD][COFF] Allow symbols with empty chunks to have no associated output section in the PDB writer #149523

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 28, 2025

Conversation

cjacek
Copy link
Contributor

@cjacek cjacek commented Jul 18, 2025

If a chunk is empty and there are no other non-empty chunks in the same section, removeEmptySections() will remove the entire section. In this case, use a section index of 0, as the MSVC linker does, instead of asserting.

…put section

If a chunk is empty and there are no other non-empty chunks in the same section,
removeEmptySections() will remove the entire section. In this case, use a section
index of 0, as the MSVC linker does, instead of asserting.
@cjacek
Copy link
Contributor Author

cjacek commented Jul 18, 2025

This issue was reported to cause problems when building Wine:
mstorsjo/llvm-mingw@ae03577#commitcomment-162336778

In that case, it involved the recently introduced __data_start__ symbol, but the problem isn't specific to that symbol. The test included in this MR demonstrates that the issue can occur more generally.

@llvmbot
Copy link
Member

llvmbot commented Jul 18, 2025

@llvm/pr-subscribers-lld-coff

@llvm/pr-subscribers-platform-windows

Author: Jacek Caban (cjacek)

Changes

If a chunk is empty and there are no other non-empty chunks in the same section, removeEmptySections() will remove the entire section. In this case, use a section index of 0, as the MSVC linker does, instead of asserting.


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

2 Files Affected:

  • (modified) lld/COFF/PDB.cpp (+6-3)
  • (added) lld/test/COFF/pdb-empty-sec.s (+19)
diff --git a/lld/COFF/PDB.cpp b/lld/COFF/PDB.cpp
index a54ea403ba2ec..94eeae2797971 100644
--- a/lld/COFF/PDB.cpp
+++ b/lld/COFF/PDB.cpp
@@ -1135,9 +1135,12 @@ static pdb::BulkPublic createPublic(COFFLinkerContext &ctx, Defined *def) {
   pub.setFlags(flags);
 
   OutputSection *os = ctx.getOutputSection(def->getChunk());
-  assert(os && "all publics should be in final image");
-  pub.Offset = def->getRVA() - os->getRVA();
-  pub.Segment = os->sectionIndex;
+  assert((os || !def->getChunk()->getSize()) &&
+         "all publics should be in final image");
+  if (os) {
+    pub.Offset = def->getRVA() - os->getRVA();
+    pub.Segment = os->sectionIndex;
+  }
   return pub;
 }
 
diff --git a/lld/test/COFF/pdb-empty-sec.s b/lld/test/COFF/pdb-empty-sec.s
new file mode 100644
index 0000000000000..0d61447b76651
--- /dev/null
+++ b/lld/test/COFF/pdb-empty-sec.s
@@ -0,0 +1,19 @@
+// REQUIRES: x86
+
+// RUN: llvm-mc -filetype=obj -triple=x86_64-windows %s -o %t.obj
+// RUN: lld-link -dll -noentry -debug %t.obj -out:%t.dll
+// RUN: llvm-pdbutil dump -publics %t.pdb | FileCheck %s
+
+// CHECK:       Records
+// CHECK-NEXT:       0 | S_PUB32 [size = 20] `func`
+// CHECK-NEXT:           flags = none, addr = 0001:0000
+// CHECK-NEXT:      20 | S_PUB32 [size = 20] `sym`
+// CHECK-NEXT:           flags = none, addr = 0000:0000
+
+        .globl sym
+        .data
+sym:
+        .text
+        .globl func
+func:
+        ret

@llvmbot
Copy link
Member

llvmbot commented Jul 18, 2025

@llvm/pr-subscribers-lld

Author: Jacek Caban (cjacek)

Changes

If a chunk is empty and there are no other non-empty chunks in the same section, removeEmptySections() will remove the entire section. In this case, use a section index of 0, as the MSVC linker does, instead of asserting.


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

2 Files Affected:

  • (modified) lld/COFF/PDB.cpp (+6-3)
  • (added) lld/test/COFF/pdb-empty-sec.s (+19)
diff --git a/lld/COFF/PDB.cpp b/lld/COFF/PDB.cpp
index a54ea403ba2ec..94eeae2797971 100644
--- a/lld/COFF/PDB.cpp
+++ b/lld/COFF/PDB.cpp
@@ -1135,9 +1135,12 @@ static pdb::BulkPublic createPublic(COFFLinkerContext &ctx, Defined *def) {
   pub.setFlags(flags);
 
   OutputSection *os = ctx.getOutputSection(def->getChunk());
-  assert(os && "all publics should be in final image");
-  pub.Offset = def->getRVA() - os->getRVA();
-  pub.Segment = os->sectionIndex;
+  assert((os || !def->getChunk()->getSize()) &&
+         "all publics should be in final image");
+  if (os) {
+    pub.Offset = def->getRVA() - os->getRVA();
+    pub.Segment = os->sectionIndex;
+  }
   return pub;
 }
 
diff --git a/lld/test/COFF/pdb-empty-sec.s b/lld/test/COFF/pdb-empty-sec.s
new file mode 100644
index 0000000000000..0d61447b76651
--- /dev/null
+++ b/lld/test/COFF/pdb-empty-sec.s
@@ -0,0 +1,19 @@
+// REQUIRES: x86
+
+// RUN: llvm-mc -filetype=obj -triple=x86_64-windows %s -o %t.obj
+// RUN: lld-link -dll -noentry -debug %t.obj -out:%t.dll
+// RUN: llvm-pdbutil dump -publics %t.pdb | FileCheck %s
+
+// CHECK:       Records
+// CHECK-NEXT:       0 | S_PUB32 [size = 20] `func`
+// CHECK-NEXT:           flags = none, addr = 0001:0000
+// CHECK-NEXT:      20 | S_PUB32 [size = 20] `sym`
+// CHECK-NEXT:           flags = none, addr = 0000:0000
+
+        .globl sym
+        .data
+sym:
+        .text
+        .globl func
+func:
+        ret

cjacek referenced this pull request in mstorsjo/llvm-mingw Jul 18, 2025
Relevant changes include:
- Lots of work for supporting ARM64EC and ARM64X
- LLD now produces more compact string tables in executables (only
  relevant when including debug symbols)
- llvm-rc now supports multiplication/division expressions, like GNU
  windres
- Improved support for armv7 and aarch64 in libunwind (for Itanium
  ABI "forced unwinding", which I'm not aware of being used in
  practice anywhere)
- Made the libunwind, libcxxabi, libcxx and compiler-rt tests pass on
  armv7 and aarch64 mingw
- Made the llvm/clang tests pass on aarch64 mingw
Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

(It may be nice to mention "PDB" somewhere in the commit message too.)

Also for added context, this should be backported to 21.x, as this fixes a regression since f66f2fe in one build configuration, as reported in mstorsjo/llvm-mingw@ae03577#commitcomment-162336778.

@cjacek cjacek changed the title [LLD][COFF] Allow symbols with empty chunks to have no associated output section [LLD][COFF] Allow symbols with empty chunks to have no associated output section in the PDB writer Jul 28, 2025
@cjacek cjacek merged commit 1ab04fc into llvm:main Jul 28, 2025
13 checks passed
@cjacek cjacek deleted the pdb-empty-sec branch July 28, 2025 15:05
@cjacek cjacek added this to the LLVM 21.x Release milestone Jul 28, 2025
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Jul 28, 2025
@cjacek
Copy link
Contributor Author

cjacek commented Jul 28, 2025

/cherry-pick 1ab04fc

@llvmbot
Copy link
Member

llvmbot commented Jul 28, 2025

/pull-request #150969

@llvmbot llvmbot moved this from Needs Triage to Done in LLVM Release Status Jul 28, 2025
tru pushed a commit to llvmbot/llvm-project that referenced this pull request Jul 29, 2025
…put section in the PDB writer (llvm#149523)

If a chunk is empty and there are no other non-empty chunks in the same
section, `removeEmptySections()` will remove the entire section. In this
case, use a section index of 0, as the MSVC linker does, instead of
asserting.

(cherry picked from commit 1ab04fc)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants