[RuntimeDyld][ELF][AArch64] Fix resolveAArch64ShortBranch. #92245
Conversation
This file contains 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
We don't know the load addresses when this function is called, so it shouldn't be trying to use them to determine whether or not the branch is short. Notably, this will fail in the case where the code is being loaded into a target in such a way that the section offsets differ between the process generating the code and the target process. rdar://127673408
This was referenced May 15, 2024
adrian-prantl
approved these changes
May 15, 2024
I guess in principle we might be able to add something to the ExecutionEngine tests. I should really have thought of that, but I've been rather focused on fixing the problem quickly to un-break the Swift build :-) |
We mustn't use direct branches for cross-section branches, because doing so will result in problems if the sections aren't loaded at the same offsets in the target process as in the host process. rdar://127673408
|
I've added a test that doing a cross-section branch on AArch64 doesn't result in a direct reference but instead goes via a stub. |
|
@adrian-prantl This all passed, but I don't have merge rights in the LLVM repo (I'm assuming you do :-)). |
|
@adrian-prantl would you merge this PR? |
|
Well the good news is we have some unexpected passes now! https://lab.llvm.org/buildbot/#/builders/96/builds/58232 I'll update the xfails. |
This was referenced May 29, 2024
Thank-you. I think you're right that this will fix FreeBSD also; the problem here was architecture and object format but not OS specific, so it would have been broken for any AArch64 ELF platform and should now be fixed for the same. |
vg0204
pushed a commit
to vg0204/llvm-project
that referenced
this pull request
May 29, 2024
We don't know the load addresses when this function is called, so it shouldn't be trying to use them to determine whether or not the branch is short. Notably, this will fail in the case where the code is being loaded into a target in such a way that the section offsets differ between the process generating the code and the target process. rdar://127673408
vg0204
pushed a commit
to vg0204/llvm-project
that referenced
this pull request
May 29, 2024
PR llvm#92245 fixed these tests on Linux. They likely work on FreeBSD too but leaving the xfail for that so it can be confirmed later. Also updated a bugzilla link to one that redirects to Github issues. Relates to issues llvm#43398 and llvm#48751.
|
The added LLVM test is failing for me when performing 32-bit x86 build. Presumably it fails on all 32-bit systems: |
|
@mgorny Could you get a symbolicated backtrace for the test failure? Or run the offending test under gdb/lldb and get a backtrace that way? (I don't have a 32-bit machine handy to test this; I could set something up, but it'll take a while, and if you've got a system set up already and we can work out why it's failing for you, that would be easier.) |
|
Sure, I'll try to get something later today. That said, I don't really have a 32-bit system — just a regular amd64 multilib. Hopefully ccache's still warm. |
|
|
Ping. Do you need anything else from me? This is blocking testing for us. |
It looks like the LLVM you were using didn't have line numbers, which is what I was hoping we'd get here (addresses aren't meaningful because of ASLR, and I think the assertion in question has been inlined from somewhere into Mostly what I need is to find the time to look at it. I suspect once we know what the assertion failure was, it'll be obvious what the problem is. |
|
Ah, sorry, I guess I forgot to add |
These numbers are as of 1af0778. |
|
That's extremely suspicious. |
|
I don't see how that could happen. There are two calls to will return Any chance you could set a breakpoint on |
|
Oh, hang on a minute… I'm looking at an older copy of the LLVM repository. Let me update and take another look. |
|
No, updating didn't change anything that would affect this. |
|
OK, I've reproduce the sign extension problem. Just trying to work out why it's happening. (It's an existing bug, nothing to do with the code I added…) |
|
Thanks. I've tried to step to get the value you've asked for but apparently it got optimized out. Do you need me to try with |
|
No, it's OK, I know what the problem is now. It's a bug, and it's all over the file. It'll affect building code for 64-bit targets from 32-bit hosts in various combinations :-( (The fundamental issue is that addresses are often regarded as signed… so before extending to 64-bit, you need to cast them to an unsigned type first. Otherwise you get sign extension, but only if the address is 32-bit to start with.) |
|
I've filed #94478 with an explanation of the problem. Since my test exposes it, I guess I'll raise a PR to fix it too. |
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.
We don't know the load addresses when this function is called, so it shouldn't be trying to use them to determine whether or not the branch is short. Notably, this will fail in the case where the code is being loaded into a target in such a way that the section offsets differ between the process generating the code and the target process.
rdar://127673408