[lld] [llvm] RFC: [LLD] [COFF] Fix linking MSVC generated implib header objects (PR #122811)
Martin Storsjö via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 16 06:04:17 PST 2025
https://github.com/mstorsjo updated https://github.com/llvm/llvm-project/pull/122811
>From dc495a028143d11556d9d0f31dd7489138ff1a04 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?= <martin at martin.st>
Date: Mon, 13 Jan 2025 15:01:17 +0200
Subject: [PATCH] [LLD] [COFF] Fix linking MSVC generated implib header objects
ecb5ea6a266d5cc4e05252f6db4c73613b73cc3b tried to fix cases
when LLD links what seems to be import library header objects
from MSVC. However, the fix seems incorrect; the review at
https://reviews.llvm.org/D133627 concluded that if this (treating
this kind of symbol as a common symbol) is what link.exe does,
it's fine.
However, this is most probably not what link.exe does. The
symbol mentioned in the commit message of
ecb5ea6a266d5cc4e05252f6db4c73613b73cc3b would be a common symbol
with a size of around 3 GB; this is not what might have been
intended.
That commit tried to avoid running into the error ".idata$4 should
not refer to special section 0"; that issue is fixed for a similar
style of section symbols in
4a4a8a1476b1386b523dc5b292ba9a5a6748a9cf.
Therefore, revert ecb5ea6a266d5cc4e05252f6db4c73613b73cc3b and
extend the fix from 4a4a8a1476b1386b523dc5b292ba9a5a6748a9cf to
also work for the section symbols in MSVC generated import libraries.
The main detail about them, is that for symbols of type
IMAGE_SYM_CLASS_SECTION, the Value field is not an offset, but
it is an optional set of flags, corresponding to the Characteristics
of the section header (although it may be empty).
---
lld/COFF/InputFiles.cpp | 31 ++++++++---
lld/test/COFF/Inputs/msvc-implib1.yaml | 75 ++++++++++++++++++++++++++
lld/test/COFF/Inputs/msvc-implib2.yaml | 24 +++++++++
lld/test/COFF/Inputs/msvc-implib3.yaml | 29 ++++++++++
lld/test/COFF/empty-section-decl.yaml | 13 +++--
lld/test/COFF/wholearchive-implib.s | 20 +++++++
llvm/include/llvm/Object/COFF.h | 7 ++-
llvm/test/Object/coff-sec-sym.test | 20 -------
8 files changed, 182 insertions(+), 37 deletions(-)
create mode 100644 lld/test/COFF/Inputs/msvc-implib1.yaml
create mode 100644 lld/test/COFF/Inputs/msvc-implib2.yaml
create mode 100644 lld/test/COFF/Inputs/msvc-implib3.yaml
delete mode 100644 llvm/test/Object/coff-sec-sym.test
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 66641ff9dcc1f0..fca2cbf277ff56 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -458,9 +458,16 @@ Symbol *ObjFile::createRegular(COFFSymbolRef sym) {
return nullptr;
return symtab.addUndefined(name, this, false);
}
- if (sc)
+ if (sc) {
+ const coff_symbol_generic *sym_gen = sym.getGeneric();
+ if (sym.isSection()) {
+ auto *custom_sym_gen = make<coff_symbol_generic>(*sym_gen);
+ custom_sym_gen->Value = 0;
+ sym_gen = custom_sym_gen;
+ }
return make<DefinedRegular>(this, /*Name*/ "", /*IsCOMDAT*/ false,
- /*IsExternal*/ false, sym.getGeneric(), sc);
+ /*IsExternal*/ false, sym_gen, sc);
+ }
return nullptr;
}
@@ -755,15 +762,23 @@ std::optional<Symbol *> ObjFile::createDefined(
memset(hdr, 0, sizeof(*hdr));
strncpy(hdr->Name, name.data(),
std::min(name.size(), (size_t)COFF::NameSize));
- // We have no idea what characteristics should be assumed here; pick
- // a default. This matches what is used for .idata sections in the regular
- // object files in import libraries.
- hdr->Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ |
- IMAGE_SCN_MEM_WRITE | IMAGE_SCN_ALIGN_4BYTES;
+ // The Value field in a section symbol may contain the characteristics,
+ // or it may be zero, where we make something up (that matches what is
+ // used in .idata sections in the regular object files in import libraries).
+ if (sym.getValue())
+ hdr->Characteristics = sym.getValue() | IMAGE_SCN_ALIGN_4BYTES;
+ else
+ hdr->Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA |
+ IMAGE_SCN_MEM_READ | IMAGE_SCN_MEM_WRITE |
+ IMAGE_SCN_ALIGN_4BYTES;
auto *sc = make<SectionChunk>(this, hdr);
chunks.push_back(sc);
+
+ coff_symbol_generic *sym_gen = make<coff_symbol_generic>(*sym.getGeneric());
+ // Ignore the Value offset of these symbols, as it may be a bitmask.
+ sym_gen->Value = 0;
return make<DefinedRegular>(this, /*name=*/"", /*isCOMDAT=*/false,
- /*isExternal=*/false, sym.getGeneric(), sc);
+ /*isExternal=*/false, sym_gen, sc);
}
if (llvm::COFF::isReservedSectionNumber(sectionNumber))
diff --git a/lld/test/COFF/Inputs/msvc-implib1.yaml b/lld/test/COFF/Inputs/msvc-implib1.yaml
new file mode 100644
index 00000000000000..874913b527e406
--- /dev/null
+++ b/lld/test/COFF/Inputs/msvc-implib1.yaml
@@ -0,0 +1,75 @@
+--- !COFF
+header:
+ Machine: IMAGE_FILE_MACHINE_AMD64
+ Characteristics: [ ]
+sections:
+ - Name: '.idata$2'
+ Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+ Alignment: 4
+ SectionData: '0000000000000000000000000000000000000000'
+ SizeOfRawData: 20
+ Relocations:
+ - VirtualAddress: 12
+ SymbolName: '.idata$6'
+ Type: IMAGE_REL_AMD64_ADDR32NB
+ - VirtualAddress: 0
+ SymbolName: '.idata$4'
+ Type: IMAGE_REL_AMD64_ADDR32NB
+ - VirtualAddress: 16
+ SymbolName: '.idata$5'
+ Type: IMAGE_REL_AMD64_ADDR32NB
+ - Name: '.idata$6'
+ Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+ Alignment: 2
+ SectionData: 666F6F2E646C6C00
+ SizeOfRawData: 8
+symbols:
+ - Name: '@comp.id'
+ Value: 16877185
+ SectionNumber: -1
+ SimpleType: IMAGE_SYM_TYPE_NULL
+ ComplexType: IMAGE_SYM_DTYPE_NULL
+ StorageClass: IMAGE_SYM_CLASS_STATIC
+ - Name: __IMPORT_DESCRIPTOR_foo
+ Value: 0
+ SectionNumber: 1
+ SimpleType: IMAGE_SYM_TYPE_NULL
+ ComplexType: IMAGE_SYM_DTYPE_NULL
+ StorageClass: IMAGE_SYM_CLASS_EXTERNAL
+ - Name: '.idata$2'
+ Value: 3221225536
+ SectionNumber: 1
+ SimpleType: IMAGE_SYM_TYPE_NULL
+ ComplexType: IMAGE_SYM_DTYPE_NULL
+ StorageClass: IMAGE_SYM_CLASS_SECTION
+ - Name: '.idata$6'
+ Value: 0
+ SectionNumber: 2
+ SimpleType: IMAGE_SYM_TYPE_NULL
+ ComplexType: IMAGE_SYM_DTYPE_NULL
+ StorageClass: IMAGE_SYM_CLASS_STATIC
+ - Name: '.idata$4'
+ Value: 3221225536
+ SectionNumber: 0
+ SimpleType: IMAGE_SYM_TYPE_NULL
+ ComplexType: IMAGE_SYM_DTYPE_NULL
+ StorageClass: IMAGE_SYM_CLASS_SECTION
+ - Name: '.idata$5'
+ Value: 3221225536
+ SectionNumber: 0
+ SimpleType: IMAGE_SYM_TYPE_NULL
+ ComplexType: IMAGE_SYM_DTYPE_NULL
+ StorageClass: IMAGE_SYM_CLASS_SECTION
+ - Name: __NULL_IMPORT_DESCRIPTOR
+ Value: 0
+ SectionNumber: 0
+ SimpleType: IMAGE_SYM_TYPE_NULL
+ ComplexType: IMAGE_SYM_DTYPE_NULL
+ StorageClass: IMAGE_SYM_CLASS_EXTERNAL
+ - Name: "foo_NULL_THUNK_DATA"
+ Value: 0
+ SectionNumber: 0
+ SimpleType: IMAGE_SYM_TYPE_NULL
+ ComplexType: IMAGE_SYM_DTYPE_NULL
+ StorageClass: IMAGE_SYM_CLASS_EXTERNAL
+...
diff --git a/lld/test/COFF/Inputs/msvc-implib2.yaml b/lld/test/COFF/Inputs/msvc-implib2.yaml
new file mode 100644
index 00000000000000..50cb1df842f9cd
--- /dev/null
+++ b/lld/test/COFF/Inputs/msvc-implib2.yaml
@@ -0,0 +1,24 @@
+--- !COFF
+header:
+ Machine: IMAGE_FILE_MACHINE_AMD64
+ Characteristics: [ ]
+sections:
+ - Name: '.idata$3'
+ Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+ Alignment: 4
+ SectionData: '0000000000000000000000000000000000000000'
+ SizeOfRawData: 20
+symbols:
+ - Name: '@comp.id'
+ Value: 16877185
+ SectionNumber: -1
+ SimpleType: IMAGE_SYM_TYPE_NULL
+ ComplexType: IMAGE_SYM_DTYPE_NULL
+ StorageClass: IMAGE_SYM_CLASS_STATIC
+ - Name: __NULL_IMPORT_DESCRIPTOR
+ Value: 0
+ SectionNumber: 1
+ SimpleType: IMAGE_SYM_TYPE_NULL
+ ComplexType: IMAGE_SYM_DTYPE_NULL
+ StorageClass: IMAGE_SYM_CLASS_EXTERNAL
+...
diff --git a/lld/test/COFF/Inputs/msvc-implib3.yaml b/lld/test/COFF/Inputs/msvc-implib3.yaml
new file mode 100644
index 00000000000000..2cc78a7048f96e
--- /dev/null
+++ b/lld/test/COFF/Inputs/msvc-implib3.yaml
@@ -0,0 +1,29 @@
+--- !COFF
+header:
+ Machine: IMAGE_FILE_MACHINE_AMD64
+ Characteristics: [ ]
+sections:
+ - Name: '.idata$5'
+ Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+ Alignment: 8
+ SectionData: '0000000000000000'
+ SizeOfRawData: 8
+ - Name: '.idata$4'
+ Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+ Alignment: 8
+ SectionData: '0000000000000000'
+ SizeOfRawData: 8
+symbols:
+ - Name: '@comp.id'
+ Value: 16877185
+ SectionNumber: -1
+ SimpleType: IMAGE_SYM_TYPE_NULL
+ ComplexType: IMAGE_SYM_DTYPE_NULL
+ StorageClass: IMAGE_SYM_CLASS_STATIC
+ - Name: "foo_NULL_THUNK_DATA"
+ Value: 0
+ SectionNumber: 1
+ SimpleType: IMAGE_SYM_TYPE_NULL
+ ComplexType: IMAGE_SYM_DTYPE_NULL
+ StorageClass: IMAGE_SYM_CLASS_EXTERNAL
+...
diff --git a/lld/test/COFF/empty-section-decl.yaml b/lld/test/COFF/empty-section-decl.yaml
index 320df340000289..12fe6d44ebb832 100644
--- a/lld/test/COFF/empty-section-decl.yaml
+++ b/lld/test/COFF/empty-section-decl.yaml
@@ -6,7 +6,7 @@
# RUN: FileCheck %s --check-prefix=MAP < %t.map
# CHECK: Contents of section .itest:
-# CHECK-NEXT: 180001000 0c100080 01000000 00000000 01000000
+# CHECK-NEXT: 180001000 0c100000 0c100000 00000000 01000000
# MAP: 00001000 0000000a 4 {{.*}}:(.itest$2)
# MAP: 00001000 00000000 0 .itest$2
@@ -28,7 +28,10 @@ sections:
Relocations:
- VirtualAddress: 0
SymbolName: '.itest$4'
- Type: IMAGE_REL_AMD64_ADDR64
+ Type: IMAGE_REL_AMD64_ADDR32NB
+ - VirtualAddress: 4
+ SymbolName: '.itest$6'
+ Type: IMAGE_REL_AMD64_ADDR32NB
- Name: '.itest$6'
Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
Alignment: 2
@@ -42,13 +45,13 @@ symbols:
ComplexType: IMAGE_SYM_DTYPE_NULL
StorageClass: IMAGE_SYM_CLASS_SECTION
- Name: '.itest$6'
- Value: 0
+ Value: 3221225536
SectionNumber: 2
SimpleType: IMAGE_SYM_TYPE_NULL
ComplexType: IMAGE_SYM_DTYPE_NULL
- StorageClass: IMAGE_SYM_CLASS_STATIC
+ StorageClass: IMAGE_SYM_CLASS_SECTION
- Name: '.itest$4'
- Value: 0
+ Value: 3221225536
SectionNumber: 0
SimpleType: IMAGE_SYM_TYPE_NULL
ComplexType: IMAGE_SYM_DTYPE_NULL
diff --git a/lld/test/COFF/wholearchive-implib.s b/lld/test/COFF/wholearchive-implib.s
index 0c98ca0ddef072..0ed81d4adf581c 100644
--- a/lld/test/COFF/wholearchive-implib.s
+++ b/lld/test/COFF/wholearchive-implib.s
@@ -3,9 +3,18 @@
// RUN: llvm-lib -machine:amd64 -out:%t.lib -def:%t.dir/lib.def
// RUN: llvm-mc -filetype=obj -triple=x86_64-windows %t.dir/main.s -o %t.main.obj
+// RUN: yaml2obj %S/Inputs/msvc-implib1.yaml -o %t.msvc-implib1.obj
+// RUN: yaml2obj %S/Inputs/msvc-implib2.yaml -o %t.msvc-implib2.obj
+// RUN: yaml2obj %S/Inputs/msvc-implib3.yaml -o %t.msvc-implib3.obj
+// RUN: llvm-lib -out:%t-msvc.lib %t.msvc-implib1.obj %t.msvc-implib2.obj %t.msvc-implib3.obj
+// RUN: llvm-mc -filetype=obj -triple=x86_64-windows %t.dir/main-nocall.s -o %t.main-nocall.obj
+
// RUN: lld-link -out:%t.exe %t.main.obj -wholearchive:%t.lib -entry:entry -subsystem:console
// RUN: llvm-readobj --coff-imports %t.exe | FileCheck %s
+// RUN: lld-link -out:%t-msvc.exe %t.main-nocall.obj -wholearchive:%t-msvc.lib -entry:entry -subsystem:console
+// RUN: llvm-readobj --coff-imports %t-msvc.exe | FileCheck %s --check-prefix=CHECK-MSVC
+
// As LLD usually doesn't use the header/trailer object files from import
// libraries, but instead synthesizes those structures, we end up with two
// import directory entries if we force those objects to be included.
@@ -22,6 +31,12 @@
// CHECK-NEXT: Symbol: func (0)
// CHECK-NEXT: }
+// CHECK-MSVC: Import {
+// CHECK-MSVC-NEXT: Name: foo.dll
+// CHECK-MSVC-NEXT: ImportLookupTableRVA: 0x203C
+// CHECK-MSVC-NEXT: ImportAddressTableRVA: 0x2048
+// CHECK-MSVC-NEXT: }
+
#--- main.s
.global entry
@@ -29,6 +44,11 @@ entry:
call func
ret
+#--- main-nocall.s
+.global entry
+entry:
+ ret
+
#--- lib.def
LIBRARY lib.dll
EXPORTS
diff --git a/llvm/include/llvm/Object/COFF.h b/llvm/include/llvm/Object/COFF.h
index 4de2c680f57b1a..3d0738c4090497 100644
--- a/llvm/include/llvm/Object/COFF.h
+++ b/llvm/include/llvm/Object/COFF.h
@@ -383,8 +383,8 @@ class COFFSymbolRef {
}
bool isCommon() const {
- return (isExternal() || isSection()) &&
- getSectionNumber() == COFF::IMAGE_SYM_UNDEFINED && getValue() != 0;
+ return isExternal() && getSectionNumber() == COFF::IMAGE_SYM_UNDEFINED &&
+ getValue() != 0;
}
bool isUndefined() const {
@@ -393,8 +393,7 @@ class COFFSymbolRef {
}
bool isEmptySectionDeclaration() const {
- return isSection() && getSectionNumber() == COFF::IMAGE_SYM_UNDEFINED &&
- getValue() == 0;
+ return isSection() && getSectionNumber() == COFF::IMAGE_SYM_UNDEFINED;
}
bool isWeakExternal() const {
diff --git a/llvm/test/Object/coff-sec-sym.test b/llvm/test/Object/coff-sec-sym.test
deleted file mode 100644
index 0b7117250150de..00000000000000
--- a/llvm/test/Object/coff-sec-sym.test
+++ /dev/null
@@ -1,20 +0,0 @@
-# Check that section symbol (IMAGE_SYM_CLASS_SECTION) is listed as common symbol.
-
-# RUN: yaml2obj %s -o %t.obj
-# RUN: llvm-nm %t.obj | FileCheck %s
-
-# CHECK: 00000001 C foo
-
---- !COFF
-header:
- Machine: IMAGE_FILE_MACHINE_AMD64
- Characteristics: [ ]
-sections:
-symbols:
- - Name: foo
- Value: 1
- SectionNumber: 0
- SimpleType: IMAGE_SYM_TYPE_NULL
- ComplexType: IMAGE_SYM_DTYPE_NULL
- StorageClass: IMAGE_SYM_CLASS_SECTION
-...
More information about the llvm-commits
mailing list