JIT: fix wasm conditional fallthrough - #133528
AndyAyersMS merged 3 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 581bf1d5-7317-4f55-8a57-fc67827c425e
|
Azure Pipelines: Successfully started running 6 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
🟢 Approval recommended
The change correctly reuses the existing wasm fallthrough safety logic to prevent invalid fallthroughs at Try/ExnRefWrapper boundaries, with no apparent behavioral regressions outside wasm.
Pull request overview
This PR fixes WebAssembly codegen for conditional branches by ensuring “adjacent target” is only treated as a valid fallthrough when it’s actually safe in the presence of wasm EH interval boundary emissions.
Changes:
- Update wasm
genCodeForJTrueto useBasicBlock::CanRemoveJumpToTargetinstead of a rawfalseTarget != block->Next()adjacency check. - Extend
BasicBlock::CanRemoveJumpToTargetto cover bothBBJ_ALWAYSandBBJ_COND, and incorporate the existing wasm interval end check to block unsafe fallthroughs (Try / ExnRefWrapper ends).
File summaries
| File | Description |
|---|---|
| src/coreclr/jit/codegenwasm.cpp | Use CanRemoveJumpToTarget when deciding whether to emit the unconditional branch to the false target. |
| src/coreclr/jit/block.cpp | Generalize CanRemoveJumpToTarget and reuse it from CanRemoveJumpToNext, including the wasm interval-end fallthrough prohibition. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
|
fyi @dotnet/wasm-contrib |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
…l-fallthrough-133465
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 581bf1d5-7317-4f55-8a57-fc67827c425e
There was a problem hiding this comment.
🔵 Needs a closer look
It changes CoreCLR JIT control-flow emission semantics for wasm (high correctness impact) and should be validated by a maintainer with wasm R2R test coverage signals.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
An adjacent conditional target is not always a valid wasm fallthrough: closing a
TryorExnRefWrapperinterval can inject instructions before the target. Reuse the existing wasm fallthrough check for conditional branches.Fixes #133465
Fixes #133469
Note
This PR description was generated with GitHub Copilot.