From 9482f0f5c0929603add9ceb07cc9e2f5988aeea6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 7 Jun 2024 07:28:18 +0200 Subject: [PATCH 1/3] Run System.Diagnostics.StackTrace tests with NAOT Also fixes some corner case issues, like whether an empty stacktrace should stringify into a newline. --- .../Diagnostics/StackFrame.NativeAot.cs | 22 +++++++++++++---- .../Diagnostics/StackTrace.NativeAot.cs | 15 ++++++------ .../StackTraceMetadata/MethodNameFormatter.cs | 5 ++-- .../TestUtilities/System/PlatformDetection.cs | 1 + .../tests/StackFrameExtensionsTests.cs | 8 +++---- .../tests/StackFrameTests.cs | 6 ++++- .../tests/StackTraceSymbolsTests.cs | 2 +- .../tests/StackTraceTests.cs | 24 +++++++++++-------- src/libraries/tests.proj | 1 - 9 files changed, 53 insertions(+), 31 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackFrame.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackFrame.NativeAot.cs index f061cf1d461d71..ee26e6b3114c70 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackFrame.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackFrame.NativeAot.cs @@ -110,14 +110,26 @@ private void InitializeForIpAddress(IntPtr ipAddress, bool needFileInfo) [MethodImplAttribute(MethodImplOptions.NoInlining)] private void BuildStackFrame(int frameIndex, bool needFileInfo) { + IntPtr ipAddress; + const int SystemDiagnosticsStackDepth = 2; - frameIndex += SystemDiagnosticsStackDepth; - IntPtr[] frameArray = new IntPtr[frameIndex + 1]; - int returnedFrameCount = RuntimeImports.RhGetCurrentThreadStackTrace(frameArray); - int realFrameCount = (returnedFrameCount >= 0 ? returnedFrameCount : frameArray.Length); + // Unreasonably high or negative frameIndex could lead to overflows or failures to allocate + // the frameArray below so spot check it's sane. + RuntimeImports.RhGetCurrentThreadStackBounds(out IntPtr stackLow, out IntPtr stackHigh); + if (frameIndex >= -SystemDiagnosticsStackDepth && frameIndex < ((stackHigh - stackLow) / IntPtr.Size)) + { + frameIndex += SystemDiagnosticsStackDepth; + IntPtr[] frameArray = new IntPtr[frameIndex + 1]; + int returnedFrameCount = RuntimeImports.RhGetCurrentThreadStackTrace(frameArray); + int realFrameCount = (returnedFrameCount >= 0 ? returnedFrameCount : frameArray.Length); - IntPtr ipAddress = (frameIndex < realFrameCount) ? frameArray[frameIndex] : IntPtr.Zero; + ipAddress = (frameIndex < realFrameCount) ? frameArray[frameIndex] : IntPtr.Zero; + } + else + { + ipAddress = IntPtr.Zero; + } InitializeForIpAddress(ipAddress, needFileInfo); } diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackTrace.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackTrace.NativeAot.cs index 32db59daefe2c2..f4c015efe34aa1 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackTrace.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackTrace.NativeAot.cs @@ -80,18 +80,19 @@ private void InitializeForIpAddressArray(IntPtr[] ipAddresses, int skipFrames, i #if !TARGET_WASM internal void ToString(TraceFormat traceFormat, StringBuilder builder) { - if (_stackFrames == null) + if (_stackFrames != null) { - return; - } - - foreach (StackFrame frame in _stackFrames) - { - frame.AppendToStackTrace(builder); + foreach (StackFrame frame in _stackFrames) + { + frame?.AppendToStackTrace(builder); + } } if (traceFormat == TraceFormat.Normal && builder.Length >= Environment.NewLine.Length) builder.Length -= Environment.NewLine.Length; + + if (traceFormat == TraceFormat.TrailingNewLine && builder.Length == 0) + builder.AppendLine(); } #endif } diff --git a/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs b/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs index 5e84594fc62975..7ee32a0c420057 100644 --- a/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs +++ b/src/coreclr/nativeaot/System.Private.StackTraceMetadata/src/Internal/StackTraceMetadata/MethodNameFormatter.cs @@ -403,9 +403,10 @@ private void EmitPointerTypeName(PointerSignatureHandle pointerSigHandle) /// /// Emit function pointer type. /// - private void EmitFunctionPointerTypeName() + private static void EmitFunctionPointerTypeName() { - _outputBuilder.Append("IntPtr"); + // Function pointer types have no textual representation and we have tests making sure + // they show up as empty strings in stack traces, so deliberately do nothing. } /// diff --git a/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs b/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs index c76a1ee94d9759..0bc3f5f124210c 100644 --- a/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs +++ b/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs @@ -172,6 +172,7 @@ private static bool GetLinqExpressionsBuiltWithIsInterpretingOnly() public static bool IsAsyncFileIOSupported => !IsBrowser && !IsWasi; public static bool IsLineNumbersSupported => !IsNativeAot; + public static bool IsILOffsetsSupported => !IsNativeAot; public static bool IsInContainer => GetIsInContainer(); public static bool IsNotInContainer => !IsInContainer; diff --git a/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameExtensionsTests.cs b/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameExtensionsTests.cs index 3278cf3f5fd39d..217c0c27f7b462 100644 --- a/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameExtensionsTests.cs +++ b/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameExtensionsTests.cs @@ -16,21 +16,21 @@ public static IEnumerable StackFrame_TestData() yield return new object[] { new StackFrame(int.MaxValue) }; } - [Theory] + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotNativeAot))] [MemberData(nameof(StackFrame_TestData))] public void HasNativeImage_StackFrame_ReturnsFalse(StackFrame stackFrame) { Assert.False(stackFrame.HasNativeImage()); } - [Theory] + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotNativeAot))] [MemberData(nameof(StackFrame_TestData))] public void GetNativeIP_StackFrame_ReturnsZero(StackFrame stackFrame) { Assert.Equal(IntPtr.Zero, stackFrame.GetNativeIP()); } - [Theory] + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotNativeAot))] [MemberData(nameof(StackFrame_TestData))] public void GetNativeImageBase_StackFrame_ReturnsZero(StackFrame stackFrame) { @@ -43,7 +43,7 @@ public static IEnumerable HasMethod_TestData() yield return new object[] { new StackFrame(int.MaxValue), false }; } - [Theory] + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsILOffsetsSupported))] [ActiveIssue("https://github.com/dotnet/runtime/issues/50957", typeof(PlatformDetection), nameof(PlatformDetection.IsBrowser), nameof(PlatformDetection.IsMonoAOT))] [MemberData(nameof(HasMethod_TestData))] public void HasILOffset_Invoke_ReturnsExpected(StackFrame stackFrame, bool expected) diff --git a/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs b/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs index 67ba8fd483eb12..6a8a9ec02e8da6 100644 --- a/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs +++ b/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs @@ -123,6 +123,7 @@ public static IEnumerable ToString_TestData() [Theory] [ActiveIssue("https://github.com/mono/mono/issues/15186", TestRuntimes.Mono)] + [ActiveIssue("TODOTODOTODO", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot))] [MemberData(nameof(ToString_TestData))] public void ToString_Invoke_ReturnsExpected(StackFrame stackFrame, string expectedToString) { @@ -168,7 +169,10 @@ private static void VerifyStackFrameSkipFrames(StackFrame stackFrame, bool isFil } else { - Assert.True(stackFrame.GetILOffset() >= 0, $"Expected GetILOffset() {stackFrame.GetILOffset()} for {stackFrame} to be greater or equal to zero."); + if (PlatformDetection.IsILOffsetsSupported) + { + Assert.True(stackFrame.GetILOffset() >= 0, $"Expected GetILOffset() {stackFrame.GetILOffset()} for {stackFrame} to be greater or equal to zero."); + } } // GetMethod returns null for unknown frames. diff --git a/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceSymbolsTests.cs b/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceSymbolsTests.cs index a095dc87007c08..b0470c1c8a5bd8 100644 --- a/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceSymbolsTests.cs +++ b/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceSymbolsTests.cs @@ -9,7 +9,7 @@ namespace System.Diagnostics.SymbolStore.Tests { public class StackTraceSymbolsTests { - [Fact] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.HasAssemblyFiles))] [ActiveIssue("https://github.com/dotnet/runtime/issues/51399", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)] public void StackTraceSymbolsDoNotLockFile() { diff --git a/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs b/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs index 93bdf3840bbd1f..1b2c9fb7274efd 100644 --- a/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs +++ b/src/libraries/System.Diagnostics.StackTrace/tests/StackTraceTests.cs @@ -47,7 +47,7 @@ public void MethodsToSkip_Get_ReturnsZero() public void Ctor_Default() { var stackTrace = new StackTrace(); - VerifyFrames(stackTrace, false); + VerifyFrames(stackTrace, false, 0); } [Theory] @@ -57,7 +57,7 @@ public void Ctor_Default() public void Ctor_FNeedFileInfo(bool fNeedFileInfo) { var stackTrace = new StackTrace(fNeedFileInfo); - VerifyFrames(stackTrace, fNeedFileInfo); + VerifyFrames(stackTrace, fNeedFileInfo, 0); } [Theory] @@ -73,7 +73,7 @@ public void Ctor_SkipFrames(int skipFrames) Assert.Equal(emptyStackTrace.FrameCount - skipFrames, stackTrace.FrameCount); Assert.Equal(expectedMethods, stackTrace.GetFrames().Select(f => f.GetMethod())); - VerifyFrames(stackTrace, false); + VerifyFrames(stackTrace, false, skipFrames); } [Fact] @@ -99,7 +99,7 @@ public void Ctor_SkipFrames_FNeedFileInfo(int skipFrames, bool fNeedFileInfo) Assert.Equal(emptyStackTrace.FrameCount - skipFrames, stackTrace.FrameCount); Assert.Equal(expectedMethods, stackTrace.GetFrames().Select(f => f.GetMethod())); - VerifyFrames(stackTrace, fNeedFileInfo); + VerifyFrames(stackTrace, fNeedFileInfo, skipFrames); } [Theory] @@ -117,7 +117,7 @@ public void Ctor_LargeSkipFramesFNeedFileInfo_GetFramesReturnsEmpty(bool fNeedFi public void Ctor_ThrownException_GetFramesReturnsExpected() { var stackTrace = new StackTrace(InvokeException()); - VerifyFrames(stackTrace, false); + VerifyFrames(stackTrace, false, 0); } [Fact] @@ -137,7 +137,7 @@ public void Ctor_EmptyException_GetFramesReturnsEmpty() public void Ctor_Bool_ThrownException_GetFramesReturnsExpected(bool fNeedFileInfo) { var stackTrace = new StackTrace(InvokeException(), fNeedFileInfo); - VerifyFrames(stackTrace, fNeedFileInfo); + VerifyFrames(stackTrace, fNeedFileInfo, 0); } [Theory] @@ -171,7 +171,7 @@ public void Ctor_Exception_SkipFrames(int skipFrames) Assert.Equal(expectedMethods, frames.Select(f => f.GetMethod())); if (frames != null) { - VerifyFrames(stackTrace, false); + VerifyFrames(stackTrace, false, skipFrames); } } @@ -211,7 +211,7 @@ public void Ctor_Exception_SkipFrames_FNeedFileInfo(int skipFrames, bool fNeedFi Assert.Equal(expectedMethods, frames.Select(f => f.GetMethod())); if (frames != null) { - VerifyFrames(stackTrace, fNeedFileInfo); + VerifyFrames(stackTrace, fNeedFileInfo, skipFrames); } } @@ -450,7 +450,7 @@ private class ClassWithConstructor public ClassWithConstructor() => StackTrace = new StackTrace(); } - private static void VerifyFrames(StackTrace stackTrace, bool hasFileInfo) + private static void VerifyFrames(StackTrace stackTrace, bool hasFileInfo, int skippedFrames) { Assert.True(stackTrace.FrameCount > 0); @@ -467,7 +467,11 @@ private static void VerifyFrames(StackTrace stackTrace, bool hasFileInfo) Assert.Equal(0, stackFrame.GetFileLineNumber()); Assert.Equal(0, stackFrame.GetFileColumnNumber()); } - Assert.NotNull(stackFrame.GetMethod()); + + // On native AOT, the reflection invoke infrastructure uses compiler-generated code + // that doesn't have a reflection method associated. Limit the checks. + if (!PlatformDetection.IsNativeAot || (i + skippedFrames) == 0) + Assert.NotNull(stackFrame.GetMethod()); } } } diff --git a/src/libraries/tests.proj b/src/libraries/tests.proj index b6b47e74094938..fd58beb466fc94 100644 --- a/src/libraries/tests.proj +++ b/src/libraries/tests.proj @@ -517,7 +517,6 @@ - From de699c9a0a29b701703be6b97faabdc277511ca7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 7 Jun 2024 11:22:35 +0200 Subject: [PATCH 2/3] Update src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs --- .../System.Diagnostics.StackTrace/tests/StackFrameTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs b/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs index 6a8a9ec02e8da6..c9d719eae3ce73 100644 --- a/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs +++ b/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs @@ -123,7 +123,7 @@ public static IEnumerable ToString_TestData() [Theory] [ActiveIssue("https://github.com/mono/mono/issues/15186", TestRuntimes.Mono)] - [ActiveIssue("TODOTODOTODO", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot))] + [ActiveIssue("https://github.com/dotnet/runtime/issues/103156", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot))] [MemberData(nameof(ToString_TestData))] public void ToString_Invoke_ReturnsExpected(StackFrame stackFrame, string expectedToString) { From 57cfca45ef689589e49290f194ba2815fea1f182 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 10 Jun 2024 06:32:23 +0200 Subject: [PATCH 3/3] FB --- .../Diagnostics/StackFrame.NativeAot.cs | 22 +++++-------------- .../tests/StackFrameExtensionsTests.cs | 2 ++ .../tests/StackFrameTests.cs | 1 + 3 files changed, 8 insertions(+), 17 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackFrame.NativeAot.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackFrame.NativeAot.cs index ee26e6b3114c70..f061cf1d461d71 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackFrame.NativeAot.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Diagnostics/StackFrame.NativeAot.cs @@ -110,26 +110,14 @@ private void InitializeForIpAddress(IntPtr ipAddress, bool needFileInfo) [MethodImplAttribute(MethodImplOptions.NoInlining)] private void BuildStackFrame(int frameIndex, bool needFileInfo) { - IntPtr ipAddress; - const int SystemDiagnosticsStackDepth = 2; - // Unreasonably high or negative frameIndex could lead to overflows or failures to allocate - // the frameArray below so spot check it's sane. - RuntimeImports.RhGetCurrentThreadStackBounds(out IntPtr stackLow, out IntPtr stackHigh); - if (frameIndex >= -SystemDiagnosticsStackDepth && frameIndex < ((stackHigh - stackLow) / IntPtr.Size)) - { - frameIndex += SystemDiagnosticsStackDepth; - IntPtr[] frameArray = new IntPtr[frameIndex + 1]; - int returnedFrameCount = RuntimeImports.RhGetCurrentThreadStackTrace(frameArray); - int realFrameCount = (returnedFrameCount >= 0 ? returnedFrameCount : frameArray.Length); + frameIndex += SystemDiagnosticsStackDepth; + IntPtr[] frameArray = new IntPtr[frameIndex + 1]; + int returnedFrameCount = RuntimeImports.RhGetCurrentThreadStackTrace(frameArray); + int realFrameCount = (returnedFrameCount >= 0 ? returnedFrameCount : frameArray.Length); - ipAddress = (frameIndex < realFrameCount) ? frameArray[frameIndex] : IntPtr.Zero; - } - else - { - ipAddress = IntPtr.Zero; - } + IntPtr ipAddress = (frameIndex < realFrameCount) ? frameArray[frameIndex] : IntPtr.Zero; InitializeForIpAddress(ipAddress, needFileInfo); } diff --git a/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameExtensionsTests.cs b/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameExtensionsTests.cs index 217c0c27f7b462..931fe55adce172 100644 --- a/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameExtensionsTests.cs +++ b/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameExtensionsTests.cs @@ -59,6 +59,7 @@ public void HasILOffset_NullStackFrame_ThrowsNullReferenceException() [Theory] [ActiveIssue("https://github.com/dotnet/runtime/issues/50957", typeof(PlatformDetection), nameof(PlatformDetection.IsBrowser), nameof(PlatformDetection.IsMonoAOT))] + [ActiveIssue("https://github.com/dotnet/runtime/issues/103218", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot))] [MemberData(nameof(HasMethod_TestData))] public void HasMethod_Invoke_ReturnsExpected(StackFrame stackFrame, bool expected) { @@ -79,6 +80,7 @@ public static IEnumerable HasSource_TestData() } [Theory] + [ActiveIssue("https://github.com/dotnet/runtime/issues/103218", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot))] [MemberData(nameof(HasSource_TestData))] public void HasSource_Invoke_ReturnsExpected(StackFrame stackFrame, bool expected) { diff --git a/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs b/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs index c9d719eae3ce73..b8f225f33d8cf0 100644 --- a/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs +++ b/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs @@ -75,6 +75,7 @@ public void SkipFrames_CallMethod_ReturnsExpected() [Theory] [InlineData(int.MinValue)] [InlineData(int.MaxValue)] + [ActiveIssue("https://github.com/dotnet/runtime/issues/103218", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot))] public void SkipFrames_ManyFrames_HasNoMethod(int skipFrames) { var stackFrame = new StackFrame(skipFrames);