a64 JIT: Fix setRWX returning nothing on Apple
#1956
Conversation
WalkthroughUpdates Apple-specific memory permission handling in the AArch64 dynarec. Emitter now explicitly returns false on Apple in setRWX(). Recompiler switches Apple path to emit with RW (gen.setRW()) and later transition to RX (gen.setRX()) before dispatch; non-Apple paths keep using RWX. Changes
Sequence Diagram(s)sequenceDiagram
participant DynaRecCPU as DynaRecCPU::Init
participant Emitter as Emitter/gen
participant OS as OS (VirtualProtect/mprotect)
alt Non-Apple
DynaRecCPU->>Emitter: setRWX()
Emitter->>OS: VirtualProtect/mprotect RWX
OS-->>Emitter: result
Emitter-->>DynaRecCPU: success/fail
DynaRecCPU->>Dispatcher: start
else Apple
DynaRecCPU->>Emitter: setRW()
Emitter->>OS: mprotect/VM ops RW
OS-->>Emitter: result
Emitter-->>DynaRecCPU: success/fail
DynaRecCPU->>Emitter: setRX() before dispatch
Emitter->>OS: mprotect/VM ops RX
OS-->>Emitter: result
Emitter-->>DynaRecCPU: success/fail
DynaRecCPU->>Dispatcher: start
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/DynaRec_aa64/emitter.h (1)
74-76: Explicit Apple fallback return fixes undefined behavior; consider deprecating on Apple to prevent accidental useReturning false on Apple prevents undefined-return UB and matches the intended “do not use RWX on Apple” policy. Optional: mark setRWX() as deprecated on Apple to catch accidental use at compile-time.
You can annotate the function on Apple platforms:
// Just above the setRWX() definition #if defined(__APPLE__) __attribute__((deprecated("setRWX is unsupported on Apple; use setRW()/setRX() instead"))) #endif bool setRWX() { ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/DynaRec_aa64/emitter.h(1 hunks)src/core/DynaRec_aa64/recompiler.cc(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: pcsx-redux Windows CLI build
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: pcsx-redux (x86_64-linux)
- GitHub Check: pcsx-redux (aarch64-linux)
- GitHub Check: toolchain
- GitHub Check: macos-build-and-test-toolchain
- GitHub Check: asan
- GitHub Check: cross-arm64
- GitHub Check: build
- GitHub Check: build-openbios
- GitHub Check: aur-build
- GitHub Check: coverage
🔇 Additional comments (2)
src/core/DynaRec_aa64/recompiler.cc (2)
252-255: Good: Toggle RW before emitting blocks on AppleSwitching to RW before emission aligns with W^X requirements on Apple and avoids RWX. Looks correct.
324-325: Good: Toggle back to RX after codegen on AppleFlipping permissions back to RX post-emission is correct and pairs with the earlier RW. Together with __builtin___clear_cache, this ensures safe execution.
| #else | ||
| gen.setRW(); // M1 wants buffer marked as readable/writable with mprotect before emitting code | ||
| #endif |
There was a problem hiding this comment.
Fail fast on Apple if code buffer allocation failed (avoid writing to a null buffer)
On Apple, the allocation check (gen.getCode() == nullptr) happens later (after emitDispatcher). If mmap failed, we may attempt to emit into a null buffer before we detect it, causing a crash. Add the null check right here before any emission.
Apply this diff to add an early guard:
#else
- gen.setRW(); // M1 wants buffer marked as readable/writable with mprotect before emitting code
+ gen.setRW(); // M1 wants buffer marked as readable/writable with mprotect before emitting code
+ // Fail fast if buffer allocation failed before emitting any code.
+ if (gen.getCode<void*>() == nullptr) {
+ PCSX::g_system->message("[Dynarec] Failed to allocate memory for Dynarec.\nTry disabling the Dynarec CPU.");
+ return false;
+ }
#endifOptionally remove the later duplicate check after emitDispatcher() to avoid redundancy.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #else | |
| gen.setRW(); // M1 wants buffer marked as readable/writable with mprotect before emitting code | |
| #endif | |
| #else | |
| gen.setRW(); // M1 wants buffer marked as readable/writable with mprotect before emitting code | |
| // Fail fast if buffer allocation failed before emitting any code. | |
| if (gen.getCode<void*>() == nullptr) { | |
| PCSX::g_system->message("[Dynarec] Failed to allocate memory for Dynarec.\nTry disabling the Dynarec CPU."); | |
| return false; | |
| } | |
| #endif |
🤖 Prompt for AI Agents
In src/core/DynaRec_aa64/recompiler.cc around lines 70-72, the Apple-specific
branch calls gen.setRW() then proceeds to emit code without checking that the
code buffer was actually allocated; add an immediate check after gen.setRW()
that if gen.getCode() == nullptr you fail fast (log an error and
return/abort/propagate failure) before any emission to avoid writing to a null
buffer; optionally remove the later duplicate gen.getCode()==nullptr check after
emitDispatcher() to avoid redundancy.
|
Update on Windows-on-ARM support: Redux doesn't currently build with either the MSVC ARM64 target (Because some dependencies don't have ARM64 DLLs on Nuget) nor with the ARM64EC target (Because both the core and third party code get confused about whether they're compiling for x64 or arm64...), so fixing the JIT there is unlikely to happen any time soon. Thankfully ARM Linux/MacOS work fine, and x64 builds also work fine on WoA. |
2 participants
This shouldn't change anything because the function was not being called at all on MacOS, but it's best to avoid any future happy accidents in case that changes.
On a relevant note, I need to fix Windows support for the a64 JIT, but I haven't gotten my WoA dev setup running yet.