Fix JIT filter successors for finally blocks in sibling catch handlers - #134505
jakobbotsch with Copilot wants to merge 4 commits into
Conversation
|
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. |
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com>
phiFound || ... in SSA for filter with sibling catch
| [Theory] | ||
| [InlineData(5, false, 6)] | ||
| [InlineData(0, false, 1)] | ||
| [InlineData(5, true, 0)] | ||
| public static void TestEntryPoint(int arg, bool argumentException, int expected) | ||
| { | ||
| s_sink = -1; | ||
| Assert.Equal(expected, Test(arg, argumentException)); | ||
| Assert.Equal(expected, s_sink); | ||
|
|
||
| s_sink = -1; | ||
| Assert.Equal(expected, TestWithEnclosedFinally(arg, argumentException)); | ||
| Assert.Equal(expected, s_sink); | ||
| } |
There was a problem hiding this comment.
@copilot We do not need multiple InlineData here, a single Fact with a single testing value suffices.
There was a problem hiding this comment.
Updated in bef02dc to a single [Fact] using input 5 and expected result 6. Both EH shapes remain covered. The focused test passed on the Checked JIT.
There was a problem hiding this comment.
@copilot Either remove the argumentException parameter and simplify the test further, or call it from TestEntryPoint with true now.
There was a problem hiding this comment.
Added argumentException: true calls for both helpers in TestEntryPoint, with return-value and sink assertions, in 188c7c5. The single Fact passes on the Checked JIT.
Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com>
Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com>
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
EH control-flow changes require final human review.
Review effort: Lite
Findings: None
What changed in this PR
Fixes JIT EH successor modeling for filters and sibling catch handlers, with regression coverage.
Changes:
- Restricts traversal to enclosing
trylinks. - Adds sibling-catch and nested-finally regression tests.
| File | Description |
|---|---|
src/tests/JIT/Regression_ro_2/Runtime_134457.cs |
Adds regression tests for issue #134457. |
src/coreclr/jit/compiler.hpp |
Corrects EH successor traversal. |
The JIT incorrectly models an EH edge from a filter to a finally nested inside a mutually-protecting sibling catch. This disagrees with predecessor enumeration and triggers an SSA phi-insertion assertion.
BlockPredsWithEH. Continue past non-enclosed descriptors so earlier valid finally/fault successors remain discoverable.phiFound || ...in SSA for a filter whose mutually-protecting sibling catch contains a try/finally #134457