JIT: Run single EH region repair pass after layout #108634

amanasifkhalid · 2024-10-08T01:51:25Z

Fixes #108608. To simplify EH maintenance logic, leave EH regions in a deconstructed phase during block reordering, and correct the end block pointers of each region in a pass over the block list after. By walking the list backwards, we can short-circuit finding the end of each region, and we don't run into the issue of an EH region not being able to determine if it's at the end of its parent region due to some awkward placement of a sibling region.

amanasifkhalid · 2024-10-08T03:28:56Z

/azp run runtime-coreclr outerloop, Antigen, Fuzzlyn

azure-pipelines · 2024-10-08T03:29:22Z

Azure Pipelines successfully started running 3 pipeline(s).

amanasifkhalid · 2024-10-08T14:33:13Z

No diffs. Antigen and Fuzzlyn runs didn't hit anything related. Outerloop failures are #104927.

cc @dotnet/jit-contrib, @AndyAyersMS PTAL. This doesn't look like a surgical fix, but I think it's preferable to trying to patch up the edge cases of the old implementation. That approach felt pattern-matchy and thus fragile, and keeping the temporary end block of a parent EH region up-to-date during moves in fgMoveColdBlocks incurred diffs, since we use these temporary end pointers as insertion points for cold blocks. The diffs looked like an improvement, but we probably want to avoid codegen churn for .NET 9 at this point, and I'm planning on making fgMoveColdBlocks more EH-agnostic for .NET 10 anyway. I hope the simplicity of the new approach justifies its case for .NET 9, though I'm open to suggestions for more focused fixes -- either way, this is the approach I'm thinking of taking for .NET 10.

amanasifkhalid · 2024-10-08T14:34:46Z

src/coreclr/jit/jiteh.cpp

+{
+    assert(compHndBBtabCount != 0);
+    bool* const tryEndsSet   = new (this, CMK_Generic) bool[compHndBBtabCount * 2]{};
+    bool* const hndEndsSet   = tryEndsSet + compHndBBtabCount;


I suppose we could use bitvectors here instead, and avoid heap allocations altogether, at the expense (?) of indexing speed. Any preference here?

You could instead to walk the EH table first and null out ebdTryLast and ebdHndLast, then use the null values there as your "not yet set" indicators?

Good point. I'm guessing it's faster/easier to vectorize zeroing out an array versus nulling out each clause's end pointers, though the reduction in local state seems ideal.

AndyAyersMS

When you run diffs you also need to look at all the zero-sized diffs to see if there are any EH clause diffs.

I don't know if you can see those in the CI reports.

AndyAyersMS · 2024-10-08T15:03:15Z

src/coreclr/jit/jiteh.cpp

+{
+    assert(compHndBBtabCount != 0);
+    bool* const tryEndsSet   = new (this, CMK_Generic) bool[compHndBBtabCount * 2]{};
+    bool* const hndEndsSet   = tryEndsSet + compHndBBtabCount;


You could instead to walk the EH table first and null out ebdTryLast and ebdHndLast, then use the null values there as your "not yet set" indicators?

amanasifkhalid · 2024-10-08T15:28:27Z

When you run diffs you also need to look at all the zero-sized diffs to see if there are any EH clause diffs.

I ran asmdiffs locally, and I didn't see anything reported, so I'm assuming this is truly a no-diff change? If we saw a change in the EH clauses due to different block placement, we'd see codegen diffs too, and if we saw the clauses change without any blocks moving, then that would probably be a correctness issue and trigger asserts, right? In other words, for a given block layout, there's only one correct set of EH clauses, so any diffs for the latter are wrong.

AndyAyersMS · 2024-10-08T15:40:55Z

Maybe another way of putting it -- do you see SPMI diffs on the repro case (with release compilers)? Presumably you should? Or else does the release JIT get the right results anyway?

amanasifkhalid · 2024-10-08T16:01:08Z

Maybe another way of putting it -- do you see SPMI diffs on the repro case (with release compilers)? Presumably you should? Or else does the release JIT get the right results anyway?

I see. With Checked compilers, I can see the EH clauses are now correct post-layout. Here's the problematic one:

---------------------------------------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight   [IL range]   [jump]                            [EH region]        [flags]
---------------------------------------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1    [000..019)-> BB02(0.5),BB11(0.5)     ( cond )                     i
BB02 [0037]  1       BB01                  8    [0B3..0CF)-> BB03(0.5),BB11(0.5)     ( cond )                     i bwd
BB03 [0003]  1  2    BB02                  2    [01A..01E)-> BB04(1),BB06(0)         ( cond ) T2      try {       i keep loophead hascall bwd
BB04 [0042]  2  2    BB03,BB06             2    [01E..???)-> BB05(1)                 (always) T2                  i keep hascall bwd
BB05 [0014]  4  2    BB13,BB05,BB04,BB08 256    [026..07E)-> BB07(0.5),BB05(0.5)     ( cond ) T2                  i loophead nullcheck bwd bwd-src
BB06 [0044]  1  2    BB03                  0    [01E..01E)-> BB04(1)                 (always) T2                  rare internal
BB07 [0011]  1  1    BB05                128    [???..???)-> BB08(1),BB09(0)         ( cond ) T1      try {       i keep hascall bwd
BB08 [0045]  2  1    BB07,BB09           128    [057..???)-> BB05(1)                 (always) T1                  i keep hascall bwd
BB09 [0047]  1  1    BB07                  0    [???..???)-> BB08(1)                 (always) T1      } }         rare internal
BB10 [0008]  0  0                          0    [03E..04F)                           (throw ) T0      try { }     i rare keep bwd
BB11 [0041]  2       BB01,BB02             1    [019..0D0)                           (return)                     i
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ funclets follow
BB12 [0009]  1  2  0                       0    [04F..057)                           (throw ) T2 H0 F fault { }   i rare keep flet nullcheck bwd
BB13 [0012]  1  2  1                       0    [061..064)-> BB05(1)                 ( cret ) T2 H1 F catch { }   i rare keep xentry flet bwd
BB14 [0016]  1     2                       0    [080..095)                           (falret)    H2 F fault { }   i rare keep xentry flet bwd
---------------------------------------------------------------------------------------------------------------------------------------------------------------------

***************  Exception Handling table
index  eTry, eHnd
  0  ::   2        - Try at BB10..BB10 [03E..04F), Fault   at BB12..BB12 [04F..057)
  1  ::   2        - Try at BB07..BB09 [058..061), Handler at BB13..BB13 [061..064)
  2  ::            - Try at BB03..BB09 [01B..080), Fault   at BB14..BB14 [080..095)

And the correct one (notice the end block for T2 being pushed back by one):

---------------------------------------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight   [IL range]   [jump]                            [EH region]        [flags]
---------------------------------------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1    [000..019)-> BB02(0.5),BB11(0.5)     ( cond )                     i
BB02 [0037]  1       BB01                  8    [0B3..0CF)-> BB03(0.5),BB11(0.5)     ( cond )                     i bwd
BB03 [0003]  1  2    BB02                  2    [01A..01E)-> BB04(1),BB06(0)         ( cond ) T2      try {       i keep loophead hascall bwd
BB04 [0042]  2  2    BB03,BB06             2    [01E..???)-> BB05(1)                 (always) T2                  i keep hascall bwd
BB05 [0014]  4  2    BB13,BB05,BB04,BB08 256    [026..07E)-> BB07(0.5),BB05(0.5)     ( cond ) T2                  i loophead nullcheck bwd bwd-src
BB06 [0044]  1  2    BB03                  0    [01E..01E)-> BB04(1)                 (always) T2                  rare internal
BB07 [0011]  1  1    BB05                128    [???..???)-> BB08(1),BB09(0)         ( cond ) T1      try {       i keep hascall bwd
BB08 [0045]  2  1    BB07,BB09           128    [057..???)-> BB05(1)                 (always) T1                  i keep hascall bwd
BB09 [0047]  1  1    BB07                  0    [???..???)-> BB08(1)                 (always) T1      }           rare internal
BB10 [0008]  0  0                          0    [03E..04F)                           (throw ) T0      try { } }   i rare keep bwd
BB11 [0041]  2       BB01,BB02             1    [019..0D0)                           (return)                     i
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ funclets follow
BB12 [0009]  1  2  0                       0    [04F..057)                           (throw ) T2 H0 F fault { }   i rare keep flet nullcheck bwd
BB13 [0012]  1  2  1                       0    [061..064)-> BB05(1)                 ( cret ) T2 H1 F catch { }   i rare keep xentry flet bwd
BB14 [0016]  1     2                       0    [080..095)                           (falret)    H2 F fault { }   i rare keep xentry flet bwd
---------------------------------------------------------------------------------------------------------------------------------------------------------------------

***************  Exception Handling table
index  eTry, eHnd
  0  ::   2        - Try at BB10..BB10 [03E..04F), Fault   at BB12..BB12 [04F..057)
  1  ::   2        - Try at BB07..BB09 [058..061), Handler at BB13..BB13 [061..064)
  2  ::            - Try at BB03..BB10 [01B..080), Fault   at BB14..BB14 [080..095)

I don't think we can dump EH tables in Release builds by default, though I can tweak this locally and take a look. Are you asking about this to evaluate the risk of this bug in product builds?

AndyAyersMS · 2024-10-08T16:09:47Z

I don't think we can dump EH tables in Release builds by default, though I can tweak this locally and take a look. Are you asking about this to evaluate the risk of this bug in product builds?

In part, yes. It is good at this stage to be able to say what might happen if we don't fix this.

AndyAyersMS · 2024-10-08T16:12:57Z

Also note SPMI doesn't rely on dumping, it is comparing the binary data the JIT sends back to the VM (which includes the EH info), so release mode diffs "work" but may be harder to understand.

amanasifkhalid · 2024-10-08T18:17:55Z

Got it, I created a collection of the repro, and replayed it with Release builds. I got a zero-sized diff, so without the fix, it looks like we're propagating the incorrect EH clause info to the VM. For this particular method, that doesn't seem to affect behavior at runtime, but I can imagine this breaking stack walking for methods that throw in inconvenient places.

amanasifkhalid · 2024-10-09T14:20:10Z

@AndyAyersMS since we're seeing diffs with Release builds, do you think a .NET 9 fix is justified, or would you like to see a repro where stack walking breaks from this? I can try to put something together.

AndyAyersMS · 2024-10-09T14:53:12Z

I'd say we should go ahead and backport, given that we know of a case where we got things wrong.

amanasifkhalid · 2024-10-09T14:55:12Z

/backport to release/9.0

github-actions · 2024-10-09T14:55:24Z

Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/11257587584

github-actions · 2024-10-09T14:57:01Z

@amanasifkhalid backporting to release/9.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Add EH region ends pass
Applying: Remove old EH fixup logic
error: sha1 information is lacking or useless (src/coreclr/jit/compiler.h).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0002 Remove old EH fixup logic
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2024-10-09T14:57:02Z

@amanasifkhalid an error occurred while backporting to release/9.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

Fixes dotnet#108608. To simplify EH maintenance logic, leave EH regions in a deconstructed phase during block reordering, and correct the end block pointers of each region in a pass over the block list after. By walking the list backwards, we can short-circuit finding the end of each region, and we don't run into the issue of an EH region not being able to determine if it's at the end of its parent region due to some awkward placement of a sibling region.

amanasifkhalid added 3 commits October 7, 2024 21:18

Add EH region ends pass

8096ead

Remove old EH fixup logic

e33947d

Style

6d47990

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 8, 2024

dotnet-policy-service bot assigned amanasifkhalid Oct 8, 2024

build-analysis bot mentioned this pull request Oct 8, 2024

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Closed

3 tasks

amanasifkhalid marked this pull request as ready for review October 8, 2024 14:16

amanasifkhalid commented Oct 8, 2024

View reviewed changes

AndyAyersMS reviewed Oct 8, 2024

View reviewed changes

Use null EH clause pointers as set indicators

dcf647a

AndyAyersMS approved these changes Oct 9, 2024

View reviewed changes

amanasifkhalid merged commit 87273d6 into dotnet:main Oct 9, 2024

amanasifkhalid deleted the eh-region-ends branch October 9, 2024 14:55

amanasifkhalid mentioned this pull request Oct 9, 2024

[release/9.0] Backport "JIT: Run single EH region repair pass after layout" #108715

Merged

4 tasks

github-actions bot locked and limited conversation to collaborators Nov 9, 2024

JIT: Run single EH region repair pass after layout #108634

JIT: Run single EH region repair pass after layout #108634

Uh oh!

Conversation

amanasifkhalid commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amanasifkhalid commented Oct 8, 2024

Uh oh!

azure-pipelines bot commented Oct 8, 2024

Uh oh!

amanasifkhalid commented Oct 8, 2024

Uh oh!

amanasifkhalid Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

amanasifkhalid Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

amanasifkhalid commented Oct 8, 2024

Uh oh!

AndyAyersMS commented Oct 8, 2024

Uh oh!

amanasifkhalid commented Oct 8, 2024

Uh oh!

AndyAyersMS commented Oct 8, 2024

Uh oh!

AndyAyersMS commented Oct 8, 2024

Uh oh!

amanasifkhalid commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amanasifkhalid commented Oct 9, 2024

Uh oh!

AndyAyersMS commented Oct 9, 2024

Uh oh!

amanasifkhalid commented Oct 9, 2024

Uh oh!

github-actions bot commented Oct 9, 2024

Uh oh!

github-actions bot commented Oct 9, 2024

Uh oh!

github-actions bot commented Oct 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

amanasifkhalid commented Oct 8, 2024 •

edited

Loading

amanasifkhalid commented Oct 8, 2024 •

edited

Loading