-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change in exception handling in 9.0 #101772
Comments
cc: @janvorli, is this related to your work? |
It goes back to the 8.0 behavior with |
To confirm that CI catches dotnet/runtime#101772
I'll look into it, the .NET 9 behavior with the new EH implementation is clearly behaving incorrectly. |
I have investigated it. It turns out that NativeAOT shares the same issue when compiled for Debug. Release build doesn't have this issue even in coreclr. The issue is caused by the JIT ordering of exception clauses. In this specific case, the clause for the finally marked by the "// Required for repro" comment is placed in between the clauses of the main try block. When the new EH and the NativeAOT EH unwinds out of the "catch", the unwinder moves to the frame of the first throw. It then wants to skip all clauses that belong to the same try block as the catch clause that was invoked to handle the first exception. But the inserted clause from the other try block causes it to believe there are no more clauses for the same try.
cc: @dotnet/jit-contrib |
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
FYI there is an aspnetcore test that is impacted by this - dotnet/aspnetcore#55564. I'm guessing that's how Andrew noticed it. It only fails in a local test run because CI isn't DEBUG. We can leave it failing until there is a fix and then check it's resolved. |
From the JIT dump, this is the current ordering of the funclet section:
I tried changing the JIT's funclet creation logic to sort handlers based on their corresponding try regions' indices, such that handlers for
With this layout, where the handlers for each try region are contiguous, I'm still getting the same incorrect behavior. @janvorli how is |
@amanasifkhalid is this a block layout issue or an EH region descriptor issue? |
@AndyAyersMS between the two, I think the latter is more likely? Unless I'm misunderstanding the invariants around how EH descriptors should be maintained when creating funclets, I don't see any obvious issues. Before funclet creation, the blocklist looks like this:
And afterwards:
Here's the EH table:
One thing that looks weird to me is On a side note, we might be able to simplify |
If this is a regression vs 8.0 then comparing jit dumps for 8 & 9 might prove instructive. Perhaps there was an implicit constraint somewhere that now needs to be explicit, given the changes we've made over the past few months? |
I took a look at the dumps for .NET 8 vs 9, and I don't see any diffs in the EH descriptors or funclet layout (aside from some JIT-specific semantic differences, like the absence of
And on .NET 9:
The diffs in the usage of the |
@amanasifkhalid the isSameTry is the CORINFO_EH_CLAUSE_SAMETRY flag. That also needs to be correct in order to make things work. |
I am sorry for a late response. I have written my response here few days ago, but apparently I must have forgotten to push the comment button. runtime/src/coreclr/jit/codegencommon.cpp Lines 2404 to 2416 in 189d768
It needs to be set correctly too in order to make things work. |
@janvorli no worries, thank you for pointing this snippet out! I'll take another look at this later today |
What is the status of this issue? ASP.NET Core local tests are still impacted by the regression. It seems like a pretty low-level problem. Leaving it to the end of .NET 9 dev cycle doesn't seem like a good idea. |
@janvorli I tweaked the JIT to propagate the runtime/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs Lines 1018 to 1025 in 53f4614
The During unwinding, if we are trying to skip over try regions with the same offsets as the last try region, and we encounter an |
Just to confirm nothing relevant on the JIT side has changed, I can reproduce this issue on .NET 8 when targeting NativeAOT in Debug -- the ordering of the EH clauses is the same as above. |
Also just to clarify on the behavioral diffs between Debug and Release, when optimizing, the JIT is able to remove the "required for repro" finally clause during its empty try removal opt pass (Compiler::fgRemoveEmptyTry); in Debug builds, this pass doesn't run. If we tweak the problematic try-finally to have code with side effects in Release builds, such as by replacing the @janvorli I don't think this can be fixed on the codegen side. I'm guessing the new/NativeAOT unwinder needs to be able to tolerate sibling EH clauses being interweaved with "same try" clauses when skipping frames -- would it be too costly to change the unwinder to tolerate this? |
It should be fine to change the order in which the JIT encodes EH clauses (without affecting any of the internal JIT invariants). The current EH clause encoding in If it helps, #88072 (comment) has more context about the motivation for |
For native AOT, the equivalent check is done earlier at build time: runtime/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs Lines 911 to 916 in 5505150
|
IMHO, this should be fixed in the JIT. I do not think that it is worth it to be changing the invariants of the EH encodings to fix this bug. |
Thank you for clarifying this! With that in mind, I agree it's easier to fix this on the JIT side. I've opened #104531 to adjust the order in which EH clauses are reported to the VM. With those changes, I can no longer repro the incorrect behavior from above; here's what the EH table reported the VM now looks like (EH#3 is the problematic try-finally):
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
FYI the ASP.NET Core tests now pass. |
Description
If you have a try-catch-catch, I don't believe C# allows the second catch to handle exceptions thrown by the first catch. At least, that's the behavior I'm seeing in 8.0. In 9.0, there seem to be cases where the second catch can handle them.
Reproduction Steps
Expected behavior
8.0 printed
Actual behavior
9.0.100-preview.5.24229.2 prints
Regression?
No response
Known Workarounds
No response
Configuration
9.0.100-preview.5.24229.2 (in aspnetcore repo)
Win11 22631.3447 on x64 (haven't tried others)
Other information
The nested try finally in the unreachable catch block seems essential to the repro.
The text was updated successfully, but these errors were encountered: