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.

Sign up for GitHub

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

Jump to bottom

Fix extra stack frame on exception stack trace #99263

Merged
merged 1 commit into from
Mar 6, 2024
Merged

Fix extra stack frame on exception stack trace #99263

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,13 @@ private static void DispatchEx(scoped ref StackFrameIterator frameIter, ref ExIn

DebugScanCallFrame(exInfo._passNumber, frameIter.ControlPC, frameIter.SP);

UpdateStackTrace(exceptionObj, exInfo._frameIter.FramePointer, (IntPtr)frameIter.OriginalControlPC, frameIter.SP, ref isFirstRethrowFrame, ref prevFramePtr, ref isFirstFrame, ref exInfo);
#if !NATIVEAOT
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want NATIVEAOT behavior to be different?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am actually not sure if the issue occurs with NativeAOT too. It uses a different way of filtering frames in the UpdateStackTrace based on equal frame pointers, which doesn't work on coreclr. So I've made the change for coreclr, but plan to investigate the nativeaot case.

Copy link
Member

@jkotas jkotas Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What does the diagnostic test that this fixing do?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is the NestedException2 test. Basically, there is this code:

        public static void Foo()
        {
            try
            {
                Console.WriteLine("Foo calling Bar");
                ExceptionsTestHelper.Bar<int>(1);
            }
            catch (Exception inner)
            {
                throw new Exception("doh", inner);
            }
        }
    }

The ExceptionsTestHelper.Bar<int>(1); just throws InvalidOperationException. The issue is that the Exception thrown in the code above has the line of ExceptionsTestHelper.Bar<int>(1); on the stack trace.

Copy link
Member

Choose a reason for hiding this comment

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

The issue is that the Exception thrown in the code above has the line of ExceptionsTestHelper.Bar(1); on the stack trace.

I see the My.Main() line duplicated:

C:\runtime\artifacts\bin\coreclr\windows.x64.Release>corerun test.exe
Foo calling Bar
Unhandled exception. System.Exception: doh
 ---> System.InvalidOperationException: Operation is not valid due to the current state of the object.
   at My.Bar[T](Int32 x)
   at My.Main()
   --- End of inner exception stack trace ---
   at My.Main()

C:\runtime\artifacts\bin\coreclr\windows.x64.Release>set DOTNET_LegacyExceptionHandling=0

C:\runtime\artifacts\bin\coreclr\windows.x64.Release>corerun test.exe
Foo calling Bar
Unhandled exception. System.Exception: doh
 ---> System.InvalidOperationException: Operation is not valid due to the current state of the object.
   at My.Bar[T](Int32 x)
   at My.Main()
   --- End of inner exception stack trace ---
   at My.Main()
   at My.Main()

There is ifdefed-out code in UpdateStackTrace that seems to be dealing with this situation:

runtime/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/ExceptionHandling.cs

Lines 966 to 968 in 68f7885

#if NATIVEAOT
if ((prevFramePtr == UIntPtr.Zero) || (curFramePtr != prevFramePtr))
#endif

Can this bug be fixed by deleting the ifdef in UpdateStackTrace?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, that's what I've mentioned above. The frame pointer check doesn't work in coreclr, since there are cases where two non-funclet methods have the same frame pointer - when they don't use Rbp as a frame pointer and use just Rsp instead. I've actually attempted to fix the problem by removing the ifdef, thinking I was originally wrong. But then another diagnostic test started to fail due the fact I've just mentioned.

// Don't add frames at collided unwind
if (startIdx == MaxTryRegionIdx)
#endif
{
UpdateStackTrace(exceptionObj, exInfo._frameIter.FramePointer, (IntPtr)frameIter.OriginalControlPC, frameIter.SP, ref isFirstRethrowFrame, ref prevFramePtr, ref isFirstFrame, ref exInfo);
}

byte* pHandler;
if (FindFirstPassHandler(exceptionObj, startIdx, ref frameIter,
Expand Down
Loading