From 10c9ebe48d1f6737e6c3b2a7c843fa429ac01ccd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Storsj=C3=B6?= 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/empty-section-decl.yaml | 13 ++++++----- llvm/include/llvm/Object/COFF.h | 7 +++--- llvm/test/Object/coff-sec-sym.test | 20 ----------------- 4 files changed, 34 insertions(+), 37 deletions(-) delete mode 100644 llvm/test/Object/coff-sec-sym.test diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp index 5ee73d4dc4f8b..74883ac9e7578 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 *symGen = sym.getGeneric(); + if (sym.isSection()) { + auto *customSymGen = make(*symGen); + customSymGen->Value = 0; + symGen = customSymGen; + } return make(this, /*Name*/ "", /*IsCOMDAT*/ false, - /*IsExternal*/ false, sym.getGeneric(), sc); + /*IsExternal*/ false, symGen, sc); + } return nullptr; } @@ -755,15 +762,23 @@ std::optional 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(this, hdr); chunks.push_back(sc); + + coff_symbol_generic *symGen = make(*sym.getGeneric()); + // Ignore the Value offset of these symbols, as it may be a bitmask. + symGen->Value = 0; return make(this, /*name=*/"", /*isCOMDAT=*/false, - /*isExternal=*/false, sym.getGeneric(), sc); + /*isExternal=*/false, symGen, sc); } if (llvm::COFF::isReservedSectionNumber(sectionNumber)) diff --git a/lld/test/COFF/empty-section-decl.yaml b/lld/test/COFF/empty-section-decl.yaml index 320df34000028..12fe6d44ebb83 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/llvm/include/llvm/Object/COFF.h b/llvm/include/llvm/Object/COFF.h index 4de2c680f57b1..3d0738c409049 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 0b7117250150d..0000000000000 --- 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 -...