JIT: fix ACD EH region removal updates for filters #115054
Conversation
A filter ACD may change region number because some other EH region was deleted. Handle this properly. Fixes dotnet#114895
ghost
added
the
area-CodeGen-coreclr
|
@EgorBo PTAL |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with filter ACDs during EH region removal, ensuring they are handled properly when EH regions are deleted. Key changes include:
- Adding a new regression test (Runtime_114324.cs) to trigger the ACD filter update.
- Inserting additional JIT logging messages in jiteh.cpp and fgehopt.cpp to trace EH removals and ACD updates.
- Updating flowgraph.cpp to assert and correctly adjust the key designator for filter and handler ACD entries.
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/tests/JIT/Regression/JitBlue/Runtime_114895/Runtime_114324.cs | New regression test to verify EH region removal handling for filters. |
| src/coreclr/jit/jiteh.cpp | Added logging for EH removal and ACD update tracking. |
| src/coreclr/jit/flowgraph.cpp | Updated key designator handling with new asserts and conditional logic. |
| src/coreclr/jit/fgehopt.cpp | Added logging before and after ACD updates during EH table entry removal. |
Files not reviewed (1)
- src/tests/JIT/Regression/JitBlue/Runtime_114895/Runtime_114324.csproj: Language not supported
| { | ||
| System.Console.WriteLine(vr11); | ||
| } | ||
| catch (System.Exception)when (s_31[0]) |
There was a problem hiding this comment.
The static array 's_31' is declared but never initialized, which may lead to a NullReferenceException when accessing s_31[0]. Consider initializing 's_31' before it is used in the filter.
Copilot uses AI. Check for mistakes.
| @@ -3909,26 +3911,35 @@ bool Compiler::AddCodeDsc::UpdateKeyDesignator(Compiler* compiler) | |||
| { | |||
| // Non-funclet case | |||
| // | |||
| assert(acdKeyDsg != AcdKeyDesignator::KD_FLT); | |||
There was a problem hiding this comment.
The assert ensures that non-filter cases do not inadvertently retain a filter key designator. Consider adding a brief inline comment to clarify the invariant and reasoning behind the assert for future maintainers.
Copilot uses AI. Check for mistakes.
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
SPMI shows diffs but this makes no sense, clearly the inlining is different. Looks like jitrollingbuild pipeline has lost permissions to upload new jits. |
|
You actually mean: Fixes #114985 |
Yes. |
A filter ACD may change region number because some other EH region was deleted. Handle this properly.
Fixes
#114895#114985