Reland [LLD] [COFF] Fix linking MSVC generated implib header objects #123916
+46
−37
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-lld Author: Martin Storsjö (mstorsjo) Changesecb5ea6 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 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). This is a reland of a previous version of this commit, earlier merged in 9457418 / #122811. The previous version failed tests when run with address sanitizer. The issue was that the synthesized coff_symbol_generic object actually will be used to access a full coff_symbol16 or coff_symbol32 struct, see DefinedCOFF::getCOFFSymbol. Therefore, we need to make a copy of the full size of either of them. Full diff: https://github.com/llvm/llvm-project/pull/123916.diff 4 Files Affected:
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 5ee73d4dc4f8b7..fe1135db636cbc 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -105,6 +105,18 @@ static bool ignoredSymbolName(StringRef name) {
return name == "@feat.00" || name == "@comp.id";
}
+static coff_symbol_generic *cloneSymbol(COFFSymbolRef sym) {
+ if (sym.isBigObj()) {
+ auto *copy = make<coff_symbol32>(
+ *reinterpret_cast<const coff_symbol32 *>(sym.getRawPtr()));
+ return reinterpret_cast<coff_symbol_generic *>(copy);
+ } else {
+ auto *copy = make<coff_symbol16>(
+ *reinterpret_cast<const coff_symbol16 *>(sym.getRawPtr()));
+ return reinterpret_cast<coff_symbol_generic *>(copy);
+ }
+}
+
ArchiveFile::ArchiveFile(COFFLinkerContext &ctx, MemoryBufferRef m)
: InputFile(ctx.symtab, ArchiveKind, m) {}
@@ -458,9 +470,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 = cloneSymbol(sym);
+ customSymGen->Value = 0;
+ symGen = customSymGen;
+ }
return make<DefinedRegular>(this, /*Name*/ "", /*IsCOMDAT*/ false,
- /*IsExternal*/ false, sym.getGeneric(), sc);
+ /*IsExternal*/ false, symGen, sc);
+ }
return nullptr;
}
@@ -755,15 +774,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);
+
+ auto *symGen = cloneSymbol(sym);
+ // Ignore the Value offset of these symbols, as it may be a bitmask.
+ symGen->Value = 0;
return make<DefinedRegular>(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 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/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
-...
|
|
@llvm/pr-subscribers-lld-coff Author: Martin Storsjö (mstorsjo) Changesecb5ea6 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 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). This is a reland of a previous version of this commit, earlier merged in 9457418 / #122811. The previous version failed tests when run with address sanitizer. The issue was that the synthesized coff_symbol_generic object actually will be used to access a full coff_symbol16 or coff_symbol32 struct, see DefinedCOFF::getCOFFSymbol. Therefore, we need to make a copy of the full size of either of them. Full diff: https://github.com/llvm/llvm-project/pull/123916.diff 4 Files Affected:
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 5ee73d4dc4f8b7..fe1135db636cbc 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -105,6 +105,18 @@ static bool ignoredSymbolName(StringRef name) {
return name == "@feat.00" || name == "@comp.id";
}
+static coff_symbol_generic *cloneSymbol(COFFSymbolRef sym) {
+ if (sym.isBigObj()) {
+ auto *copy = make<coff_symbol32>(
+ *reinterpret_cast<const coff_symbol32 *>(sym.getRawPtr()));
+ return reinterpret_cast<coff_symbol_generic *>(copy);
+ } else {
+ auto *copy = make<coff_symbol16>(
+ *reinterpret_cast<const coff_symbol16 *>(sym.getRawPtr()));
+ return reinterpret_cast<coff_symbol_generic *>(copy);
+ }
+}
+
ArchiveFile::ArchiveFile(COFFLinkerContext &ctx, MemoryBufferRef m)
: InputFile(ctx.symtab, ArchiveKind, m) {}
@@ -458,9 +470,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 = cloneSymbol(sym);
+ customSymGen->Value = 0;
+ symGen = customSymGen;
+ }
return make<DefinedRegular>(this, /*Name*/ "", /*IsCOMDAT*/ false,
- /*IsExternal*/ false, sym.getGeneric(), sc);
+ /*IsExternal*/ false, symGen, sc);
+ }
return nullptr;
}
@@ -755,15 +774,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);
+
+ auto *symGen = cloneSymbol(sym);
+ // Ignore the Value offset of these symbols, as it may be a bitmask.
+ symGen->Value = 0;
return make<DefinedRegular>(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 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/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
-...
|
|
@llvm/pr-subscribers-llvm-binary-utilities Author: Martin Storsjö (mstorsjo) Changesecb5ea6 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 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). This is a reland of a previous version of this commit, earlier merged in 9457418 / #122811. The previous version failed tests when run with address sanitizer. The issue was that the synthesized coff_symbol_generic object actually will be used to access a full coff_symbol16 or coff_symbol32 struct, see DefinedCOFF::getCOFFSymbol. Therefore, we need to make a copy of the full size of either of them. Full diff: https://github.com/llvm/llvm-project/pull/123916.diff 4 Files Affected:
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 5ee73d4dc4f8b7..fe1135db636cbc 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -105,6 +105,18 @@ static bool ignoredSymbolName(StringRef name) {
return name == "@feat.00" || name == "@comp.id";
}
+static coff_symbol_generic *cloneSymbol(COFFSymbolRef sym) {
+ if (sym.isBigObj()) {
+ auto *copy = make<coff_symbol32>(
+ *reinterpret_cast<const coff_symbol32 *>(sym.getRawPtr()));
+ return reinterpret_cast<coff_symbol_generic *>(copy);
+ } else {
+ auto *copy = make<coff_symbol16>(
+ *reinterpret_cast<const coff_symbol16 *>(sym.getRawPtr()));
+ return reinterpret_cast<coff_symbol_generic *>(copy);
+ }
+}
+
ArchiveFile::ArchiveFile(COFFLinkerContext &ctx, MemoryBufferRef m)
: InputFile(ctx.symtab, ArchiveKind, m) {}
@@ -458,9 +470,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 = cloneSymbol(sym);
+ customSymGen->Value = 0;
+ symGen = customSymGen;
+ }
return make<DefinedRegular>(this, /*Name*/ "", /*IsCOMDAT*/ false,
- /*IsExternal*/ false, sym.getGeneric(), sc);
+ /*IsExternal*/ false, symGen, sc);
+ }
return nullptr;
}
@@ -755,15 +774,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);
+
+ auto *symGen = cloneSymbol(sym);
+ // Ignore the Value offset of these symbols, as it may be a bitmask.
+ symGen->Value = 0;
return make<DefinedRegular>(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 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/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
-...
|
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). This is a reland of a previous version of this commit, earlier merged in 9457418 / llvm#122811. The previous version failed tests when run with address sanitizer. The issue was that the synthesized coff_symbol_generic object actually will be used to access a full coff_symbol16 or coff_symbol32 struct, see DefinedCOFF::getCOFFSymbol. Therefore, we need to make a copy of the full size of either of them.
mstorsjo
force-pushed
the
lld-msvc-implib-header
branch
from
6575a1f to
9c21a06
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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).
This is a reland of a previous version of this commit, earlier merged in 9457418 / #122811. The previous version failed tests when run with address sanitizer. The issue was that the synthesized coff_symbol_generic object actually will be used to access a full coff_symbol16 or coff_symbol32 struct, see DefinedCOFF::getCOFFSymbol. Therefore, we need to make a copy of the full size of either of them.