Ensure the Thumb bit is set for resumption stub relocs#125421
Ensure the Thumb bit is set for resumption stub relocs#125421jtschuster wants to merge 1 commit intodotnet:mainfrom
Conversation
…t once" This reverts commit 47ee93d.
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR addresses ARM32 Thumb-2 requirements for async resumption-related code pointers so ReadyToRun/crossgen2 can safely emit async methods again on ARM32 without illegal-instruction failures.
Changes:
- Remove the ARM32-specific exclusion that prevented async methods from being emitted in ReadyToRun mode (still excluded for composite builds).
- Update JIT emission of
CORINFO_AsyncResumeInforelocations to apply an additional delta on ARM32 so relocated code pointers have bit 0 (Thumb bit) set.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs |
Stops filtering out async methods on ARM32; keeps composite-build exclusion. |
src/coreclr/jit/emit.cpp |
Records async resume info relocations with an ARM32 addend delta to ensure Thumb-bit-correct code pointers. |
| emitRecordRelocationWithAddlDelta(&aDstRW[i].Resume, emitAsyncResumeStubEntryPoint, CorInfoReloc::DIRECT, codeDelta); | ||
| if (target != nullptr) | ||
| { | ||
| emitRecordRelocation(&aDstRW[i].DiagnosticIP, target, CorInfoReloc::DIRECT); | ||
| emitRecordRelocationWithAddlDelta(&aDstRW[i].DiagnosticIP, target, CorInfoReloc::DIRECT, codeDelta); | ||
| } |
There was a problem hiding this comment.
This Thumb-bit fix is ARM32-specific and easy to regress (e.g., future relocation/emission changes around async resume info). Please add a targeted regression test that runs on ARM32 with ReadyToRun/crossgen output and validates that the resumption stub pointer is callable (or that the emitted ResumeInfo code pointers include the Thumb bit), so CI will catch a return of the illegal-instruction failure this PR is addressing.
| #ifdef TARGET_ARM | ||
| // ARM32 requires the Thumb bit (bit 0) set on code pointers. | ||
| int32_t codeDelta = 1; | ||
| #else | ||
| int32_t codeDelta = 0; | ||
| #endif |
There was a problem hiding this comment.
Does the JIT not have an existing abstraction for the thumb bit today?
There was a problem hiding this comment.
I couldn't find one. Copilot only found these places where we handle thumb bits:
| File | Line | Operation |
|---|---|---|
emit.cpp |
8394 | target = (BYTE*)((size_t)target | 1); — sets thumb bit |
emitarm.cpp |
5337 | distVal = (ssize_t)emitOffsetToPtr(dstOffs) + 1; — sets thumb bit |
emitarm.cpp |
6483 | addr = (BYTE*)((size_t)addr & ~1); — clears thumb bit |
| if (target != nullptr) | ||
| { | ||
| emitRecordRelocation(&aDstRW[i].DiagnosticIP, target, CorInfoReloc::DIRECT); | ||
| emitRecordRelocationWithAddlDelta(&aDstRW[i].DiagnosticIP, target, CorInfoReloc::DIRECT, codeDelta); |
There was a problem hiding this comment.
For absolute basic blocks addresses above we do not use emitRecordRelocationWithAddlDelta. Instead it just changes the address. Should this do the same?
ARM32 requires bit 0 to be set for Thumb-2 code. Without this bit set, we encountered illegal instruction failures in CI and disabled crossgenning async methods on ARM32.