Implement a workaround for ld-prime hitting an assert on large addends#124721
Implement a workaround for ld-prime hitting an assert on large addends#124721filipnavara wants to merge 13 commits into
Conversation
|
Needs more work. I'll investigate the unit test failures first. |
|
I don't get the System.Linq.Expressions test failure on Xcode 26.2 anymore. According the log the compiler/linker version on CI is |
|
Going back to the drawing board now. |
|
@akoeplinger Is there some easy way to switch some CI lane to use newer Xcode? They should be available in the runner images, just not as default. |
|
I assume adding xcode-select to the path mentioned in https://github.com/actions/runner-images/blob/main/images/macos/macos-15-Readme.md#xcode somewhere early in the build should suffice? |
Thanks, seems to pass the smoke test with Xcode 26.2. I'll probably open a separate PR to check what is the behavior just with the Xcode bump alone. |
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
There was a problem hiding this comment.
Pull request overview
This PR adds a workaround in the Mach-O object writer to avoid Apple ld-prime assertions when emitting IMAGE_REL_BASED_RELPTR32 as *_RELOC_SUBTRACTOR + *_RELOC_UNSIGNED with large addends, by introducing reusable per-section temporary labels to keep addends within the linker’s expected signed 20-bit range.
Changes:
- Track and emit per-section temporary labels to bound relocation addends for
IMAGE_REL_BASED_RELPTR32on ARM64 and x64.eh_frame. - Adjust Mach relocation emission to optionally reference a temporary label symbol (instead of the section base symbol) for SUBTRACTOR relocs.
- Modify
build.shto attempt switching Xcode versions on macOS.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/coreclr/tools/Common/Compiler/ObjectWriter/MachObjectWriter.cs | Adds temporary-label generation/reuse and uses label symbol indices to avoid ld-prime large-addend assertions. |
| build.sh | Adds macOS-only logic to switch Xcode via sudo xcode-select. |
|
Apple support engineers confirmed that 1 MB boundary for the temporary labels (as currently implemented in this PR) should be sufficient not to hit the linker limits. The PR needs to be rebased and the temporary Xcode version bump removed once the tests pass. |
We translate the IMAGE_REL_BASED_RELPTR32 relocation into ARM64_RELOC_SUBTRACTOR and ARM64_RELOC_UNSIGNED pair on ARM64. To emulate the behavior of PC relative relocation we bake the section-relative PC offset into the addend with negative sign. The ARM64_RELOC_SUBTRACTOR relocation then subtract the base address of the section and finally the ARM64_RELOC_UNSIGNED relocation adds the target symbol address. This works fine for addends that fit into signed 24-bit integer, but ld-prime hits an assert when the addend is larger. The workaround is to generate a temporary label at the offset of the relocated address and adjust the addend to be relative to that label. We reuse the last temporary label for following relocations in the same section until we hit another overflow. This way we minimize the number of temporary labels we need to generate while still satisfying the ld-prime requirement. Same logic applies to X86_64_RELOC_SUBTRACTOR + X86_64_RELOC_UNSIGNED pair for x64 targets.
3861310 to
610e0f1
Compare
This reverts commit 610e0f1.
- Replace (int) cast-based 20-bit overflow check with direct long comparison against signed 20-bit bounds [-524288, 524287] to avoid silent truncation for large addend values. - Add Debug.Assert before the int cast in WriteInt32LittleEndian to guard against unexpected out-of-range values. - Add Debug.Assert in EmitRelocationsX64/Arm64 to verify _temporaryLabelsBaseIndex has been initialized before use. - Improve comment on Addend repurposing to document the design choice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| // The SymbolicRelocation.Addend field is repurposed to carry the | ||
| // 1-based temporary label index (0 means no temporary label). This | ||
| // avoids introducing a side table and is consumed by | ||
| // EmitRelocationsX64/EmitRelocationsArm64 when building the | ||
| // subtractor relocation pair. | ||
| addend = temporaryLabelIndex; |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| foreach (SymbolDefinition definition in _temporaryLabels) | ||
| { | ||
| temporaryLabelNameBuilder.Clear(); | ||
| temporaryLabelNameBuilder.Append("ltemp"u8).Append((int)(symbolIndex - _temporaryLabelsBaseIndex)); |
There was a problem hiding this comment.
Utf8StringBuilder doesn't have an overload that takes unsigned integers. This should not overflow for any reasonably sized object file.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| symbolicRelocation.Addend != 0 ? | ||
| (uint)(_temporaryLabelsBaseIndex + symbolicRelocation.Addend - 1) : | ||
| (uint)sectionIndex; | ||
| Debug.Assert(baseSymbolIndex < _symbolTable.Count); |
| data, | ||
| BinaryPrimitives.ReadInt32LittleEndian(data) + | ||
| (int)(addend - offset)); | ||
| (int)addend); |
| // The SymbolicRelocation.Addend field is repurposed to carry the | ||
| // 1-based temporary label index (0 means no temporary label). This | ||
| // avoids introducing a side table and is consumed by | ||
| // EmitRelocationsX64/EmitRelocationsArm64 when building the | ||
| // subtractor relocation pair. | ||
| addend = temporaryLabelIndex; |
|
@agocke do you know who would be the best to review macho object writer changes? We have a test that was running into this: runtime/src/libraries/tests.proj Lines 408 to 413 in e9dec1b But I just had to disable the test on all platforms because the test became completely broken over the weekend or so (see the two quoted bugs above, it's two disablements for the same test for different reasons). |
|
You and @elinor-fung 😄 |
We translate the IMAGE_REL_BASED_RELPTR32 relocation into ARM64_RELOC_SUBTRACTOR and ARM64_RELOC_UNSIGNED pair on ARM64. To emulate the behavior of PC relative relocation we bake the section-relative PC offset into the addend with negative sign. The ARM64_RELOC_SUBTRACTOR relocation then subtract the base address of the section and finally the ARM64_RELOC_UNSIGNED relocation adds the target symbol address.
This works fine for addends that fit into signed 20-bit integer, but ld-prime hits an assert when the addend is larger. The workaround is to generate a temporary label at the offset of the relocated address and adjust the addend to be relative to that label. We reuse the last temporary label for following relocations in the same section until we hit another overflow. This way we minimize the number of temporary labels we need to generate while still satisfying the ld-prime requirement.
Same logic applies to X86_64_RELOC_SUBTRACTOR + X86_64_RELOC_UNSIGNED pair for x64 targets.
Fixes #119380