JIT: Fix null-check ordering in EH regions - #133827
AndyAyersMS wants to merge 2 commits into
Conversation
Ensure this pointer null checks happen before inlinee side effects observable by catch handlers and exception filters. Prevent early propagation from folding these checks across such side effects, and add regression coverage for both cases. Fixes dotnet#133714 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 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 |
|
@EgorBo PTAL No diffs expected |
There was a problem hiding this comment.
🟡 Changes recommended
Nested inlining can still miss the enclosing protected-region state, leaving the null-check ordering bug unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
JIT: Fix null-check ordering in EH regions
This PR preserves receiver null-check ordering across inlinee side effects observable by exception handlers and filters.
Changes:
- Treat assignments as visible side effects in protected regions.
- Prevent unsafe null-check folding.
- Add catch and filter regression coverage.
File summaries
| File | Summary |
|---|---|
src/tests/JIT/Regression_ro_2/Runtime_133714.cs |
Adds regression coverage. |
src/coreclr/jit/importer.cpp |
Preserves null checks across inlinee assignments. |
src/coreclr/jit/earlyprop.cpp |
Restricts null-check folding in EH regions. |
src/coreclr/jit/compiler.h |
Updates the helper parameter name. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
|
There were a couple of minor diffs, looks like just one method. |
Use HasPotentialEHSuccs to identify try and filter regions when preserving null-check ordering. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3fb0bcff-10ee-4de2-a4dd-442ad89b2b11
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
|
The new test is failing on mono and coreclr Wasm. Going to exclude mono and see if #133932 fixes things for Wasm. |
Yes, that fixes the Wasm failure. So will wait on that PR. |
Ensure this pointer null checks happen before inlinee side effects observable
by catch handlers and exception filters. Prevent early propagation from folding
these checks across such side effects, and add regression coverage for both
cases.
Fixes #133714
Note
This pull request description was generated by GitHub Copilot.