[LLD] [COFF] Fix linking MSVC generated implib header objects #122811

mstorsjo · 2025-01-13T22:34:41Z

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).

llvmbot · 2025-01-13T22:35:15Z

@llvm/pr-subscribers-lld-coff

@llvm/pr-subscribers-platform-windows

Author: Martin Storsjö (mstorsjo)

Changes

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 #122806.

Therefore, revert ecb5ea6 and extend the fix from #122806 to also work for the section symbols in MSVC generated import libraries.

The primary strange thing about them, is that the section symbols have the value 3221225536, 0xC0000040. This value matches the Windows status code STATUS_SECTION_TOO_BIG.

If interpreted literally, this value would mean that the symbols refer to an offset of 3 GB after the start of a section. Therefore, this seemingly bogus offset needs to be handled somehow.

GNU binutils also does support linking against MSVC generated import libraries - I wonder how they do it. Do they ignore the symbol Value field for symbols of type IMAGE_SYM_CLASS_SECTION? The current hacky solution in this commit is not correct as is though.

Finally - for testing, I'm constructing the header/trailer object files from .yaml, but it's hard to construct a full proper import library that way, as the short import library files for the individual symbols can't be synthesized from .yaml yet. (And even if we could do that, it's a bit tricky to build a static library with all members named e.g. "foo.dll".)

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

8 Files Affected:

(modified) lld/COFF/InputFiles.cpp (+20)
(modified) lld/COFF/Symbols.h (+3-1)
(added) lld/test/COFF/Inputs/msvc-implib1.yaml (+75)
(added) lld/test/COFF/Inputs/msvc-implib2.yaml (+24)
(added) lld/test/COFF/Inputs/msvc-implib3.yaml (+29)
(added) lld/test/COFF/wholearchive-implib.s (+55)
(modified) llvm/include/llvm/Object/COFF.h (+6-2)
(removed) llvm/test/Object/coff-sec-sym.test (-20)

diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index a94c984cfd4870..86284f042fe7a3 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -746,6 +746,26 @@ std::optional<Symbol *> ObjFile::createDefined(
   if (sectionNumber == llvm::COFF::IMAGE_SYM_DEBUG)
     return nullptr;
 
+  if (sym.isEmptySectionDeclaration()) {
+    // As there is no coff_section in the object file for these, make a
+    // new virtual one, with everything zeroed out (i.e. an empty section),
+    // with only the name and characteristics set.
+    StringRef name = getName();
+    auto *hdr = make<coff_section>();
+    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;
+    auto *sc = make<SectionChunk>(this, hdr);
+    chunks.push_back(sc);
+    return make<DefinedRegular>(this, /*Name*/ "", /*IsCOMDAT*/ false,
+                                /*IsExternal*/ false, sym.getGeneric(), sc);
+  }
+
   if (llvm::COFF::isReservedSectionNumber(sectionNumber))
     Fatal(ctx) << toString(this) << ": " << getName()
                << " should not refer to special section "
diff --git a/lld/COFF/Symbols.h b/lld/COFF/Symbols.h
index 465d4df52c6300..6e8a9a92283530 100644
--- a/lld/COFF/Symbols.h
+++ b/lld/COFF/Symbols.h
@@ -219,7 +219,9 @@ class DefinedRegular : public DefinedCOFF {
     return s->kind() == DefinedRegularKind;
   }
 
-  uint64_t getRVA() const { return (*data)->getRVA() + sym->Value; }
+  uint64_t getRVA() const {
+    return (*data)->getRVA() + (sym->Value != 0xC0000040 ? sym->Value : 0);
+  }
   SectionChunk *getChunk() const { return *data; }
   uint32_t getValue() const { return sym->Value; }
 
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/wholearchive-implib.s b/lld/test/COFF/wholearchive-implib.s
new file mode 100644
index 00000000000000..4611d778316c42
--- /dev/null
+++ b/lld/test/COFF/wholearchive-implib.s
@@ -0,0 +1,55 @@
+// REQUIRES: x86
+// RUN: split-file %s %t.dir
+// 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.
+
+// CHECK:      Import {
+// CHECK-NEXT:   Name: lib.dll
+// CHECK-NEXT:   ImportLookupTableRVA: 0x2050
+// CHECK-NEXT:   ImportAddressTableRVA: 0x2070
+// CHECK-NEXT: }
+// CHECK-NEXT: Import {
+// CHECK-NEXT:   Name: lib.dll
+// CHECK-NEXT:   ImportLookupTableRVA: 0x2058
+// CHECK-NEXT:   ImportAddressTableRVA: 0x2078
+// CHECK-NEXT:   Symbol: func (0)
+// CHECK-NEXT: }
+
+// CHECK-MSVC:      Import {
+// CHECK-MSVC-NEXT:   Name: foo.dll
+// CHECK-MSVC-NEXT:   ImportLookupTableRVA: 0x2040
+// CHECK-MSVC-NEXT:   ImportAddressTableRVA: 0x2050
+// CHECK-MSVC-NEXT: }
+
+
+#--- main.s
+.global entry
+entry:
+  call func
+  ret
+
+#--- main-nocall.s
+.global entry
+entry:
+  ret
+
+#--- lib.def
+LIBRARY lib.dll
+EXPORTS
+func
diff --git a/llvm/include/llvm/Object/COFF.h b/llvm/include/llvm/Object/COFF.h
index 05b3587224c296..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 {
@@ -392,6 +392,10 @@ class COFFSymbolRef {
            getValue() == 0;
   }
 
+  bool isEmptySectionDeclaration() const {
+    return isSection() && getSectionNumber() == COFF::IMAGE_SYM_UNDEFINED;
+  }
+
   bool isWeakExternal() const {
     return getStorageClass() == COFF::IMAGE_SYM_CLASS_WEAK_EXTERNAL;
   }
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
-...

llvmbot · 2025-01-13T22:35:15Z

@llvm/pr-subscribers-lld