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

mstorsjo · mstorsjo · commit 02789ac934a8 · 2025-01-16T15:36:42.000+02:00
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).
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
@@ -458,9 +458,12 @@ Symbol *ObjFile::createRegular(COFFSymbolRef sym) {
       return nullptr;
     return symtab.addUndefined(name, this, false);
   }
-  if (sc)
+  if (sc) {
+    if (sym.isSection())
+      return make<DefinedSection>(this, sym.getGeneric(), sc);
     return make<DefinedRegular>(this, /*Name*/ "", /*IsCOMDAT*/ false,
                                 /*IsExternal*/ false, sym.getGeneric(), sc);
+  }
   return nullptr;
 }
 
@@ -755,15 +758,18 @@ 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);
-    return make<DefinedRegular>(this, /*name=*/"", /*isCOMDAT=*/false,
-                                /*isExternal=*/false, sym.getGeneric(), sc);
+    return make<DefinedSection>(this, sym.getGeneric(), sc);
   }
 
   if (llvm::COFF::isReservedSectionNumber(sectionNumber))
diff --git a/lld/COFF/Symbols.h b/lld/COFF/Symbols.h
@@ -50,6 +50,7 @@ class Symbol {
     // the higher kind when testing whether one symbol should take precedence
     // over another.
     DefinedRegularKind = 0,
+    DefinedSectionKind,
     DefinedCommonKind,
     DefinedLocalImportKind,
     DefinedImportThunkKind,
@@ -226,6 +227,31 @@ class DefinedRegular : public DefinedCOFF {
   SectionChunk **data;
 };
 
+// A symbol for a section; this is like DefinedRegular above, but ignoring
+// sym->Value in getRVA
+class DefinedSection : public DefinedCOFF {
+public:
+  DefinedSection(InputFile *f, const coff_symbol_generic *s = nullptr,
+                 SectionChunk *c = nullptr)
+      : DefinedCOFF(DefinedSectionKind, f, /*name=*/"", s),
+        data(c ? &c->repl : nullptr) {
+    this->isExternal = false;
+    this->isCOMDAT = false;
+    this->isWeak = false;
+  }
+
+  static bool classof(const Symbol *s) {
+    return s->kind() == DefinedSectionKind;
+  }
+
+  uint64_t getRVA() const { return (*data)->getRVA(); }
+  SectionChunk *getChunk() const { return *data; }
+  // No need for getValue(); it's not a virtual method and it is only present
+  // in DefinedRegular.
+
+  SectionChunk **data;
+};
+
 class DefinedCommon : public DefinedCOFF {
 public:
   DefinedCommon(InputFile *f, StringRef n, uint64_t size,
@@ -461,6 +487,8 @@ inline uint64_t Defined::getRVA() {
     return cast<DefinedCommon>(this)->getRVA();
   case DefinedRegularKind:
     return cast<DefinedRegular>(this)->getRVA();
+  case DefinedSectionKind:
+    return cast<DefinedSection>(this)->getRVA();
   case LazyArchiveKind:
   case LazyObjectKind:
   case LazyDLLSymbolKind:
@@ -486,6 +514,8 @@ inline Chunk *Defined::getChunk() {
     return cast<DefinedLocalImport>(this)->getChunk();
   case DefinedCommonKind:
     return cast<DefinedCommon>(this)->getChunk();
+  case DefinedSectionKind:
+    return cast<DefinedSection>(this)->getChunk();
   case LazyArchiveKind:
   case LazyObjectKind:
   case LazyDLLSymbolKind:
@@ -500,6 +530,7 @@ inline Chunk *Defined::getChunk() {
 // using the placement new.
 union SymbolUnion {
   alignas(DefinedRegular) char a[sizeof(DefinedRegular)];
+  alignas(DefinedSection) char l[sizeof(DefinedSection)];
   alignas(DefinedCommon) char b[sizeof(DefinedCommon)];
   alignas(DefinedAbsolute) char c[sizeof(DefinedAbsolute)];
   alignas(DefinedSynthetic) char d[sizeof(DefinedSynthetic)];
diff --git a/lld/test/COFF/Inputs/msvc-implib1.yaml 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
@@ -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
@@ -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
@@ -6,14 +6,11 @@
 # 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
 # MAP: 0000100c 00000000     4         {{.*}}:(.itest$4)
-# MAP: 0000100c 00000000     0                 .itest$4
 # MAP: 0000100c 00000004     2         {{.*}}:(.itest$6)
-# MAP: 0000100c 00000000     0                 .itest$6
 
 --- !COFF
 header:
@@ -28,7 +25,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 +42,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
@@ -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,13 +31,24 @@
 // 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
 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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -383,8 +383,8 @@ class COFFSymbolRef {`
`383`	`383`	`}`
`384`	`384`
`385`	`385`	`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;`
`388`	`388`	`}`
`389`	`389`
`390`	`390`	`bool isUndefined() const {`
`@@ -393,8 +393,7 @@ class COFFSymbolRef {`
`393`	`393`	`}`
`394`	`394`
`395`	`395`	`bool isEmptySectionDeclaration() const {`
`396`		`- return isSection() && getSectionNumber() == COFF::IMAGE_SYM_UNDEFINED &&`
`397`		`- getValue() == 0;`
	`396`	`+ return isSection() && getSectionNumber() == COFF::IMAGE_SYM_UNDEFINED;`
`398`	`397`	`}`
`399`	`398`
`400`	`399`	`bool isWeakExternal() const {`