-
Notifications
You must be signed in to change notification settings - Fork 11
Use darwin / clang syntax, with #define for platform #76
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
Use darwin / clang syntax, with #define for platform #76
Conversation
Araneidae
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, this is a major step forward. I hope that your patch can be simplified to a single unconditional adr instruction; if you can test this I think we'll merge this and try and update our CI runners if necessary.
| #if defined(__aarch64__) && defined(__APPLE__) // darwin syntax | ||
| " adrp lr, action_entry@PAGE\n" | ||
| " add lr, lr, action_entry@PAGEOFF\n" | ||
| #else | ||
| " ldr lr, =action_entry\n" | ||
| #endif |
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.
I don't think there's anything specific to Apple about this two part syntax. If possible I think I'd like to see this unconditionally using the new syntax.
It does occur to me that we shouldn't need to be using a linker relocation here at all: action_entry is nearby at a constant offset, so I'd expect something like
adr lr, action_entryto work on both targets (it looks like ADR is the appropriate instruction here).
@AlexanderWells-diamond, do we have CI runners for Linux and OSX ARM64 for cothread?
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.
We currently do not have Linux ARM64, nor any MacOS CI as part of our default set.
In theory, as per this blog post, we can simply add ubuntu-24.04-arm and macos-latest to the test matrix to get full coverage of Arm64 platforms. However, a quick test shows failures on both platforms.
I'll do a little digging into why these are failing.
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.
Indeed, the corresponding instruction in switch-arm.c also ought to be ADR, but I guess that's out of scope for this PR.
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.
There's currently no drop-in way to make either Linux or MacOS Arm64 work.
Both have the issue that they will need changes to the epics-base container to support ARM.
MacOS additionally needs a complete overhaul of how the CI works; we currently use the epics-base container to provide us an EPICS environment for various tests. We cannot use this on MacOS, the Github runners do not let us run containers inside them. So we need to either stop doing all the EPICS-specific tests there, or set up the environment ourselves in the CI scripts.
This is all arising from issue #68 and PR #76. It seems that MacOS Arm64 does not like the use of relocation in the original switch-arm64.c code. This arises from a single instruction ldr lr, =action_entry As action_entry is extremely close to this code this can simply be replaced with adr lr, action_entry for both ARM32 and ARM64.
|
This is now obsoleted by PR #77 which uses the |
This is all arising from issue #68 and PR #76. It seems that MacOS Arm64 does not like the use of relocation in the original switch-arm64.c code. This arises from a single instruction ldr lr, =action_entry As action_entry is extremely close to this code this can simply be replaced with adr lr, action_entry for both ARM32 and ARM64.
|
Oh well. It looks like PR #77 can't be made to work on OSX. |
Fix build on macOS, resolves
ld: Found illegal text-relocations