Fix GC hole with filter liveness

[BruceForstall]

Execution following a filter clause can be to finally/fault clauses nested within the try region that the filter protects. E.g.,

   try {
      try {
         // 0
      } finally {
         // 1
      }
   } filter {
      // 2
   } filter-handler {
   }

In this case, after 2 executes, 1 might execute if the exception came from 0. This is a consequence of the 2-pass EH model. This means that GC variables in 1 must be kept alive when 2 is executed, in case a GC occurs during the execution of 2.

Code to handle this case was introduced with dotnet/coreclr#23044, however it didn't handle the case where a filter is a top-level EH construct (and not nested within another try).

This change callers of fgGetHandlerLiveVars (and it's virtual copy PromotionLiveness::AddHandlerLiveVars()) to always call it (not just when ehBlockHasExnFlowDsc() is true) so the code that handles the filter case has a chance to run. To make this a little more efficient, the VARSET_TP that will be filled in is expected to be pre-allocated and passed in.

Fixes #88168

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Execution following a filter clause can be to finally/fault clauses nested within the try region that the filter protects. E.g.,

   try {
      try {
      } finally {
         // 1
      }
   } filter {
      // 2
   } filter-handler {
   }

In this case, after 2 executes, 1 might execute. This is a consequence of the 2-pass EH model. This means that GC variables in 1 must be kept alive when 2 is executed, in case a GC occurs during the execution of 2.

Code to handle this case was introduced with dotnet/coreclr#23044, however it didn't handle the case where a filter is a top-level EH construct (and not nested within another try).

This change callers of fgGetHandlerLiveVars (and it's virtual copy PromotionLiveness::AddHandlerLiveVars()) to always call it (not just when ehBlockHasExnFlowDsc() is true) so the code that handles the filter case has a chance to run. To make this a little more efficient, the VARSET_TP that will be filled in is expected to be pre-allocated and passed in.

Fixes #88168

Author:	BruceForstall
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[jakobbotsch]

I'm fine with this fix as is since it just builds on what's there, but note that there are broader issues when we don't model these successors in the usual way, and special case it in liveness: see e.g. #86538 (comment).

Ideally we would fix this by fixing the BB successor visitor code. I tried to do that here: https://github.com/dotnet/runtime/compare/main...jakobbotsch:fix-86538?expand=1

There were two problems:

1. As far as I remember, the assumption made here is incorrect. There are cases where this is not true.
2. How do we efficiently implement BlockPredsWithEH? Perhaps iterating over all blocks is ok for this rare case -- there is already a case where we do so. This is how I found the above issue (by asserting that the found predecessors really did have the current block as the successor according to VisitEHSecondPassSuccs).

Anyway, as mentioned, I'm ok with this fix, but maybe you have some ideas on the above?

[edit] The TP impact looks unexpectedly significant

[BruceForstall]

Diffs -- a few diffs with extended variable lifetimes, including the function identified in the bug (System.Runtime.Serialization.XsdDataContractExporter:Export).

TP looks pretty bad, though: up to +0.98%.

[BruceForstall]

It would be nice to unify our successor iterators, as suggested in #86538.

However:

As far as I remember, the assumption made here is incorrect. There are cases where this is not true.

I'd be surprised if that assumption is incorrect. That's pretty fundamental.

[jakobbotsch]

The particular case is the following context in the current win-x64 coreclr_tests.run collection:

****** START compiling Microsoft.Build.Execution.BuildManager+<>c__DisplayClass100_0:<IssueBuildRequestForBuildSubmission>g__IssueBuildSubmissionToSchedulerImpl|1(Microsoft.Build.Execution.BuildSubmission,bool):this (MethodHash=4030165b)

It has an EH table that looks like the following when we build SSA:

***************  Exception Handling table
index  eTry, eHnd
  0  ::   3        - Try at BB03..BB15 [010..095), Fault   at BB19..BB21 [095..09F)
  1  ::         3  - Try at BB40..BB43 [118..176), Finally at BB22..BB24 [176..180)
  2  ::         3  - Try at BB47..BB47 [199..1CD), Finally at BB25..BB27 [1CD..1D7)
  3  ::            - Try at BB02..BB17 [002..0A4), Filter at BB28..BB31 [0A4..0C4), Handler at BB32..BB50 [0C4..1D9)

Note that EH0 is enclosed by EH3, but EH1 and EH2 aren't and cause the search to exit before BB19 is reported as a successor of BB28 to BB31. It seems like the EH table has EH descriptors for the EH clauses nested within the handler of EH3 before the filter, so perhaps we just need to identify this case and continue the search?
The EH table corresponds to the following C# code: https://github.com/dotnet/msbuild/blob/a9034a279cdb9f31166fa1212439822e6b2fbf56/src/Build/BackEnd/BuildManager/BuildManager.cs#L1768-L1830

edit: Perhaps the logic should actually be using ebdGetEnclosingRegionIndex instead of ehGetEnclosingTryIndex?

[jakobbotsch]

Another case is

****** START compiling System.Net.Http.Http2Connection+Http2Stream+<SendDataAsync>d__81:MoveNext():this (MethodHash=e59e56f7)

from win-x64 libraries_tests.pmi. It has an EH table that looks like:

***************  Exception Handling table
index  eTry, eHnd
  0  ::   3        - Try at BB15..BB31 [07B..111), Fault   at BB105..BB108 [111..121)
  1  ::   3        - Try at BB67..BB72 [1A9..1C3), Fault   at BB109..BB112 [1C3..1D3)
  2  ::   4     3  - Try at BB122..BB125 [2C6..2F8), Finally at BB113..BB116 [2F8..308)
  3  ::   4        - Try at BB11..BB91 [055..290), Filter at BB117..BB120 [290..2BA), Handler at BB121..BB128 [2BA..30A)
  4  ::   5        - Try at BB10..BB91 [055..30C), Finally at BB129..BB136 [30C..31C)
  5  ::            - Try at BB02..BB102 [00E..31E), Handler at BB137..BB137 [31E..343)

EH3 has successors due to EH0 and EH1, but stops the backwards iteration when it gets to EH2.

[jakobbotsch]

I pushed a commit to my branch linked above that implements the BlockPredsWithEH part with the assert that finds these.

[jakobbotsch]

@BruceForstall I've submitted #89275 that folds this change in and fixes both #86538, #88168, the issue I have identified above (by switching to ehGetEnclosingRegionIndex instead of ehGetEnclosingTryIndex) and also fixes the TP regression (locally).

[BruceForstall]

It seems like the EH table has EH descriptors for the EH clauses nested within the handler of EH3 before the filter, so perhaps we just need to identify this case and continue the search?

Yes, this makes sense. All the EH clauses nested within the try as well as those nested within the handler will appear before the outer try/handler clause.

[BruceForstall]

Subsumed by #89275