Skip to content

Fix nested exception handler reachability in ILLink#131236

Merged
sbomer merged 3 commits into
dotnet:mainfrom
sbomer:illink-constprop-fix
Jul 23, 2026
Merged

Fix nested exception handler reachability in ILLink#131236
sbomer merged 3 commits into
dotnet:mainfrom
sbomer:illink-constprop-fix

Conversation

@sbomer

@sbomer sbomer commented Jul 22, 2026

Copy link
Copy Markdown
Member

Fix exception-handler reachability analysis in the unreachable blocks optimizer by rescanning handlers until no newly reachable handler bodies expose additional nested protected regions.

Previously, handlers were scanned only once. A nested handler could therefore be classified as unreachable before its enclosing handler body was traversed, causing required cleanup code to be removed.

Add a regression test covering an async method with nested finally blocks.

Validation:

  • Mono.Linker UnreachableBlock tests: 22 passed
  • Full Mono.Linker test suite: 1,134 passed, 30 skipped
  • Original standalone repro behaves identically with ipconstprop enabled and disabled

Fixes #131088

Note

This content was created with assistance from AI (GitHub Copilot).

Rescan exception handlers until no newly reachable handler bodies expose additional nested protected regions. Add an async nested-finally regression test for the trimming failure.

Assisted-by: GitHub Copilot:gpt-5.6-sol

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 2cee0e07-8e06-4275-b1de-508fe931bf95
Copilot AI review requested due to automatic review settings July 22, 2026 21:53
@github-actions github-actions Bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Jul 22, 2026
@dotnet-policy-service dotnet-policy-service Bot added the linkable-framework Issues associated with delivering a linker friendly framework label Jul 22, 2026
@azure-pipelines

Copy link
Copy Markdown
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.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke, @dotnet/illink
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes ILLink’s unreachable-block analysis for exception handlers by repeatedly discovering newly reachable handler bodies until reaching a fixed point, preventing nested handlers (e.g., nested finally blocks in async state machines) from being misclassified as unreachable and incorrectly removed.

Changes:

  • Update UnreachableBlocksOptimizer reachability analysis to iteratively enqueue newly reachable exception handler (and filter) blocks until no additional handlers become reachable.
  • Add a regression test case exercising an async method with nested finally blocks under ipconstprop.
  • Add a dependency library used by the new test to reproduce the nested-handler shape.
Show a summary per file
File Description
src/tools/illink/src/linker/Linker.Steps/UnreachableBlocksOptimizer.cs Reworks exception-handler reachability to rescan handlers and traverse newly reachable handler bodies, enabling correct discovery of nested protected regions.
src/tools/illink/test/Mono.Linker.Tests.Cases/UnreachableBlock/NestedFinallyInAsyncMethod.cs New test driver enabling ipconstprop and asserting nested-cleanup executes (via a counter).
src/tools/illink/test/Mono.Linker.Tests.Cases/UnreachableBlock/Dependencies/NestedFinallyInAsyncMethod_Lib.cs New dependency library containing an async method with nested finally blocks to reproduce the scenario.

Copilot's findings

  • Files reviewed: 3/3 changed files
  • Comments generated: 0

@sbomer
sbomer requested a review from a team July 22, 2026 22:11

@jtschuster jtschuster left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source changes look good to me, just wanted some clarifications on why tests are the way they are.

Replace the async dependency-based repro with a synchronous nested-finally case that exercises the same handler reachability bug. Track the test as an expected ILTrim limitation because ILTrim does not implement ipconstprop.

Assisted-by: GitHub Copilot:gpt-5.6-sol

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 2cee0e07-8e06-4275-b1de-508fe931bf95
Copilot AI review requested due to automatic review settings July 23, 2026 02:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

Comments suppressed due to low confidence (1)

src/tools/illink/test/Mono.Linker.Tests.Cases/UnreachableBlock/NestedFinallyInFinallyHandler.cs:52

  • After removing CleanupCount, Cleanup should no longer reference it. Keeping Cleanup as a no-op still allows the test to validate that the nested finally body remains reachable (via the existing [Kept] expectation on Cleanup).
        static void Cleanup() => CleanupCount++;
  • Files reviewed: 3/3 changed files
  • Comments generated: 2

Comment thread src/coreclr/tools/ILTrim.Tests/ILTrimExpectedFailures.txt

@hydraxman hydraxman left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this. The change looks good to me. Left 2 small comments. Also the PR text still says the test is async, but its sync now.

Avoid repeating linear instruction lookups during fixed-point handler discovery. Strengthen the regression with a third nested finally and rely on linker expectations instead of an unexecuted runtime counter.

Assisted-by: GitHub Copilot:gpt-5.6-sol

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 2cee0e07-8e06-4275-b1de-508fe931bf95
Copilot AI review requested due to automatic review settings July 23, 2026 17:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 3/3 changed files
  • Comments generated: 1

@sbomer
sbomer enabled auto-merge (squash) July 23, 2026 17:40
@sbomer
sbomer requested a review from jtschuster July 23, 2026 17:40
@sbomer
sbomer merged commit ce07ce4 into dotnet:main Jul 23, 2026
128 of 131 checks passed
@github-project-automation github-project-automation Bot moved this to Done in AppModel Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Tools-ILLink .NET linker development as well as trimming analyzers linkable-framework Issues associated with delivering a linker friendly framework

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Trimming] ipconstprop deletes cleanup (SemaphoreSlim.Release / Dispose / Monitor.Exit) inside an async finally in a preserved method

4 participants