[LLD][COFF] Emit base relocation for native CHPE metadata pointer on ARM64X #121500
Conversation
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-lld Author: Jacek Caban (cjacek) ChangesFull diff: https://github.com/llvm/llvm-project/pull/121500.diff 2 Files Affected:
diff --git a/lld/COFF/Writer.cpp b/lld/COFF/Writer.cpp
index b3dd5f6cf49265..a0ab80f0bf2d16 100644
--- a/lld/COFF/Writer.cpp
+++ b/lld/COFF/Writer.cpp
@@ -2594,6 +2594,38 @@ void Writer::createDynamicRelocs() {
LOAD_CONFIG_TABLE * sizeof(data_directory) +
offsetof(data_directory, Size),
0);
+
+ // Insert a 64-bit relocation for CHPEMetadataPointer. Its value will be set
+ // later in prepareLoadConfig to match the value in the EC load config.
+ // However, a base relocation must be allocated in advance, so we handle it
+ // here.
+ if (ctx.symtab.loadConfigSym && ctx.hybridSymtab->loadConfigSym &&
+ ctx.symtab.loadConfigSize >=
+ offsetof(coff_load_configuration64, CHPEMetadataPointer) +
+ sizeof(coff_load_configuration64::CHPEMetadataPointer)) {
+ DefinedRegular *sym = ctx.symtab.loadConfigSym;
+ SectionChunk *chunk = sym->getChunk();
+ ArrayRef<coff_relocation> curRelocs = chunk->getRelocs();
+ MutableArrayRef<coff_relocation> newRelocs(
+ bAlloc().Allocate<coff_relocation>(curRelocs.size() + 1),
+ curRelocs.size() + 1);
+ size_t chpeOffset = sym->getValue() + offsetof(coff_load_configuration64,
+ CHPEMetadataPointer);
+ size_t i;
+ for (i = 0;
+ i < curRelocs.size() && curRelocs[i].VirtualAddress < chpeOffset; ++i)
+ newRelocs[i] = curRelocs[i];
+ newRelocs[i].VirtualAddress = chpeOffset;
+ // The specific symbol used here is irrelevant as long as it's valid, since
+ // it will be overridden by prepareLoadConfig. Use the load config symbol
+ // itself.
+ newRelocs[i].SymbolTableIndex =
+ chunk->file->getCOFFObj()->getSymbolIndex(sym->getCOFFSymbol());
+ newRelocs[i].Type = IMAGE_REL_ARM64_ADDR64;
+ for (; i < curRelocs.size(); ++i)
+ newRelocs[i + 1] = curRelocs[i];
+ chunk->setRelocs(newRelocs);
+ }
}
PartialSection *Writer::createPartialSection(StringRef name,
diff --git a/lld/test/COFF/arm64x-loadconfig.s b/lld/test/COFF/arm64x-loadconfig.s
index d21f4bfe95b843..3b53d32a9b5492 100644
--- a/lld/test/COFF/arm64x-loadconfig.s
+++ b/lld/test/COFF/arm64x-loadconfig.s
@@ -118,10 +118,14 @@
// BASERELOC: BaseReloc [
// BASERELOC-NEXT: Entry {
// BASERELOC-NEXT: Type: DIR64
+// BASERELOC-NEXT: Address: 0x10C8
+// BASERELOC-NEXT: }
+// BASERELOC-NEXT: Entry {
+// BASERELOC-NEXT: Type: DIR64
// BASERELOC-NEXT: Address: 0x1208
// BASERELOC-NEXT: }
// BASERELOC-NEXT: Entry {
-// BASERELOC: Type: DIR64
+// BASERELOC-NEXT: Type: DIR64
// BASERELOC-NEXT: Address: 0x2074
// BASERELOC-NEXT: }
|
lld/COFF/Writer.cpp
Outdated
| // The specific symbol used here is irrelevant as long as it's valid, since | ||
| // it will be overridden by prepareLoadConfig. Use the load config symbol | ||
| // itself. | ||
| newRelocs[i].SymbolTableIndex = |
There was a problem hiding this comment.
The whole logic of allocating a whole new array of relocations, inserting a new element at the right location within it, and replacing the relocations array with the new one, feels a bit complex, when it is interleaved with the actual logic that we're trying to add here. (Also, allocating a whole new array every time we want to add one dynamic relocation feels a bit clumsy, but I'm not sure if that's an issue in practice.)
Is it possible to abstract away the mechanism of adding new dynamic relocations into an entry, so that we can separate the logic for the contents of the new relocation, and the mechanics of allocate+insert+replace?
There was a problem hiding this comment.
The approach was kept localized since this is the only known case requiring the addition of extra base relocation to a section chunk, and the associated reallocation overhead seemed acceptable to me. While a similar relocation rewrite mechanism exists for range extension thunks and I used it as a reference, the differences were substantial enough that there was not much to share.
I revised the MR with a simpler solution and plan to extract the first commit into a standalone PR, as it appears independently valid. This change represents more of a special case than an abstraction. Section chunks were left unmodified due to their sensitivity to memory usage, and addressing the issue at the OutputSection level proved impractical because those assignments occur too late in the process.
For context, hybrid ARM64X uses two load configs but a single CHPE metadata structure. The metadata resides in the EC namespace, where the CRT initializes the EC load config’s CHPE metadata pointer as a relocatable value, while the native load config's pointer remains unset. The linker has three tasks related to CHPE metadata:
- Copying the pointer value from the EC load config to the native one (as implemented by [LLD][COFF] Use EC symbol table for CHPE metadata #120328 in lld-link).
- Emitting a base relocation for the updated native load config (which MSVC linker always does, even when the EC load config lacks a similar relocation).
- Emitting an ARM64X relocation for the hybrid entry point in the CHPE metadata. Here, the MSVC linker references the
__chpe_metadatasymbol.
The logic for tasks 1 and 3 differs, although they could theoretically rely on the same approach to identify the CHPE metadata. However, aligning with MSVC's behavior was straightforward and ensures compatibility.
Additionally, entire load configurations in the PE header are modified using ARM64X relocations, as addressed by #121337.
Thanks for the review.
There was a problem hiding this comment.
I updated my WIP branch to include a draft of 3: cjacek@d7a07e8
lld/COFF/Writer.cpp
Outdated
| newRelocs[i] = curRelocs[i]; | ||
| newRelocs[i].VirtualAddress = chpeOffset; | ||
| // The specific symbol used here is irrelevant as long as it's valid, since | ||
| // it will be overridden by prepareLoadConfig. Use the load config symbol |
There was a problem hiding this comment.
I'm not sure I follow the comment here - I don't see any code for updating any symbol references in the dynamic relocations within prepareLoadConfig?
There was a problem hiding this comment.
This wasn't about dynamic relocations; it updates the pointer within the load configuration itself, which was the part affected by this relocation. That said, this aspect no longer exists in the updated version.
Now I also managed to dig up and reread the original version of the commit - now I understand what that version really did. The previous version was quite confusing, as it modified regular coff object relocations, in order to allocate a new base relocation, done in the function for creating dynamic relocations. So the problem that is being fixed here, is that the original load config struct in the object files has a plain zero in these files (no relocation) - as we copy over a value synthetically in the linker (rather than by resolving a regular object file relocation), we also need to make sure that a corresponding base relocation is generated here. (I'm sure you've explained this already once or twice here - but unfortunately it sometimes takes a bit of time to actually understand it.) |
No description provided.