From 73de9016df46085dd3602f028cebc3add796fddf Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 3 Aug 2023 14:57:57 +0200 Subject: [PATCH 1/2] Fix isIPAdjusted setting during exception handling The CrawlFrame::isIPAdjusted flag is set to true for all frames when handling software exceptions. That is not correct, as the IP is not adjusted in those cases. I've also cleaned up the values the flag is set to at various places to use the real type of the member, which is `bool`. It was being set to TRUE / FALSE or even 0 / 1 at some places. As a related cleanup, I've removed the FRAME_ATTR_OUT_OF_LINE flag as it was never set anywhere and still was being tested at few places. --- src/coreclr/vm/exceptionhandling.cpp | 1 - src/coreclr/vm/frames.h | 1 - src/coreclr/vm/stackwalk.cpp | 19 +++++++++---------- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 79daadcb1b7a9b..be5abf936483d6 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -1573,7 +1573,6 @@ void ExceptionTracker::InitializeCrawlFrame(CrawlFrame* pcfThisFrame, Thread* pT pcfThisFrame->pThread = pThread; Frame* pTopFrame = pThread->GetFrame(); - pcfThisFrame->isIPadjusted = (FRAME_TOP != pTopFrame) && (pTopFrame->GetVTablePtr() != FaultingExceptionFrame::GetMethodFrameVPtr()); if (pcfThisFrame->isFrameless && (pcfThisFrame->isIPadjusted == false) && (pcfThisFrame->GetRelOffset() == 0)) { // If we are here, then either a hardware generated exception happened at the first instruction diff --git a/src/coreclr/vm/frames.h b/src/coreclr/vm/frames.h index f8d50b445d34cb..cba39a808a9866 100644 --- a/src/coreclr/vm/frames.h +++ b/src/coreclr/vm/frames.h @@ -396,7 +396,6 @@ class Frame : public FrameBase enum FrameAttribs { FRAME_ATTR_NONE = 0, FRAME_ATTR_EXCEPTION = 1, // This frame caused an exception - FRAME_ATTR_OUT_OF_LINE = 2, // The exception out of line (IP of the frame is not correct) FRAME_ATTR_FAULTED = 4, // Exception caused by Win32 fault FRAME_ATTR_RESUMABLE = 8, // We may resume from this frame FRAME_ATTR_CAPTURE_DEPTH_2 = 0x10, // This is a helperMethodFrame and the capture occurred at depth 2 diff --git a/src/coreclr/vm/stackwalk.cpp b/src/coreclr/vm/stackwalk.cpp index 6abe943c47b4c4..cb1f63a52b41df 100644 --- a/src/coreclr/vm/stackwalk.cpp +++ b/src/coreclr/vm/stackwalk.cpp @@ -1386,7 +1386,7 @@ BOOL StackFrameIterator::ResetRegDisp(PREGDISPLAY pRegDisp, if (m_crawl.isInterrupted) { m_crawl.hasFaulted = ((uFrameAttribs & Frame::FRAME_ATTR_FAULTED) != 0); - m_crawl.isIPadjusted = ((uFrameAttribs & Frame::FRAME_ATTR_OUT_OF_LINE) != 0); + m_crawl.isIPadjusted = false; } m_crawl.pFrame->UpdateRegDisplay(m_crawl.pRD); @@ -2537,10 +2537,10 @@ StackWalkAction StackFrameIterator::NextRaw(void) DBG_ADDR(GetRegdisplaySP(m_crawl.pRD)), DBG_ADDR(GetControlPC(m_crawl.pRD)))); - m_crawl.isFirst = FALSE; - m_crawl.isInterrupted = FALSE; - m_crawl.hasFaulted = FALSE; - m_crawl.isIPadjusted = FALSE; + m_crawl.isFirst = false; + m_crawl.isInterrupted = false; + m_crawl.hasFaulted = false; + m_crawl.isIPadjusted = false; #ifndef PROCESS_EXPLICIT_FRAME_BEFORE_MANAGED_FRAME // remember, x86 handles the managed stack frame before the explicit frames contained in it @@ -2578,8 +2578,7 @@ StackWalkAction StackFrameIterator::NextRaw(void) if (m_crawl.isInterrupted) { m_crawl.hasFaulted = (uFrameAttribs & Frame::FRAME_ATTR_FAULTED) != 0; - m_crawl.isIPadjusted = (uFrameAttribs & Frame::FRAME_ATTR_OUT_OF_LINE) != 0; - _ASSERTE(!m_crawl.hasFaulted || !m_crawl.isIPadjusted); // both cant be set together + m_crawl.isIPadjusted = false; } PCODE adr = m_crawl.pFrame->GetReturnAddress(); @@ -3214,9 +3213,9 @@ void StackFrameIterator::PostProcessingForNoFrameTransition() m_crawl.isFrameless = true; // Flags the same as from a FaultingExceptionFrame. - m_crawl.isInterrupted = 1; - m_crawl.hasFaulted = 1; - m_crawl.isIPadjusted = 0; + m_crawl.isInterrupted = true; + m_crawl.hasFaulted = true; + m_crawl.isIPadjusted = false; #if defined(STACKWALKER_MAY_POP_FRAMES) // If Frames would be unlinked from the Frame chain, also reset the UseExInfoForStackwalk bit From 627a3c021aab357d3d4a0028cff1e9cbd9e122cf Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 3 Aug 2023 17:31:30 +0200 Subject: [PATCH 2/2] Add regression test --- .../coreclr/GitHub_89834/test89834.cs | 129 ++++++++++++++++++ .../coreclr/GitHub_89834/test89834.csproj | 10 ++ 2 files changed, 139 insertions(+) create mode 100644 src/tests/Regressions/coreclr/GitHub_89834/test89834.cs create mode 100644 src/tests/Regressions/coreclr/GitHub_89834/test89834.csproj diff --git a/src/tests/Regressions/coreclr/GitHub_89834/test89834.cs b/src/tests/Regressions/coreclr/GitHub_89834/test89834.cs new file mode 100644 index 00000000000000..a55ac117383e5a --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_89834/test89834.cs @@ -0,0 +1,129 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; +using System.Runtime.CompilerServices; +using System.Runtime.ExceptionServices; +using System.Runtime.InteropServices; +using System.Text; +using System.Threading; +using System.Threading.Tasks; + +public class test89834 +{ + public static async Task Main() + { + int status = 103; + Console.WriteLine("boo"); + + EventHandler handler = + (s, e) => FirstChanceExceptionCallback(e.Exception, ref status); + + AppDomain.CurrentDomain.FirstChanceException += handler; + try + { + await ThrowAndCatchTaskCancellationExceptionAsync(); + } + finally + { + AppDomain.CurrentDomain.FirstChanceException -= handler; + } + + return status; + } + + private static readonly ThreadLocal ReentrancyTracker = new(); + + private static void FirstChanceExceptionCallback(Exception thrownException, ref int status) + { + if (ReentrancyTracker.Value) + return; + + ReentrancyTracker.Value = true; + + Console.WriteLine("Begin Observing Exception: " + thrownException.GetType()); + + status = ValidateExceptionStackFrame(thrownException); + Console.WriteLine("End Observing Exception: " + thrownException.GetType()); + + ReentrancyTracker.Value = false; + } + + private static int ValidateExceptionStackFrame(Exception thrownException) + { + StackTrace exceptionStackTrace = new(thrownException, fNeedFileInfo: false); + + // The stack trace of thrown exceptions is populated as the exception unwinds the + // stack. In the case of observing the exception from the FirstChanceException event, + // there is only one frame on the stack (the throwing frame). In order to get the + // full call stack of the exception, get the current call stack of the thread and + // filter out the call frames that are "above" the exception's throwing frame. + StackFrame throwingFrame = null; + foreach (StackFrame stackFrame in exceptionStackTrace.GetFrames()) + { + if (null != stackFrame.GetMethod()) + { + throwingFrame = stackFrame; + break; + } + } + + if (throwingFrame == null) + { + return 101; + } + + Console.WriteLine($"Throwing Frame: [{throwingFrame.GetMethod().Name}, 0x{GetOffset(throwingFrame):X}]"); + + StackTrace threadStackTrace = new(fNeedFileInfo: false); + ReadOnlySpan threadStackFrames = threadStackTrace.GetFrames(); + int index = 0; + + Console.WriteLine("Begin Checking Thread Frames:"); + while (index < threadStackFrames.Length) + { + StackFrame threadStackFrame = threadStackFrames[index]; + + Console.WriteLine($"- [{threadStackFrame.GetMethod().Name}, 0x{GetOffset(threadStackFrame):X}]"); + + if (throwingFrame.GetMethod() == threadStackFrame.GetMethod() && + GetOffset(throwingFrame) == GetOffset(threadStackFrame)) + { + break; + } + + index++; + } + Console.WriteLine("End Checking Thread Frames:"); + + return (index != threadStackFrames.Length) ? 100 : 102; + } + + private static int GetOffset(StackFrame stackFrame) + { + return stackFrame.GetILOffset(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static async Task ThrowAndCatchTaskCancellationExceptionAsync() + { + using CancellationTokenSource source = new(); + CancellationToken token = source.Token; + + Task innerTask = Task.Run( + () => Task.Delay(Timeout.InfiniteTimeSpan, token), + token); + + try + { + source.Cancel(); + await innerTask; + } + catch (Exception) + { + } + } +} diff --git a/src/tests/Regressions/coreclr/GitHub_89834/test89834.csproj b/src/tests/Regressions/coreclr/GitHub_89834/test89834.csproj new file mode 100644 index 00000000000000..6fb353a366d90f --- /dev/null +++ b/src/tests/Regressions/coreclr/GitHub_89834/test89834.csproj @@ -0,0 +1,10 @@ + + + Exe + true + 1 + + + + +