[LLD][COFF] Implement support for hybrid IAT on ARM64X #124189
Merged
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-lld-coff @llvm/pr-subscribers-lld Author: Jacek Caban (cjacek) ChangesIn hybrid images, the PE header references a single IAT for both native and EC views, merging entries where possible. When merging isn't feasible, different imports are grouped together, and ARM64X relocations are emitted as needed. Patch is 29.68 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/124189.diff 5 Files Affected:
diff --git a/lld/COFF/Chunks.cpp b/lld/COFF/Chunks.cpp
index 11e7cf4346b236..a01c69c709876b 100644
--- a/lld/COFF/Chunks.cpp
+++ b/lld/COFF/Chunks.cpp
@@ -1172,11 +1172,12 @@ uint64_t Arm64XRelocVal::get() const {
size_t Arm64XDynamicRelocEntry::getSize() const {
switch (type) {
+ case IMAGE_DVRT_ARM64X_FIXUP_TYPE_ZEROFILL:
+ return sizeof(uint16_t); // Just a header.
case IMAGE_DVRT_ARM64X_FIXUP_TYPE_VALUE:
return sizeof(uint16_t) + size; // A header and a payload.
case IMAGE_DVRT_ARM64X_FIXUP_TYPE_DELTA:
- case IMAGE_DVRT_ARM64X_FIXUP_TYPE_ZEROFILL:
- llvm_unreachable("unsupported type");
+ return 2 * sizeof(uint16_t); // A header and a delta.
}
llvm_unreachable("invalid type");
}
@@ -1186,6 +1187,9 @@ void Arm64XDynamicRelocEntry::writeTo(uint8_t *buf) const {
*out = (offset.get() & 0xfff) | (type << 12);
switch (type) {
+ case IMAGE_DVRT_ARM64X_FIXUP_TYPE_ZEROFILL:
+ *out |= ((bit_width(size) - 1) << 14); // Encode the size.
+ break;
case IMAGE_DVRT_ARM64X_FIXUP_TYPE_VALUE:
*out |= ((bit_width(size) - 1) << 14); // Encode the size.
switch (size) {
@@ -1203,8 +1207,23 @@ void Arm64XDynamicRelocEntry::writeTo(uint8_t *buf) const {
}
break;
case IMAGE_DVRT_ARM64X_FIXUP_TYPE_DELTA:
- case IMAGE_DVRT_ARM64X_FIXUP_TYPE_ZEROFILL:
- llvm_unreachable("unsupported type");
+ int delta = value.get();
+ // Negative offsets use a sign bit in the header.
+ if (delta < 0) {
+ *out |= 1 << 14;
+ delta = -delta;
+ }
+ // Depending on the value, the delta is encoded with a shift of 2 or 3 bits.
+ if (delta & 7) {
+ assert(!(delta & 3));
+ delta >>= 2;
+ } else {
+ *out |= (1 << 15);
+ delta >>= 3;
+ }
+ out[1] = delta;
+ assert(!(delta & ~0xffff));
+ break;
}
}
diff --git a/lld/COFF/DLL.cpp b/lld/COFF/DLL.cpp
index 6a3f8eb21e8475..98d7760a6b663b 100644
--- a/lld/COFF/DLL.cpp
+++ b/lld/COFF/DLL.cpp
@@ -717,6 +717,46 @@ class ExportOrdinalChunk : public NonSectionChunk {
void IdataContents::create(COFFLinkerContext &ctx) {
std::vector<std::vector<DefinedImportData *>> v = binImports(ctx, imports);
+ // Merge compatible EC and native import files in hybrid images.
+ if (ctx.hybridSymtab) {
+ for (std::vector<DefinedImportData *> &syms : v) {
+ // At this point, symbols are sorted by base name, ensuring that
+ // compatible import files, if present, are adjacent.
+ std::vector<DefinedImportData *> hybridSyms;
+ ImportFile *prev = nullptr;
+ for (DefinedImportData *sym : syms) {
+ ImportFile *file = sym->file;
+ if (!prev || file->isEC() == prev->isEC() ||
+ !file->isSameImport(prev)) {
+ hybridSyms.push_back(sym);
+ prev = file;
+ continue;
+ }
+
+ // The native variant exposes a subset of EC symbols and chunks. Use the
+ // EC variant to represent both.
+ if (file->isEC()) {
+ hybridSyms.pop_back();
+ hybridSyms.push_back(sym);
+ }
+
+ prev->hybridFile = file;
+ file->hybridFile = prev;
+ prev = nullptr;
+ }
+
+ // Sort symbols by type: native-only files first, followed by merged
+ // hybrid files, and then EC-only files.
+ llvm::stable_sort(hybridSyms,
+ [](DefinedImportData *a, DefinedImportData *b) {
+ if (a->file->hybridFile)
+ return !b->file->hybridFile && b->file->isEC();
+ return !a->file->isEC() && b->file->isEC();
+ });
+ syms = std::move(hybridSyms);
+ }
+ }
+
// Create .idata contents for each DLL.
for (std::vector<DefinedImportData *> &syms : v) {
// Create lookup and address tables. If they have external names,
@@ -724,19 +764,52 @@ void IdataContents::create(COFFLinkerContext &ctx) {
// If they don't (if they are import-by-ordinals), we store only
// ordinal values to the table.
size_t base = lookups.size();
+ Chunk *lookupsTerminator = nullptr, *addressesTerminator = nullptr;
for (DefinedImportData *s : syms) {
uint16_t ord = s->getOrdinal();
+ HintNameChunk *hintChunk = nullptr;
+ Chunk *lookupsChunk, *addressesChunk;
+
if (s->getExternalName().empty()) {
- lookups.push_back(make<OrdinalOnlyChunk>(ctx, ord));
- addresses.push_back(make<OrdinalOnlyChunk>(ctx, ord));
+ lookupsChunk = make<OrdinalOnlyChunk>(ctx, ord);
+ addressesChunk = make<OrdinalOnlyChunk>(ctx, ord);
} else {
- auto *c = make<HintNameChunk>(s->getExternalName(), ord);
- lookups.push_back(make<LookupChunk>(ctx, c));
- addresses.push_back(make<LookupChunk>(ctx, c));
- hints.push_back(c);
+ hintChunk = make<HintNameChunk>(s->getExternalName(), ord);
+ lookupsChunk = make<LookupChunk>(ctx, hintChunk);
+ addressesChunk = make<LookupChunk>(ctx, hintChunk);
+ hints.push_back(hintChunk);
}
- if (s->file->impECSym) {
+ // Detect the first EC-only import in the hybrid IAT. Emit null chunks
+ // and add an ARM64X relocation to replace it with the import for the EC
+ // view. Additionally, use the original chunks as import terminators
+ // and zero them with ARM64X relocations. Since these chunks appear
+ // after the null terminator in the native view, they are always ignored
+ // by the loader. However, MSVC emits them for some reason.
+ if (ctx.hybridSymtab && !lookupsTerminator && s->file->isEC() &&
+ !s->file->hybridFile) {
+ lookupsTerminator = lookupsChunk;
+ addressesTerminator = addressesChunk;
+ lookupsChunk = make<NullChunk>(ctx);
+ addressesChunk = make<NullChunk>(ctx);
+
+ Arm64XRelocVal relocVal = hintChunk;
+ if (!hintChunk)
+ relocVal = (1ULL << 63) | ord;
+ ctx.dynamicRelocs->add(IMAGE_DVRT_ARM64X_FIXUP_TYPE_VALUE,
+ sizeof(uint64_t), lookupsChunk, relocVal);
+ ctx.dynamicRelocs->add(IMAGE_DVRT_ARM64X_FIXUP_TYPE_VALUE,
+ sizeof(uint64_t), addressesChunk, relocVal);
+ ctx.dynamicRelocs->add(IMAGE_DVRT_ARM64X_FIXUP_TYPE_ZEROFILL,
+ sizeof(uint64_t), lookupsTerminator);
+ ctx.dynamicRelocs->add(IMAGE_DVRT_ARM64X_FIXUP_TYPE_ZEROFILL,
+ sizeof(uint64_t), addressesTerminator);
+ }
+
+ lookups.push_back(lookupsChunk);
+ addresses.push_back(addressesChunk);
+
+ if (s->file->isEC()) {
auto chunk = make<AuxImportChunk>(s->file);
auxIat.push_back(chunk);
s->file->impECSym->setLocation(chunk);
@@ -744,18 +817,27 @@ void IdataContents::create(COFFLinkerContext &ctx) {
chunk = make<AuxImportChunk>(s->file);
auxIatCopy.push_back(chunk);
s->file->auxImpCopySym->setLocation(chunk);
+ } else if (ctx.hybridSymtab) {
+ // Fill the auxiliary IAT with null chunks for native-only imports.
+ auxIat.push_back(make<NullChunk>(ctx));
+ auxIatCopy.push_back(make<NullChunk>(ctx));
}
}
// Terminate with null values.
- lookups.push_back(make<NullChunk>(ctx));
- addresses.push_back(make<NullChunk>(ctx));
- if (ctx.config.machine == ARM64EC) {
+ lookups.push_back(lookupsTerminator ? lookupsTerminator
+ : make<NullChunk>(ctx));
+ addresses.push_back(addressesTerminator ? addressesTerminator
+ : make<NullChunk>(ctx));
+ if (ctx.symtabEC) {
auxIat.push_back(make<NullChunk>(ctx));
auxIatCopy.push_back(make<NullChunk>(ctx));
}
- for (int i = 0, e = syms.size(); i < e; ++i)
+ for (int i = 0, e = syms.size(); i < e; ++i) {
syms[i]->setLocation(addresses[base + i]);
+ if (syms[i]->file->hybridFile)
+ syms[i]->file->hybridFile->impSym->setLocation(addresses[base + i]);
+ }
// Create the import table header.
dllNames.push_back(make<StringChunk>(syms[0]->getDLLName()));
@@ -763,6 +845,27 @@ void IdataContents::create(COFFLinkerContext &ctx) {
dir->lookupTab = lookups[base];
dir->addressTab = addresses[base];
dirs.push_back(dir);
+
+ if (ctx.hybridSymtab) {
+ // If native-only imports exist, emit ARM64X relocations, skipping them in
+ // the EC view.
+ uint32_t nativeOnly =
+ llvm::find_if(syms,
+ [](DefinedImportData *s) { return s->file->isEC(); }) -
+ syms.begin();
+ if (nativeOnly) {
+ ctx.dynamicRelocs->add(
+ IMAGE_DVRT_ARM64X_FIXUP_TYPE_DELTA, 0,
+ Arm64XRelocVal(
+ dir, offsetof(ImportDirectoryTableEntry, ImportLookupTableRVA)),
+ nativeOnly * sizeof(uint64_t));
+ ctx.dynamicRelocs->add(
+ IMAGE_DVRT_ARM64X_FIXUP_TYPE_DELTA, 0,
+ Arm64XRelocVal(dir, offsetof(ImportDirectoryTableEntry,
+ ImportAddressTableRVA)),
+ nativeOnly * sizeof(uint64_t));
+ }
+ }
}
// Add null terminator.
dirs.push_back(make<NullChunk>(sizeof(ImportDirectoryTableEntry), 4));
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 5ee73d4dc4f8b7..f92a8549a6cfb1 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -1100,15 +1100,21 @@ void ObjFile::enqueuePdbFile(StringRef path, ObjFile *fromFile) {
}
ImportFile::ImportFile(COFFLinkerContext &ctx, MemoryBufferRef m)
- : InputFile(ctx.symtab, ImportKind, m), live(!ctx.config.doGC) {}
+ : InputFile(ctx.getSymtab(getMachineType(m)), ImportKind, m),
+ live(!ctx.config.doGC) {}
-MachineTypes ImportFile::getMachineType() const {
+MachineTypes ImportFile::getMachineType(MemoryBufferRef m) {
uint16_t machine =
- reinterpret_cast<const coff_import_header *>(mb.getBufferStart())
- ->Machine;
+ reinterpret_cast<const coff_import_header *>(m.getBufferStart())->Machine;
return MachineTypes(machine);
}
+bool ImportFile::isSameImport(const ImportFile *other) const {
+ if (!externalName.empty())
+ return other->externalName == externalName;
+ return hdr->OrdinalHint == other->hdr->OrdinalHint;
+}
+
ImportThunkChunk *ImportFile::makeImportThunk() {
switch (hdr->Machine) {
case AMD64:
diff --git a/lld/COFF/InputFiles.h b/lld/COFF/InputFiles.h
index 823561cda247a0..21b9aeef21d4f8 100644
--- a/lld/COFF/InputFiles.h
+++ b/lld/COFF/InputFiles.h
@@ -351,11 +351,15 @@ class ImportFile : public InputFile {
explicit ImportFile(COFFLinkerContext &ctx, MemoryBufferRef m);
static bool classof(const InputFile *f) { return f->kind() == ImportKind; }
- MachineTypes getMachineType() const override;
+ MachineTypes getMachineType() const override { return getMachineType(mb); }
+ static MachineTypes getMachineType(MemoryBufferRef m);
+ bool isSameImport(const ImportFile *other) const;
+ bool isEC() const { return impECSym != nullptr; }
DefinedImportData *impSym = nullptr;
Defined *thunkSym = nullptr;
ImportThunkChunkARM64EC *impchkThunk = nullptr;
+ ImportFile *hybridFile = nullptr;
std::string dllName;
private:
diff --git a/lld/test/COFF/arm64x-import.test b/lld/test/COFF/arm64x-import.test
new file mode 100644
index 00000000000000..b9ccac9a01d329
--- /dev/null
+++ b/lld/test/COFF/arm64x-import.test
@@ -0,0 +1,487 @@
+REQUIRES: aarch64
+RUN: split-file %s %t.dir && cd %t.dir
+
+RUN: llvm-mc -filetype=obj -triple=arm64ec-windows func12-thunks-arm64ec.s -o func12-thunks-arm64ec.obj
+RUN: llvm-mc -filetype=obj -triple=aarch64-windows func12-thunks-arm64.s -o func12-thunks-arm64.obj
+RUN: llvm-mc -filetype=obj -triple=arm64ec-windows func12-arm64ec.s -o func12-arm64ec.obj
+RUN: llvm-mc -filetype=obj -triple=aarch64-windows func123-arm64.s -o func123-arm64.obj
+RUN: llvm-mc -filetype=obj -triple=arm64ec-windows func123-arm64ec.s -o func123-arm64ec.obj
+RUN: llvm-mc -filetype=obj -triple=aarch64-windows func12-arm64.s -o func12-arm64.obj
+RUN: llvm-mc -filetype=obj -triple=aarch64-windows func234-arm64.s -o func234-arm64.obj
+RUN: llvm-mc -filetype=obj -triple=arm64ec-windows func12o-arm64ec.s -o func12o-arm64ec.obj
+RUN: llvm-mc -filetype=obj -triple=aarch64-windows func34o-arm64.s -o func34o-arm64.obj
+RUN: llvm-mc -filetype=obj -triple=aarch64-windows funco-arm64.s -o funco-arm64.obj
+RUN: llvm-mc -filetype=obj -triple=arm64ec-windows icall.s -o icall.obj
+RUN: llvm-mc -filetype=obj -triple=arm64ec-windows %S/Inputs/loadconfig-arm64ec.s -o loadconfig-arm64ec.obj
+RUN: llvm-mc -filetype=obj -triple=aarch64-windows %S/Inputs/loadconfig-arm64.s -o loadconfig-arm64.obj
+RUN: llvm-lib -machine:arm64ec -def:imp.def -out:imp-arm64ec.lib
+RUN: llvm-lib -machine:arm64 -def:imp.def -out:imp-arm64.lib
+RUN: llvm-lib -machine:arm64x -def:imp.def -defArm64Native:imp.def -out:imp-arm64x.lib
+RUN: llvm-lib -machine:arm64x -def:imp-ord10.def -defArm64Native:imp.def -out:imp-ecord.lib
+RUN: llvm-lib -machine:arm64x -def:imp-ord10.def -defArm64Native:imp-ord20.def -out:imp-ecord.lib
+RUN: llvm-lib -machine:arm64x -def:imp2.def -defArm64Native:imp2.def -out:imp2.lib
+RUN: llvm-lib -machine:arm64x -def:noname-ec.def -defArm64Native:noname-native.def -out:noname.lib
+
+
+# Link to the imported func1, func2, and func1's thunks from both native and EC code.
+
+RUN: lld-link -machine:arm64x -dll -noentry -out:test-12-thunks.dll loadconfig-arm64.obj loadconfig-arm64ec.obj \
+RUN: icall.obj func12-thunks-arm64ec.obj func12-thunks-arm64.obj imp-arm64ec.lib imp-arm64.lib
+
+RUN: llvm-objdump -d test-12-thunks.dll | FileCheck --check-prefix=DISASM-12T %s
+DISASM-12T: 0000000180001000 <.text>:
+DISASM-12T-NEXT: 180001000: f0000010 adrp x16, 0x180004000
+DISASM-12T-NEXT: 180001004: f9400610 ldr x16, [x16, #0x8]
+DISASM-12T-NEXT: 180001008: d61f0200 br x16
+DISASM-12T-NEXT: ...
+DISASM-12T-NEXT: 180002000: 52800040 mov w0, #0x2 // =2
+DISASM-12T-NEXT: 180002004: d65f03c0 ret
+DISASM-12T-NEXT: 180002008: 90000030 adrp x16, 0x180006000
+DISASM-12T-NEXT: 18000200c: f9400210 ldr x16, [x16]
+DISASM-12T-NEXT: 180002010: d61f0200 br x16
+DISASM-12T-NEXT: 180002014: d000000b adrp x11, 0x180004000
+DISASM-12T-NEXT: 180002018: f940016b ldr x11, [x11]
+DISASM-12T-NEXT: 18000201c: 9000000a adrp x10, 0x180002000 <.text+0x1000>
+DISASM-12T-NEXT: 180002020: 9100f14a add x10, x10, #0x3c
+DISASM-12T-NEXT: 180002024: 17fffff7 b 0x180002000 <.text+0x1000>
+DISASM-12T-NEXT: 180002028: d000000b adrp x11, 0x180004000
+DISASM-12T-NEXT: 18000202c: f940056b ldr x11, [x11, #0x8]
+DISASM-12T-NEXT: 180002030: d0ffffea adrp x10, 0x180000000
+DISASM-12T-NEXT: 180002034: 9100014a add x10, x10, #0x0
+DISASM-12T-NEXT: 180002038: 17fffff2 b 0x180002000 <.text+0x1000>
+DISASM-12T-NEXT: 18000203c: 52800060 mov w0, #0x3 // =3
+DISASM-12T-NEXT: 180002040: d65f03c0 ret
+DISASM-12T-NEXT: ...
+DISASM-12T-NEXT: 180003000: ff 25 fa 0f 00 00 jmpq *0xffa(%rip) # 0x180004000
+
+RUN: llvm-readobj --coff-imports test-12-thunks.dll | FileCheck --check-prefix=IMPORTS-12 %s
+IMPORTS-12: Import {
+IMPORTS-12-NEXT: Name: test.dll
+IMPORTS-12-NEXT: ImportLookupTableRVA: 0x5348
+IMPORTS-12-NEXT: ImportAddressTableRVA: 0x4000
+IMPORTS-12-NEXT: Symbol: func1 (0)
+IMPORTS-12-NEXT: Symbol: func2 (0)
+IMPORTS-12-NEXT: }
+IMPORTS-12-NEXT: HybridObject {
+IMPORTS-12: Import {
+IMPORTS-12-NEXT: Name: test.dll
+IMPORTS-12-NEXT: ImportLookupTableRVA: 0x5348
+IMPORTS-12-NEXT: ImportAddressTableRVA: 0x4000
+IMPORTS-12-NEXT: Symbol: func1 (0)
+IMPORTS-12-NEXT: Symbol: func2 (0)
+IMPORTS-12-NEXT: }
+IMPORTS-12-NEXT: }
+
+RUN: llvm-readobj --hex-dump=.test test-12-thunks.dll | FileCheck --check-prefix=FUNC-12-THUNKS %s
+FUNC-12-THUNKS: 0x180009000 00600000 00400000 00300000 08200000
+FUNC-12-THUNKS-NEXT: 0x180009010 08600000 08400000
+
+RUN: llvm-readobj --hex-dump=.testa test-12-thunks.dll | FileCheck --check-prefix=FUNC-12-THUNKSA %s
+FUNC-12-THUNKSA: 0x18000a000 00400000 08400000 00100000
+
+
+# If the ordinals of named imports don't match, use the EC value.
+
+RUN: lld-link -machine:arm64x -dll -noentry -out:test-12-thunks-ord.dll loadconfig-arm64.obj loadconfig-arm64ec.obj \
+RUN: icall.obj func12-thunks-arm64ec.obj func12-thunks-arm64.obj imp-ecord.lib
+RUN: llvm-readobj --coff-imports test-12-thunks-ord.dll | FileCheck --check-prefix=IMPORTS-ORD %s
+
+IMPORTS-ORD: Format: COFF-ARM64X
+IMPORTS-ORD-NEXT: Arch: aarch64
+IMPORTS-ORD-NEXT: AddressSize: 64bit
+IMPORTS-ORD-NEXT: Import {
+IMPORTS-ORD-NEXT: Name: test.dll
+IMPORTS-ORD-NEXT: ImportLookupTableRVA: 0x5348
+IMPORTS-ORD-NEXT: ImportAddressTableRVA: 0x4000
+IMPORTS-ORD-NEXT: Symbol: func1 (11)
+IMPORTS-ORD-NEXT: Symbol: func2 (12)
+IMPORTS-ORD-NEXT: }
+IMPORTS-ORD-NEXT: HybridObject {
+IMPORTS-ORD-NEXT: Format: COFF-ARM64EC
+IMPORTS-ORD-NEXT: Arch: aarch64
+IMPORTS-ORD-NEXT: AddressSize: 64bit
+IMPORTS-ORD-NEXT: Import {
+IMPORTS-ORD-NEXT: Name: test.dll
+IMPORTS-ORD-NEXT: ImportLookupTableRVA: 0x5348
+IMPORTS-ORD-NEXT: ImportAddressTableRVA: 0x4000
+IMPORTS-ORD-NEXT: Symbol: func1 (11)
+IMPORTS-ORD-NEXT: Symbol: func2 (12)
+IMPORTS-ORD-NEXT: }
+IMPORTS-ORD-NEXT: }
+
+
+# Link to NONAME imports.
+
+RUN: lld-link -machine:arm64x -dll -noentry -out:test-noname.dll loadconfig-arm64.obj loadconfig-arm64ec.obj \
+RUN: icall.obj func12-thunks-arm64ec.obj func12-thunks-arm64.obj noname.lib
+RUN: llvm-readobj --coff-imports test-noname.dll | FileCheck --check-prefix=IMPORTS-ORD2 %s
+
+IMPORTS-ORD2: Format: COFF-ARM64X
+IMPORTS-ORD2-NEXT: Arch: aarch64
+IMPORTS-ORD2-NEXT: AddressSize: 64bit
+IMPORTS-ORD2-NEXT: Import {
+IMPORTS-ORD2-NEXT: Name: test.dll
+IMPORTS-ORD2-NEXT: ImportLookupTableRVA: 0x5348
+IMPORTS-ORD2-NEXT: ImportAddressTableRVA: 0x4000
+IMPORTS-ORD2-NEXT: Symbol: (12)
+IMPORTS-ORD2-NEXT: Symbol: (11)
+IMPORTS-ORD2-NEXT: }
+IMPORTS-ORD2-NEXT: HybridObject {
+IMPORTS-ORD2-NEXT: Format: COFF-ARM64EC
+IMPORTS-ORD2-NEXT: Arch: aarch64
+IMPORTS-ORD2-NEXT: AddressSize: 64bit
+IMPORTS-ORD2-NEXT: Import {
+IMPORTS-ORD2-NEXT: Name: test.dll
+IMPORTS-ORD2-NEXT: ImportLookupTableRVA: 0x5350
+IMPORTS-ORD2-NEXT: ImportAddressTableRVA: 0x4008
+IMPORTS-ORD2-NEXT: Symbol: (11)
+IMPORTS-ORD2-NEXT: Symbol: (10)
+IMPORTS-ORD2-NEXT: }
+IMPORTS-ORD2-NEXT: }
+
+# Link to the imported func1 and func2 from both native and EC code, and func3 from native code.
+
+RUN: lld-link -machine:arm64x -dll -noentry -out:test2.dll loadconfig-arm64.obj loadconfig-arm64ec.obj \
+RUN: icall.obj func12-arm64ec.obj func123-arm64.obj imp-arm64x.lib
+
+RUN: llvm-readobj --coff-imports test2.dll | FileCheck --check-prefix=IMPORTS-123-12 %s
+IMPORTS-123-12: Import {
+IMPORTS-123-12-NEXT: Name: test.dll
+IMPORTS-123-12-NEXT: ImportLookupTableRVA: 0x3338
+IMPORTS-123-12-NEXT: ImportAddressTableRVA: 0x2000
+IMPORTS-123-12-NEXT: Symbol: func3 (0)
+IMPORTS-123-12-NEXT: Symbol: func1 (0)
+IMPORTS-123-12-NEXT: Symbol: func2 (0)
+IMPORTS-123-12-NEXT: }
+IMPORTS-123-12-NEXT: HybridObject {
+IMPORTS-123-12: Import {
+IMPORTS-123-12-NEXT: Name: test.dll
+IMPORTS-123-12-NEXT: ImportLookupTableRVA: 0x3340
+IMPORTS-123-12-NEXT: ImportAddressTableRVA: 0x2008
+IMPORTS-123-12-NEXT: Symbol: func1 (0)
+IMPORTS-123-12-NEXT: Symbol: func2 (0)
+IMPORTS-123-12-NEXT: }
+IMPORTS-123-12-NEXT: }
+
+RUN: llvm-readobj --hex-dump=.test test2.dll | FileCheck --check-prefix=TEST-123-12 %s
+TEST-123-12: 0x180007000 08400000 08200000 10400000 10200000
+
+RUN: llvm-readobj --hex-dump=.testa test2.dll | FileCheck --check-prefix=TEST-123-12A %s
+TEST-123-12A: 0x180008000 08200000 10200000 00200000
+
+RUN: llvm-readobj --hex-dump=.rdata test2.dll | FileCheck --check-prefix=TEST-123-12AUX %s
+TEST-123-12AUX: 0x180004000 00000000 00000000 08100080 01000000
+TEST-123-12AUX-NEXT: 0x180004010 1c100080 01000000 00000000 00000000
+
+
+# Link to the imported func1 and func2 from both native and EC code, and func3 from EC code.
+
+RUN: lld-link -machine:arm64x...
[truncated]
|
|
As far as I can tell, this approach is compatible with how MSVC arranges hybrid IAT. However, I've noticed two differences:
|
|
Note that you have one failing test on the Windows CI job (on buildkit) https://buildkite.com/llvm-project/github-pull-requests/builds/140034#019494f4-99b5-464b-b932-d46371db3949: |
mstorsjo
reviewed
Jan 24, 2025
| } | ||
| // Depending on the value, the delta is encoded with a shift of 2 or 3 bits. | ||
| if (delta & 7) { | ||
| assert(!(delta & 3)); |
There was a problem hiding this comment.
Can you trigger this assert with quirky user input, or is it a true linker bug if we hit it?
There was a problem hiding this comment.
If we encounter this, it's a linker bug; the delta must always be a multiple of 4. If other deltas are ever required, we will need to use IMAGE_DVRT_ARM64X_FIXUP_TYPE_VALUE instead.
| // The native variant exposes a subset of EC symbols and chunks. Use the | ||
| // EC variant to represent both. | ||
| if (file->isEC()) { | ||
| hybridSyms.pop_back(); |
There was a problem hiding this comment.
Hmm, I feel unsure about this logic here, or about the assumptions it makes. Can you add more comments about how the logic of this loop works, and the assumptions? The comments currently do explain the highlevel goals of it, but I don't feel confident in the dedup logic.
If there are both native and EC imports, are they ordered so that the native one comes first - is this assumed and guaranteed?
I think what I may have a hard time wrapping my head around, is when the loop is written with the early-continue style (which may be policy and anyway generally is good structure) - the codepath we have here is essentially the special case, for prev && file->isEC() != prev->isEC() && file->isSameImport(prev), i.e. deduplication. With that bit clarified for me, I have an easier time to understand the rest of the loop.
Is the check for isEC() required for that aspect of the deduplication? Having isSameImport() be true while file->isEC() == prev->isEC() seems unexpected. Or is it only included as a quicker way to decide this case without needing to do string comparisons?
There was a problem hiding this comment.
Hmm, I feel unsure about this logic here, or about the assumptions it makes. Can you add more comments about how the logic of this loop works, and the assumptions? The comments currently do explain the highlevel
goals of it, but I don't feel confident in the dedup logic.
I'll try to improve these comments.
If there are both native and EC imports, are they ordered so that the native one comes first - is this assumed and guaranteed?
I don't think binImports guarantees this; the order may vary. That's why I check isEC on the new symbol and swap it with the one stored in hybridSyms if the native symbol appears first.
I think what I may have a hard time wrapping my head around, is when the loop is written with the early-continue style (which may be policy and anyway generally is good structure) - the codepath we have here is essentially the special case, for
prev && file->isEC() != prev->isEC() && file->isSameImport(prev), i.e. deduplication. With that bit clarified for me, I have an easier time to understand the rest of the loop.
Yes, I can change the code to avoid using an early continue if you prefer.
Is the check for
isEC()required for that aspect of the deduplication? HavingisSameImport()be true whilefile->isEC() == prev->isEC()seems unexpected. Or is it only included as a quicker way to decide this case without needing to do string comparisons?
I think it's possible for isSameImport to return true for two EC or two native symbols in cases like this:
func1
func2 EXPORTAS func1
Since the export name is the same, isSameImport() will return true, and they might appear next to each other. I'll add a test for this scenario.
There was a problem hiding this comment.
I don't think binImports guarantees this; the order may vary. That's why I check isEC on the new symbol and swap it with the one stored in hybridSyms if the native symbol appears first.
Ok, now I see - if we're in the dedup codepath, we don't end up pushing the second symbol at all, if it was native.
Perhaps we can make it even clearer with more comments? Like this:
if (file->isEC()) {
// Native symbol was pushed to hybridSyms before, replace it with the EC one
pop_back()
push_back()
} else {
// EC symbol was pushed to hybridSyms before, keep it as is and don't push this one
}
Yes, I can change the code to avoid using an early continue if you prefer.
I'm not entirely sure if it is needed, if we have more comments for understanding the code flow.
There was a problem hiding this comment.
Since the export name is the same, isSameImport() will return true, and they might appear next to each other. I'll add a test for this scenario.
I've pushed a new version that adds a test for this. While experimenting, I noticed that MSVC is buggy when using EXPORTAS in this way. It handles the test I attached here correctly, but I was able to make it import incorrect symbols due to improper merging. It seems that MSVC compares only symbol names rather than import names, which might also explain the previously mentioned issues with NONAME handling. I didn't follow MSVC's approach here and instead kept what I believe to be the correct behavior.
I also added more comments.
| sizeof(uint64_t), lookupsChunk, relocVal); | ||
| ctx.dynamicRelocs->add(IMAGE_DVRT_ARM64X_FIXUP_TYPE_VALUE, | ||
| sizeof(uint64_t), addressesChunk, relocVal); | ||
| ctx.dynamicRelocs->add(IMAGE_DVRT_ARM64X_FIXUP_TYPE_ZEROFILL, |
There was a problem hiding this comment.
I'm trying to form an understanding of the expected end state of things after this loop.
At the end, we're going to have the regular IAT (lookups here) look like this:
[native]...[native][hybrid]...[hybrid][null chunk][second EC-only]...[more EC-only][lookupTerminator]
I presume the purpose of the null chunk in the middle of the regular IAT is to terminate it, so in native mode you only see the native-only and hybrid entries? After applying dynamic relocations, the null chunk is turned into the entry it was initially meant to be, so you see all of it - and we adjust the pointer to the start, so that it points at the first non-native entry.
What's the idea and purpose of the lookupTerminator? In the native view, it's not visible, and after relocations in EC mode, it's turned into a null terminator. Why not just do a plain null terminator to begin with?
Secondly, if I follow it correctly, the aux IAT is supposed to look like this:
[null chunks for native-only][aux entries for hybrid imports][aux entries for ec-only imports][null chunk]
I've forgotten the purpose/role of the AUX IAT. Doesn't it use null pointers as terminators - how does this work? Or does the delta for the start of the regular IAT also get applied to this one?
There was a problem hiding this comment.
I'm trying to form an understanding of the expected end state of things after this loop.
At the end, we're going to have the regular IAT (
lookupshere) look like this:[native]...[native][hybrid]...[hybrid][null chunk][second EC-only]...[more EC-only][lookupTerminator]I presume the purpose of the null chunk in the middle of the regular IAT is to terminate it, so in native mode you only see the native-only and hybrid entries? After applying dynamic relocations, the null chunk is turned into the entry it was initially meant to be, so you see all of it - and we adjust the pointer to the start, so that it points at the first non-native entry.
Yes, it's all correct.
What's the idea and purpose of the lookupTerminator? In the native view, it's not visible, and after relocations in EC mode, it's turned into a null terminator. Why not just do a plain null terminator to begin with?
Secondly, if I follow it correctly, the aux IAT is supposed to look like this:
[null chunks for native-only][aux entries for hybrid imports][aux entries for ec-only imports][null chunk]
I think you're correct, using a null chunk should work here. However, that's not what MSVC does. I'm not sure why; I can only speculate. My best guess is that a null chunk might confuse some applications that manually read the IAT (such as anti-cheat systems or DRM software). Following MSVC's approach ensures that the number of null chunks in .idata$4 and .idata$5 matches the number of different imported DLLs, and that all hint chunks are referenced from the IAT in both the native and EC views, even if they are referenced from an entry that is inactive in the native view.
Since this is just speculation, I'm open to changing it to null chunks if you think strictly following MSVC isn't necessary.
I've forgotten the purpose/role of the AUX IAT.
On ARM64EC, the regular IAT contains pointers to imported functions as exposed by PE exports. These may be x86 export thunks that perform FFS jumps to EC code, or they may simply be x86 functions. The purpose of the auxiliary IAT is to provide __imp symbols in a form that can be directly called from EC code (possibly by pointing to a thunk that performs a runtime check on the regular IAT pointer).
Doesn't it use null pointers as terminators - how does this work? Or does the delta for the start of the regular IAT also get applied to this one?
The delta from the start of the IAT needs to match. The auxiliary IAT is referenced in CHPE metadata as an RVA to the entire table. The loader may need to associate its entries with those in the regular IAT, potentially using the same offset from the table's start as in the regular IAT. To ensure this works, we need to populate the auxiliary IAT with native-only imports, even though they are not used at runtime.
There was a problem hiding this comment.
I think you're correct, using a null chunk should work here. However, that's not what MSVC does. I'm not sure why; I can only speculate. My best guess is that a null chunk might confuse some applications that manually read the IAT (such as anti-cheat systems or DRM software). Following MSVC's approach ensures that the number of null chunks in .idata$4 and .idata$5 matches the number of different imported DLLs, and that all hint chunks are referenced from the IAT in both the native and EC views, even if they are referenced from an entry that is inactive in the native view.
Since this is just speculation, I'm open to changing it to null chunks if you think strictly following MSVC isn't necessary.
Ok, that's probably a good enough reason. But it's quite confusing when trying to make out how it works from reading the code (ok, figuring it out from MS link.exe probably isn't any more straightforward...), so it would be good to explain this with some comments.
lld/test/COFF/arm64x-import.test
Outdated
| TEST-234-123A: 0x180008000 08200000 10200000 00200000 | ||
|
|
||
|
|
||
| # Link to the imported func3 and func4 from both native code, and func1 and func2 from EC code. |
There was a problem hiding this comment.
The word both here feels a bit off?
cjacek
force-pushed
the
arm64x-import
branch
2 times, most recently
from
3e16b71 to
46617ff
Compare
mstorsjo
approved these changes
Jan 25, 2025
There was a problem hiding this comment.
LGTM now, thanks! Just one minor comment wording nit.
With the additional comments I think this is clearer now (although as I did manage to understand it now, the previous version of it also felt more understandable now - it's hard to recreate the previous "trying to understand it from scratch" state).
lld/COFF/DLL.cpp
Outdated
| } | ||
|
|
||
| if (s->file->impECSym) { | ||
| // Detect the first EC-only import in the hybrid IAT. Emit null chunks |
There was a problem hiding this comment.
Nit: As it is only one chunk that is inserted, would it be clearer to just mention "a null chunk" here? (The next row talks about "as a terminator" - if we keep the plural form here we probably should say "as terminators" below as well?)
There was a problem hiding this comment.
Fixed and merged. Thanks!
In hybrid images, the PE header references a single IAT for both native and EC views, merging entries where possible. When merging isn't feasible, different imports are grouped together, and ARM64X relocations are emitted as needed.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/13515 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/9778 Here is the relevant piece of the build log for the reference |
cjacek
added a commit
that referenced
this pull request
Jan 26, 2025
This ensures the disassembler can handle ARM64X binaries correctly. Fixes #124189.
mstorsjo
reviewed
Jan 26, 2025
| @@ -0,0 +1,533 @@ | |||
| REQUIRES: aarch64 | |||
There was a problem hiding this comment.
This file seems to require x86 too, as we're disassembling some x86 thunks(?), see https://lab.llvm.org/buildbot/#/builders/190/builds/13515/steps/6/logs/FAIL__lld__arm64x-import_test.
There was a problem hiding this comment.
Yes, sorry about that. I pushed a fix as 80ab237.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/193/builds/5247 Here is the relevant piece of the build log for the reference |
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.
In hybrid images, the PE header references a single IAT for both native and EC views, merging entries where possible. When merging isn't feasible, different imports are grouped together, and ARM64X relocations are emitted as needed.