-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix MachO adrp/add and branch relocations with embedded addend #124083
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
These cannot have any addend embedded in the instruction and must instead be handled via a additional reloc entry. The object writer already knows how to create these reloc entries, but it was leaving the embedded addend in the intstruction in some cases.
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug in the MachO object writer where ARM64 relocations (ADRP/ADD and BRANCH26) were incorrectly leaving embedded addends in the instruction encoding. According to the MachO format specification for ARM64, these relocation types cannot have addends embedded in the instruction - instead, addends must be communicated through separate ARM64_RELOC_ADDEND relocation entries.
Changes:
- Added IMAGE_REL_BASED_ARM64_BRANCH26 to the case statement handling ARM64 relocations that require addend extraction and clearing
- Ensured that any existing addend value embedded in the BRANCH26 instruction is read out, accumulated into the
addendvariable, and then the instruction is cleared to zero
|
Can we re-enable the test disabled against the fixed issue?
|
Thanks, reenabled that. |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
|
The LLVM code at https://github.com/llvm/llvm-project/blob/3a084241248efd7d5259183cffae1d5b7057a54c/llvm/lib/Target/AArch64/MCTargetDesc/AArch64MachObjectWriter.cpp#L355-L382 does it for Branch26 as well, but in my tests the embedded addends for those appear to be fine, so I have left that out. Edit: The documentation is quite clear to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
This reverts commit 8461bc8.
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The runtime-nativeaot-outerloop osx-arm64 failure looks related, but I cannot reproduce it locally after running in a loop for a while. |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
## Description This PR fixes several failures encountered in extra-platforms pipeline: - Updates `ZipMergedPayloadDirectory` target to use `PreparePayloadDirectories` instead of `PrepareMergedTestPayloadDirectories`, which was inlined in #123110. This should fix Apple mobile runtime tests - Includes StandaloneTestRunner markers used for Native AOT mobile tests - Excludes test markers for shared libraries on Android - Disables unreliable socket tests on Apple mobile Native AOT builds will be fixed in #124083 --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
MichalStrehovsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
|
/ba-g Timeout and unmatched dotnet/dnceng#6457 |
1 similar comment
|
/ba-g Timeout and unmatched dotnet/dnceng#6457 |
These cannot have any addend embedded in the instruction and must instead be handled via a additional reloc entry. The object writer already knows how to create these reloc entries, but it was leaving the embedded addend in the instruction in some cases.
Fix #124015