Skip to content

Commit dc495a0

Browse files
mstorsjomstorsjo
committed
[LLD] [COFF] Fix linking MSVC generated implib header objects
ecb5ea6 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 ecb5ea6 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 4a4a8a1. Therefore, revert ecb5ea6 and extend the fix from 4a4a8a1 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).
1 parent c25bd6e commit dc495a0

File tree

8 files changed

+182
-37
lines changed

8 files changed

+182
-37
lines changed

‎lld/COFF/InputFiles.cpp

+23-8
Original file line numberDiff line numberDiff line change
@@ -458,9 +458,16 @@ Symbol *ObjFile::createRegular(COFFSymbolRef sym) {
458458
return nullptr;
459459
return symtab.addUndefined(name, this, false);
460460
}
461-
if (sc)
461+
if (sc) {
462+
const coff_symbol_generic *sym_gen = sym.getGeneric();
463+
if (sym.isSection()) {
464+
auto *custom_sym_gen = make<coff_symbol_generic>(*sym_gen);
465+
custom_sym_gen->Value = 0;
466+
sym_gen = custom_sym_gen;
467+
}
462468
return make<DefinedRegular>(this, /*Name*/ "", /*IsCOMDAT*/ false,
463-
/*IsExternal*/ false, sym.getGeneric(), sc);
469+
/*IsExternal*/ false, sym_gen, sc);
470+
}
464471
return nullptr;
465472
}
466473

@@ -755,15 +762,23 @@ std::optional<Symbol *> ObjFile::createDefined(
755762
memset(hdr, 0, sizeof(*hdr));
756763
strncpy(hdr->Name, name.data(),
757764
std::min(name.size(), (size_t)COFF::NameSize));
758-
// We have no idea what characteristics should be assumed here; pick
759-
// a default. This matches what is used for .idata sections in the regular
760-
// object files in import libraries.
761-
hdr->Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ |
762-
IMAGE_SCN_MEM_WRITE | IMAGE_SCN_ALIGN_4BYTES;
765+
// The Value field in a section symbol may contain the characteristics,
766+
// or it may be zero, where we make something up (that matches what is
767+
// used in .idata sections in the regular object files in import libraries).
768+
if (sym.getValue())
769+
hdr->Characteristics = sym.getValue() | IMAGE_SCN_ALIGN_4BYTES;
770+
else
771+
hdr->Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA |
772+
IMAGE_SCN_MEM_READ | IMAGE_SCN_MEM_WRITE |
773+
IMAGE_SCN_ALIGN_4BYTES;
763774
auto *sc = make<SectionChunk>(this, hdr);
764775
chunks.push_back(sc);
776+
777+
coff_symbol_generic *sym_gen = make<coff_symbol_generic>(*sym.getGeneric());
778+
// Ignore the Value offset of these symbols, as it may be a bitmask.
779+
sym_gen->Value = 0;
765780
return make<DefinedRegular>(this, /*name=*/"", /*isCOMDAT=*/false,
766-
/*isExternal=*/false, sym.getGeneric(), sc);
781+
/*isExternal=*/false, sym_gen, sc);
767782
}
768783

769784
if (llvm::COFF::isReservedSectionNumber(sectionNumber))

‎lld/test/COFF/Inputs/msvc-implib1.yaml

+75
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
--- !COFF
2+
header:
3+
Machine: IMAGE_FILE_MACHINE_AMD64
4+
Characteristics: [ ]
5+
sections:
6+
- Name: '.idata$2'
7+
Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
8+
Alignment: 4
9+
SectionData: '0000000000000000000000000000000000000000'
10+
SizeOfRawData: 20
11+
Relocations:
12+
- VirtualAddress: 12
13+
SymbolName: '.idata$6'
14+
Type: IMAGE_REL_AMD64_ADDR32NB
15+
- VirtualAddress: 0
16+
SymbolName: '.idata$4'
17+
Type: IMAGE_REL_AMD64_ADDR32NB
18+
- VirtualAddress: 16
19+
SymbolName: '.idata$5'
20+
Type: IMAGE_REL_AMD64_ADDR32NB
21+
- Name: '.idata$6'
22+
Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
23+
Alignment: 2
24+
SectionData: 666F6F2E646C6C00
25+
SizeOfRawData: 8
26+
symbols:
27+
- Name: '@comp.id'
28+
Value: 16877185
29+
SectionNumber: -1
30+
SimpleType: IMAGE_SYM_TYPE_NULL
31+
ComplexType: IMAGE_SYM_DTYPE_NULL
32+
StorageClass: IMAGE_SYM_CLASS_STATIC
33+
- Name: __IMPORT_DESCRIPTOR_foo
34+
Value: 0
35+
SectionNumber: 1
36+
SimpleType: IMAGE_SYM_TYPE_NULL
37+
ComplexType: IMAGE_SYM_DTYPE_NULL
38+
StorageClass: IMAGE_SYM_CLASS_EXTERNAL
39+
- Name: '.idata$2'
40+
Value: 3221225536
41+
SectionNumber: 1
42+
SimpleType: IMAGE_SYM_TYPE_NULL
43+
ComplexType: IMAGE_SYM_DTYPE_NULL
44+
StorageClass: IMAGE_SYM_CLASS_SECTION
45+
- Name: '.idata$6'
46+
Value: 0
47+
SectionNumber: 2
48+
SimpleType: IMAGE_SYM_TYPE_NULL
49+
ComplexType: IMAGE_SYM_DTYPE_NULL
50+
StorageClass: IMAGE_SYM_CLASS_STATIC
51+
- Name: '.idata$4'
52+
Value: 3221225536
53+
SectionNumber: 0
54+
SimpleType: IMAGE_SYM_TYPE_NULL
55+
ComplexType: IMAGE_SYM_DTYPE_NULL
56+
StorageClass: IMAGE_SYM_CLASS_SECTION
57+
- Name: '.idata$5'
58+
Value: 3221225536
59+
SectionNumber: 0
60+
SimpleType: IMAGE_SYM_TYPE_NULL
61+
ComplexType: IMAGE_SYM_DTYPE_NULL
62+
StorageClass: IMAGE_SYM_CLASS_SECTION
63+
- Name: __NULL_IMPORT_DESCRIPTOR
64+
Value: 0
65+
SectionNumber: 0
66+
SimpleType: IMAGE_SYM_TYPE_NULL
67+
ComplexType: IMAGE_SYM_DTYPE_NULL
68+
StorageClass: IMAGE_SYM_CLASS_EXTERNAL
69+
- Name: "foo_NULL_THUNK_DATA"
70+
Value: 0
71+
SectionNumber: 0
72+
SimpleType: IMAGE_SYM_TYPE_NULL
73+
ComplexType: IMAGE_SYM_DTYPE_NULL
74+
StorageClass: IMAGE_SYM_CLASS_EXTERNAL
75+
...

‎lld/test/COFF/Inputs/msvc-implib2.yaml

+24
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
--- !COFF
2+
header:
3+
Machine: IMAGE_FILE_MACHINE_AMD64
4+
Characteristics: [ ]
5+
sections:
6+
- Name: '.idata$3'
7+
Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
8+
Alignment: 4
9+
SectionData: '0000000000000000000000000000000000000000'
10+
SizeOfRawData: 20
11+
symbols:
12+
- Name: '@comp.id'
13+
Value: 16877185
14+
SectionNumber: -1
15+
SimpleType: IMAGE_SYM_TYPE_NULL
16+
ComplexType: IMAGE_SYM_DTYPE_NULL
17+
StorageClass: IMAGE_SYM_CLASS_STATIC
18+
- Name: __NULL_IMPORT_DESCRIPTOR
19+
Value: 0
20+
SectionNumber: 1
21+
SimpleType: IMAGE_SYM_TYPE_NULL
22+
ComplexType: IMAGE_SYM_DTYPE_NULL
23+
StorageClass: IMAGE_SYM_CLASS_EXTERNAL
24+
...

‎lld/test/COFF/Inputs/msvc-implib3.yaml

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
--- !COFF
2+
header:
3+
Machine: IMAGE_FILE_MACHINE_AMD64
4+
Characteristics: [ ]
5+
sections:
6+
- Name: '.idata$5'
7+
Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
8+
Alignment: 8
9+
SectionData: '0000000000000000'
10+
SizeOfRawData: 8
11+
- Name: '.idata$4'
12+
Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
13+
Alignment: 8
14+
SectionData: '0000000000000000'
15+
SizeOfRawData: 8
16+
symbols:
17+
- Name: '@comp.id'
18+
Value: 16877185
19+
SectionNumber: -1
20+
SimpleType: IMAGE_SYM_TYPE_NULL
21+
ComplexType: IMAGE_SYM_DTYPE_NULL
22+
StorageClass: IMAGE_SYM_CLASS_STATIC
23+
- Name: "foo_NULL_THUNK_DATA"
24+
Value: 0
25+
SectionNumber: 1
26+
SimpleType: IMAGE_SYM_TYPE_NULL
27+
ComplexType: IMAGE_SYM_DTYPE_NULL
28+
StorageClass: IMAGE_SYM_CLASS_EXTERNAL
29+
...

‎lld/test/COFF/empty-section-decl.yaml

+8-5
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
# RUN: FileCheck %s --check-prefix=MAP < %t.map
77

88
# CHECK: Contents of section .itest:
9-
# CHECK-NEXT: 180001000 0c100080 01000000 00000000 01000000
9+
# CHECK-NEXT: 180001000 0c100000 0c100000 00000000 01000000
1010

1111
# MAP: 00001000 0000000a 4 {{.*}}:(.itest$2)
1212
# MAP: 00001000 00000000 0 .itest$2
@@ -28,7 +28,10 @@ sections:
2828
Relocations:
2929
- VirtualAddress: 0
3030
SymbolName: '.itest$4'
31-
Type: IMAGE_REL_AMD64_ADDR64
31+
Type: IMAGE_REL_AMD64_ADDR32NB
32+
- VirtualAddress: 4
33+
SymbolName: '.itest$6'
34+
Type: IMAGE_REL_AMD64_ADDR32NB
3235
- Name: '.itest$6'
3336
Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
3437
Alignment: 2
@@ -42,13 +45,13 @@ symbols:
4245
ComplexType: IMAGE_SYM_DTYPE_NULL
4346
StorageClass: IMAGE_SYM_CLASS_SECTION
4447
- Name: '.itest$6'
45-
Value: 0
48+
Value: 3221225536
4649
SectionNumber: 2
4750
SimpleType: IMAGE_SYM_TYPE_NULL
4851
ComplexType: IMAGE_SYM_DTYPE_NULL
49-
StorageClass: IMAGE_SYM_CLASS_STATIC
52+
StorageClass: IMAGE_SYM_CLASS_SECTION
5053
- Name: '.itest$4'
51-
Value: 0
54+
Value: 3221225536
5255
SectionNumber: 0
5356
SimpleType: IMAGE_SYM_TYPE_NULL
5457
ComplexType: IMAGE_SYM_DTYPE_NULL

‎lld/test/COFF/wholearchive-implib.s

+20
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,18 @@
33
// RUN: llvm-lib -machine:amd64 -out:%t.lib -def:%t.dir/lib.def
44
// RUN: llvm-mc -filetype=obj -triple=x86_64-windows %t.dir/main.s -o %t.main.obj
55

6+
// RUN: yaml2obj %S/Inputs/msvc-implib1.yaml -o %t.msvc-implib1.obj
7+
// RUN: yaml2obj %S/Inputs/msvc-implib2.yaml -o %t.msvc-implib2.obj
8+
// RUN: yaml2obj %S/Inputs/msvc-implib3.yaml -o %t.msvc-implib3.obj
9+
// RUN: llvm-lib -out:%t-msvc.lib %t.msvc-implib1.obj %t.msvc-implib2.obj %t.msvc-implib3.obj
10+
// RUN: llvm-mc -filetype=obj -triple=x86_64-windows %t.dir/main-nocall.s -o %t.main-nocall.obj
11+
612
// RUN: lld-link -out:%t.exe %t.main.obj -wholearchive:%t.lib -entry:entry -subsystem:console
713
// RUN: llvm-readobj --coff-imports %t.exe | FileCheck %s
814

15+
// RUN: lld-link -out:%t-msvc.exe %t.main-nocall.obj -wholearchive:%t-msvc.lib -entry:entry -subsystem:console
16+
// RUN: llvm-readobj --coff-imports %t-msvc.exe | FileCheck %s --check-prefix=CHECK-MSVC
17+
918
// As LLD usually doesn't use the header/trailer object files from import
1019
// libraries, but instead synthesizes those structures, we end up with two
1120
// import directory entries if we force those objects to be included.
@@ -22,13 +31,24 @@
2231
// CHECK-NEXT: Symbol: func (0)
2332
// CHECK-NEXT: }
2433

34+
// CHECK-MSVC: Import {
35+
// CHECK-MSVC-NEXT: Name: foo.dll
36+
// CHECK-MSVC-NEXT: ImportLookupTableRVA: 0x203C
37+
// CHECK-MSVC-NEXT: ImportAddressTableRVA: 0x2048
38+
// CHECK-MSVC-NEXT: }
39+
2540

2641
#--- main.s
2742
.global entry
2843
entry:
2944
call func
3045
ret
3146

47+
#--- main-nocall.s
48+
.global entry
49+
entry:
50+
ret
51+
3252
#--- lib.def
3353
LIBRARY lib.dll
3454
EXPORTS

‎llvm/include/llvm/Object/COFF.h

+3-4
Original file line numberDiff line numberDiff line change
@@ -383,8 +383,8 @@ class COFFSymbolRef {
383383
}
384384

385385
bool isCommon() const {
386-
return (isExternal() || isSection()) &&
387-
getSectionNumber() == COFF::IMAGE_SYM_UNDEFINED && getValue() != 0;
386+
return isExternal() && getSectionNumber() == COFF::IMAGE_SYM_UNDEFINED &&
387+
getValue() != 0;
388388
}
389389

390390
bool isUndefined() const {
@@ -393,8 +393,7 @@ class COFFSymbolRef {
393393
}
394394

395395
bool isEmptySectionDeclaration() const {
396-
return isSection() && getSectionNumber() == COFF::IMAGE_SYM_UNDEFINED &&
397-
getValue() == 0;
396+
return isSection() && getSectionNumber() == COFF::IMAGE_SYM_UNDEFINED;
398397
}
399398

400399
bool isWeakExternal() const {

‎llvm/test/Object/coff-sec-sym.test

-20
This file was deleted.

0 commit comments

Comments
 (0)