Skip to content
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

[release/9.0] Remove managed EH code frames from stack trace #108831

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Oct 14, 2024

Backport of #108723 to release/9.0

/cc @janvorli

Customer Impact

[x] Customer reported
[ ] Found internally

When stack trace is captured using the System.Diagnostics.StackTrace in an exception filter, it contains internal frames of the exception handling implementation that should not be visible to the user as they are an implementation detail.

Here is an example provided by the customer who has reported the problem:

at RyuJitReproduction.Program.TestStackTraceInFilterCallee() in C:\Users\Accretion\source\dotnet\RyuJit\RyuJitReproduction\Program.cs:line 94
   at System.Runtime.EH.FindFirstPassHandler(Object exception, UInt32 idxStart, StackFrameIterator& frameIter, UInt32& tryRegionIdx, Byte*& pHandler)
   at System.Runtime.EH.DispatchEx(StackFrameIterator& frameIter, ExInfo& exInfo)
   at System.Runtime.EH.RhThrowEx(Object exceptionObj, ExInfo& exInfo)
   at RyuJitReproduction.Program.TestStackTraceInFilterCallee() in C:\Users\Accretion\source\dotnet\RyuJit\RyuJitReproduction\Program.cs:line 92
   at RyuJitReproduction.Program.TestStackTraceInFilter() in C:\Users\Accretion\source\dotnet\RyuJit\RyuJitReproduction\Program.cs:line 76
   at RyuJitReproduction.Program.Main() in C:\Users\Accretion\source\dotnet\RyuJit\RyuJitReproduction\Program.cs:line 70

And this is what it looks like in .NET 8 (and also with this fix):

   at RyuJitReproduction.Program.TestStackTraceInFilterCallee() in C:\Users\Accretion\source\dotnet\RyuJit\RyuJitReproduction\Program.cs:line 94
   at RyuJitReproduction.Program.TestStackTraceInFilterCallee() in C:\Users\Accretion\source\dotnet\RyuJit\RyuJitReproduction\Program.cs:line 92
   at RyuJitReproduction.Program.TestStackTraceInFilter() in C:\Users\Accretion\source\dotnet\RyuJit\RyuJitReproduction\Program.cs:line 76
   at RyuJitReproduction.Program.Main() in C:\Users\Accretion\source\dotnet\RyuJit\RyuJitReproduction\Program.cs:line 70

Regression

[x] Yes
[ ] No

Regression in .NET 9 due to the new exception handling being enabled by default.

Testing

Testing using a repro provided by the customer.

Risk

Low, the change only modifies metadata (adds attributes to the internal EH implementation methods that prevents showing them on the stack trace.

When StackTrace is created inside of an exception filter, it contains
stack frames of the managed exception handling code, like
System.Runtime.EH.RhThrowEx
System.Runtime.EH.DispatchEx
System.Runtime.EH.FindFirstPassHandler

These should not occur on the stack trace as they are internal
implementation detail of the new EH.

This change fixes it by adding [StackTraceHidden] attribute to these
methods.

Close #107995
Copy link
Contributor

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

@janvorli janvorli self-assigned this Oct 14, 2024
@janvorli janvorli added this to the 9.0.0 milestone Oct 14, 2024
@janvorli janvorli added the Servicing-consider Issue for next servicing release review label Oct 14, 2024
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

lgtm. please get a code review. we will take for consideration in 9 GA

@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Oct 14, 2024
@jeffschwMSFT
Copy link
Member

please take a look at the pr failures

@jkotas
Copy link
Member

jkotas commented Oct 15, 2024

please take a look at the pr failures

This is known intermittent issue with rc1 toolset. We need to update to rc2 to pick up the fix.

@jkotas
Copy link
Member

jkotas commented Oct 15, 2024

We need to update to rc2 to pick up the fix.

@carlossanlop When do we expect release/9.0 to be updated to rc2 toolset?

@jeffschwMSFT jeffschwMSFT merged commit cf1c582 into release/9.0 Oct 15, 2024
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants