Fix Wasm R2R exceptions escaping filters - #133932
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 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 'arch-wasm': @lewing, @pavelsavara |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The exception-handling condition must be restricted to filter funclets to preserve collided-unwind fail-fast behavior.
Get a fresh assessment by requesting another Copilot review.
Review tier: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes WASM ReadyToRun exception-filter propagation and re-enables three quarantined tests.
Changes:
- Adds WASM R2R filter-boundary handling.
- Removes obsolete quarantines and an unused test dependency.
| File | Description |
|---|---|
src/tests/JIT/Regression/CLR-x86-JIT/V1-M12-Beta2/b68872/b68872.ilproj |
Removes an obsolete test-library reference. |
src/tests/JIT/Regression/CLR-x86-JIT/V1-M12-Beta2/b68872/b68872.il |
Removes WASM R2R quarantine metadata. |
src/tests/JIT/Regression_3/GitHub_4044/GitHub_4044.cs |
Removes the WASM R2R quarantine. |
src/tests/JIT/Methodical/eh/basics/throwinfilter.il |
Removes the WASM R2R quarantine. |
src/coreclr/vm/exceptionhandling.cpp |
Adjusts WASM R2R filter-boundary exception handling. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @agocke |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
I wonder if this offset discrepancy isn't actually what should be fixed. Why are those two offsets different? |
|
I think that is the design? cc @AndyAyersMS @davidwrighton |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Agreed, I think the failure analysis was wrong and the revert is probably wrong, digging into the details now. |
|
It looks like the filter fix uncovered a different issue in the GC-filter. Looking into that now. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@AndyAyersMS @janvorli the offset change is restored and the GC change fixes the test that regressed as a result. I am not sure if there is a better approach there too? |
|
That change seems a bit odd too, I don't think the filter funclet parent frame should ever be at a non-interruptible location (they should all be sitting at a call). Maybe there is something else inhibiting parent frame reporting? |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
You are right that conceptually the filter parent should be sitting at a call. I traced why it is still treated as non-reportable on Wasm. The recovered parent context is the original exception context and carries This is not just the usual optimization that drops a boring call site late in encoding. The Wasm emitter does not call I mutation-tested two deeper alternatives:
So the current scoped behavior is intentional: for an R2R filter, report the always-live, zero-initialized untracked slots from the filter frame rather than suppressing them in expectation that the aborted parent will report. Implementing the normal parent-reporting model would require broader Wasm GC-info work to emit precise call-site metadata. The exact A/B matrix was:
I updated the code comment to describe the missing call-site GC metadata rather than merely saying the parent is non-interruptible. Note This response was authored with GitHub Copilot. |
Ok, that makes more sense. Since Wasm only has untracked lifetimes there is no per-call site info to report. I suppose we could still report all the call sites to make the main method look like it is in fact interruptible (not sure how much this costs). We have the same (only untracked lifetime) setup for Tier0 and perhaps we've avoided this problem there by reporting call sites. Also note the filter proper doesn't have any untracked GC slots to report, they are all shared with the parent frame (which is why duplication is possible). |
|
Ah apparently for native targets we force methods with EH to be fully interruptible, and hence the parent frame does the untracked reporting and we suppress reporting in the filter. But for Wasm we don't do this forcing, fully interruptible makes no sense... so then the parent doesn't report, and we have to report in the filter. So from what I can tell your fix looks good. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@AndyAyersMS comment fixed, please reapprove when you have a chance |
## Summary - recognize the implicit WASM R2R boundary between a filter funclet and its VM caller - propagate exceptions thrown by filters back to the caller that applies filter exception semantics - preserve the existing collided-unwind fail-fast invariant for non-WASM-R2R cases - re-enable all three runtime tests quarantined under dotnet#133468 ## Testing - `./build.sh clr+libs+host` - `./build.sh clr` - `./build.sh clr -os browser -arch wasm -c Checked /p:HostConfiguration=Release` - browser WASM R2R `b68872`: exit 100 - browser WASM R2R `throwinfilter_d`: exit 100 - browser WASM R2R `GitHub_4044`: exit 100 - native osx-arm64 Debug `b68872`: exit 100 - native collided-unwind regression `GitHub_121578`: passed Resolves dotnet#133468 > [!NOTE] > This pull request description was authored with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Summary
FilterOffsetvalues and the existing collided-unwind fail-fast invariantTesting
./build.sh clr+libs+host./build.sh clr./build.sh clr -os browser -arch wasm -c Checked /p:HostConfiguration=Releaseb68872: exit 100throwinfilter_d: exit 100GitHub_4044: exit 100Runtime_88168: exit 100GitHub_22820: exit 100diagnostic-ip-stacktrace, with and without a test R2R component: exit 100b68872: exit 100GitHub_121578: passedResolves #133468
Note
This pull request description was authored with GitHub Copilot.