-
Notifications
You must be signed in to change notification settings - Fork 99
Preserve callee-saved registers in threadentry_trampoline #347
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
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 ABI compliance issues in threadentry_trampoline functions across multiple architectures (amd64, arm64, loong64). The trampoline functions are called from C code (glibc) when creating threads, and must preserve callee-saved registers according to the Linux C ABI to prevent unexpected behavior when returning to glibc after thread exit.
Key Changes:
- Implements proper preservation and restoration of callee-saved registers in
threadentry_trampolinefor all architectures - Adjusts stack frame management to accommodate saved registers
- Ensures X15 register is zeroed on amd64 as required by Go's ABIInternal
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/fakecgo/trampolines_amd64.s | Uses existing PUSH/POP macros to save/restore callee-saved registers; zeros X15 for ABIInternal compliance |
| internal/fakecgo/trampolines_arm64.s | Manually saves/restores callee-saved registers R19-R28, F8-F15, and frame pointer/link register with adjusted stack frame |
| internal/fakecgo/trampolines_loong64.s | Manually saves/restores callee-saved registers R22-R31, F24-F31, and return address with adjusted stack frame |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
hajimehoshi
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.
looks good to me in general
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 3 out of 3 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
TotallyGamerJet
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.
Besides the open comment this LGTM
Clarified comment regarding calling ABIInternal and zero register.
hajimehoshi
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.
LGTM, thanks!
|
Note that this didn't fix the NetBSD segv issue yet unfortunately. |
threadentry_trampolineis called from glibc (that is, C code) when creating a new thread. When the thread exists,threadentry_trampolinereturn back to glibc, which can perform some additional cleanups.We currently aren't honoring the Linux C ABI. It specify some registers as calle-saved which are clobbered by the Go code. This can cause unexpected behaviors when returning to glibc when the thread exists, as the C compiler assumes some registers will have the same value as before the
threadentry_trampolinefunction was called, but they don't.This is a precondition to merge #344.
Also, might fix #304.