Fix up hijacking on arm64 (preserve async continuation register) #122942
Conversation
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
There was a problem hiding this comment.
Pull request overview
This PR fixes thread hijacking on ARM64 by preserving the x2 register, which may contain async continuation values. The key change is moving the thread pointer from x2 to x9 to avoid conflicts, and updating the probe frame to save and restore x2 along with x0 and x1.
Key Changes:
- Changed thread register from x2 to x9 in hijacking code to preserve x2 for async continuations
- Expanded probe frame to save/restore x2 register (frame size increased from 0xD0 to 0xD8 bytes)
- Added GETTHREAD_ETLS_9 macro and PTFF_SAVE_X2 flag for proper thread pointer retrieval and register saving
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/nativeaot/Runtime/unix/unixasmmacrosarm64.inc | Adds GETTHREAD_ETLS_9 macro to retrieve thread pointer in x9 while preserving x0-x3, and defines PTFF_SAVE_X2 flag |
| src/coreclr/nativeaot/Runtime/arm64/GcProbe.asm | Updates probe frame layout to include x2, changes thread register from x2 to x9, adjusts all offsets and register references, and fixes orr->mov instruction |
| src/coreclr/nativeaot/Runtime/arm64/GcProbe.S | Parallel changes to GcProbe.asm for Unix assembly syntax |
|
@VSadov @jakobbotsch is there some doc on the async calling convention? I don't know how to review this change because I don't know what the appropriate callee/caller save/kill registers are. |
The calling convention is documented in the BOTR. The relevant section about returns is: For the purpose of this PR the relevant part is that the special handling of r0 and r1 now needs to be generalized to r2 as well. What makes it a bit less trivial is that the hijacking itself assumes that r2 is dead/undefined at the point of return and we may be using r2 as a temp. That should switch to use some other scratch register. |
| ;; | ||
| NESTED_ENTRY RhpGcStressProbe | ||
| PUSH_PROBE_FRAME x2, x3, x12 | ||
| PUSH_PROBE_FRAME x4, x3, x12 |
There was a problem hiding this comment.
RhpGcStressProbe is dead code after we switched suspension machinery to be like on CoreCLR a few releases ago.
The preexisting GcStress code in NAOT assumes cooperative suspension with loop polling/hijacking and therefore incompatible with what we have now. It was not high in priorities to remove it, hoping maybe at least some portion could be useful (doubtful).
Perhaps should just remove it (in a separate change of course), so noone needs to worry about dead code in changes like this.
|
Since there's no need to preserve x3, I removed |
Flag x2 (see async calling convention) during GC as it might contain an async continuation.
Contributes to #122492.