[LLD][COFF] Align import directory chunk. #80014
Conversation
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-lld-coff Author: Jacek Caban (cjacek) ChangesFull diff: https://github.com/llvm/llvm-project/pull/80014.diff 2 Files Affected:
diff --git a/lld/COFF/DLL.cpp b/lld/COFF/DLL.cpp
index 6b516d8c6d5ef..d0b74ac445499 100644
--- a/lld/COFF/DLL.cpp
+++ b/lld/COFF/DLL.cpp
@@ -110,7 +110,7 @@ class OrdinalOnlyChunk : public NonSectionChunk {
// A chunk for the import descriptor table.
class ImportDirectoryChunk : public NonSectionChunk {
public:
- explicit ImportDirectoryChunk(Chunk *n) : dllName(n) {}
+ explicit ImportDirectoryChunk(Chunk *n) : dllName(n) { setAlignment(4); }
size_t getSize() const override { return sizeof(ImportDirectoryTableEntry); }
void writeTo(uint8_t *buf) const override {
diff --git a/lld/test/COFF/pdb-publics-import.test b/lld/test/COFF/pdb-publics-import.test
index 83f087b662186..3f3ab3662f8f1 100644
--- a/lld/test/COFF/pdb-publics-import.test
+++ b/lld/test/COFF/pdb-publics-import.test
@@ -115,7 +115,7 @@ CHECK-NEXT: characteristics =
CHECK-NEXT: initialized data
CHECK-NEXT: read permissions
CHECK-NEXT: {{[0-9]+}} | S_COFFGROUP [size = 28] `.idata$2`
-CHECK-NEXT: length = 40, addr = 0002:0061
+CHECK-NEXT: length = 40, addr = 0002:0064
CHECK-NEXT: characteristics =
CHECK-NEXT: initialized data
CHECK-NEXT: read permissions
@@ -162,9 +162,9 @@ CHECK-NEXT: SC[.rdata] | mod = 3, 0002:0000, size = 28, data crc = 0, reloc c
CHECK-NEXT: IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ
CHECK-NEXT: SC[.rdata] | mod = 3, 0002:0028, size = 33, data crc = 0, reloc crc = 0
CHECK-NEXT: IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ
-CHECK-NEXT: SC[.rdata] | mod = 3, 0002:0061, size = 20, data crc = 0, reloc crc = 0
+CHECK-NEXT: SC[.rdata] | mod = 3, 0002:0064, size = 20, data crc = 0, reloc crc = 0
CHECK-NEXT: IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ
-CHECK-NEXT: SC[.rdata] | mod = 3, 0002:0081, size = 20, data crc = 0, reloc crc = 0
+CHECK-NEXT: SC[.rdata] | mod = 3, 0002:0084, size = 20, data crc = 0, reloc crc = 0
CHECK-NEXT: IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ
CHECK-NEXT: SC[.rdata] | mod = 3, 0002:0104, size = 8, data crc = 0, reloc crc = 0
CHECK-NEXT: IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ
|
There was a problem hiding this comment.
Code wise, this looks good, and it does seem to affect the output of tests, so that's good.
Can you add a few words about the situation where this matters (why hasn't this been an issue so far, have we just been lucky?)? Would a specific testcase for this detail be clearer than just details in a PDB?
|
I imagine that the loader will remap the page for the IAT if not aligned. But, if it is aligned it avoids that, and would also repair page sharing. This should be in essence a tiny optimization at the cost of <=3 bytes. |
|
I think LLD always emitted unaligned import dir and it generally works fine (I just verified that it happened on aarch64, x86 and x86_64), the loader can handle it. It's not optimal and it's not what I observe with MSVC link.exe. In my case, I was trying to find why some of ARM64X binaries load fine, while others don't (it requires my WIP branch). I narrowed it down to a subset of cases, where ARM64X base relocations need to modify import directory. Generally, aarch64 and arm64ec imports are shared. As long as they use the same set of functions from import directory, both the directory and import addresses chunk are just shared. When used set of functions differs, ARM64X dynamic relocations are used to modify import dir to point to different names and import addresses for its EC view. Aligning import directory chunk fixed the problem. I suspect that loader expects some alignment on ARM64X dynamic relocation offset (but I didn't dig deeper into the loader itself) and it wasn't the case when relocated import dir was not aligned. I will add a more specific test case. |
|
@cjacek Could you please add that comment as part of the commit description, this is super interesting information. |
The loader can usually handle an unaligned import dir chunk, but It's not optimal and it's not what MSVC link.exe does. Windows refuses to load ARM64X binaries with unaligned import directory. aarch64 and arm64ec imports are shared in such binaries as much as possible. As long as they use the same set of functions from given import directory, both the directory and import addresses chunk are just shared. When used set of functions differs, ARM64X dynamic relocations are used to modify import dir to point to different names and import addresses for its EC view. I suspect that the loader expects some alignment on ARM64X dynamic relocation offset and may not be the case when relocated import dir is not aligned.
cjacek
force-pushed
the
import-dir-align
branch
from
ed50950
to
fdeba46
Compare
|
Pushed with the additional test and the description, thanks. |
No description provided.