Repository navigation
Conversation
|
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 this area: @JulieLeeMSFT, @dotnet/jit-contrib |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused implementation addresses the root cause and includes comprehensive regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes ILVerify’s stack initialization for filters nested within exception handlers.
Changes:
- Prioritizes a filter’s own region when initializing its entry stack.
- Diagnoses conflicting filter/handler entry offsets.
- Adds five regression tests covering valid and invalid nesting.
| File | Description |
|---|---|
ILImporter.Verify.cs |
Corrects region selection and validates conflicting entry points. |
VerifierError.cs |
Adds the new verifier error code. |
Strings.resx |
Adds the diagnostic message. |
ExceptionRegionTests.il |
Adds nested-filter regression coverage. |
|
Thanks for fixing this. In case it helps the rework, I built ILVerification and ilverify at 2e95101 and at its merge base (33baf8e, "main" below), on Linux, macOS and Windows:
Happy to post the IL as test cases. Catch-when inside finally from C# (plain, nested, lambda, async MoveNext) and VB goes from StackUnderflow to clean. On Linux, output is unchanged for all 313 NETCore.App and AspNetCore.App 11.0.0-preview.7 assemblies, and across 466 assemblies in my NuGet cache this branch drops two StackUnderflows (in FSharp.Compiler.Service and Microsoft.Build.Tasks.Core) and adds none. Note This comment was prepared with AI assistance (Anthropic Claude and OpenAI Codex), under my direction. I reviewed it before posting. |
Thanks! Could you share the IL cases? I'd like to check them against the updated EH validation and add any missing coverage. |
|
Sure. The eight methods below are in the test file's style and were run with the Release and Debug ilverify builds at 72a6735, plus IL cases (names carry the verdicts 72a6735 reports; all agree with the JIT)Two things the JIT comparison turned up, neither a problem with this PR:
I filed them as #135592 and #135593. Otherwise the head looks good in these checks: 425/425 tests passed, output was identical to d8f9151 across the 313 NETCore.App and AspNetCore.App 11.0.0-preview.7 assemblies, and the 466 NuGet assemblies differed only in stack-trace line numbers from two pre-existing crashes. For each of the eight IL cases above, ilverify and Note This comment was prepared with AI assistance (Anthropic Claude and OpenAI Codex), under my direction. I reviewed it before posting. |
| Check(_exceptionRegions[_currentBasicBlock.HandlerIndex.Value].ILRegion.Kind == ILExceptionRegionKind.Finally || | ||
| _exceptionRegions[_currentBasicBlock.HandlerIndex.Value].ILRegion.Kind == ILExceptionRegionKind.Fault, VerifierError.Endfinally); |
There was a problem hiding this comment.
| Check(_exceptionRegions[_currentBasicBlock.HandlerIndex.Value].ILRegion.Kind == ILExceptionRegionKind.Finally || | |
| _exceptionRegions[_currentBasicBlock.HandlerIndex.Value].ILRegion.Kind == ILExceptionRegionKind.Fault, VerifierError.Endfinally); | |
| Check(_exceptionRegions[_currentBasicBlock.HandlerIndex.Value].ILRegion.Kind | |
| is ILExceptionRegionKind.Finally or ILExceptionRegionKind.Fault, VerifierError.Endfinally); |


Fixes #134711.
Fix incorrect entry stack initialization for nested exception filters, which caused false verification errors.
Validate EH boundaries, overlaps, nesting, clause ordering, and filter termination, including unreachable regions. (And reject method entry inside a handler or filter.)
Add regression tests.
(Assisted by Codex)