Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace Internal.Runtime.Augments
[CLSCompliant(false)]
public abstract class StackTraceMetadataCallbacks
{
public abstract string TryGetMethodStackFrameInfo(IntPtr methodStartAddress, int offset, bool needsFileInfo, out string owningType, out string genericArgs, out string methodSignature, out bool isStackTraceHidden, out string fileName, out int lineNumber);
public abstract string TryGetMethodStackFrameInfo(IntPtr methodStartAddress, int offset, bool needsFileInfo, out string owningType, out string genericArgs, out string methodSignature, out bool isStackTraceHidden, out bool isAsync, out string fileName, out int lineNumber);

public abstract DiagnosticMethodInfo TryGetDiagnosticMethodInfoFromStartAddress(IntPtr methodStartAddress);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,13 @@ public partial class StackFrame

private bool _isStackTraceHidden;

/// <summary>
/// Will be true if this frame corresponds to a runtime-async method or the MoveNext method of a
/// compiler-generated async state machine. Used to suppress the "--- End of stack trace from
/// previous location ---" delimiter.
/// </summary>
private bool _isAsync;

// If stack trace metadata is available, _methodOwningType is the namespace-qualified name of the owning type,
// _methodName is the name of the method, _methodGenericArgs are generic arguments, and _methodSignature is the list of parameters
// without braces. StackTrace will format this as `{_methodOwningType}.{_methodName}<{_genericArgs}>({_methodSignature}).
Expand Down Expand Up @@ -122,7 +129,7 @@ private void InitializeForIpAddress(IntPtr ipAddress, bool needFileInfo)
StackTraceMetadataCallbacks stackTraceCallbacks = RuntimeAugments.StackTraceCallbacksIfAvailable;
if (stackTraceCallbacks != null)
{
_methodName = stackTraceCallbacks.TryGetMethodStackFrameInfo(methodStartAddress, _nativeOffset, needFileInfo, out _methodOwningType, out _methodGenericArgs, out _methodSignature, out _isStackTraceHidden, out _fileName, out _lineNumber);
_methodName = stackTraceCallbacks.TryGetMethodStackFrameInfo(methodStartAddress, _nativeOffset, needFileInfo, out _methodOwningType, out _methodGenericArgs, out _methodSignature, out _isStackTraceHidden, out _isAsync, out _fileName, out _lineNumber);
}

if (_methodName == null)
Expand Down Expand Up @@ -230,7 +237,7 @@ internal void AppendToStackTrace(StringBuilder builder)
builder.AppendLine();
}
}
if (_isLastFrameFromForeignExceptionStackTrace)
if (_isLastFrameFromForeignExceptionStackTrace && !_isAsync)
{
// Passing default for Exception_EndStackTraceFromPreviousThrow in case SR.UsingResourceKeys is set.
builder.AppendLine(SR.UsingResourceKeys() ?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ internal static void Initialize()
/// <summary>
/// Locate the containing module for a method and try to resolve its name based on start address.
/// </summary>
public static unsafe string GetMethodNameFromStartAddressIfAvailable(IntPtr methodStartAddress, out string owningTypeName, out string genericArgs, out string methodSignature, out bool isStackTraceHidden, out int hashCodeForLineInfo)
public static unsafe string GetMethodNameFromStartAddressIfAvailable(IntPtr methodStartAddress, out string owningTypeName, out string genericArgs, out string methodSignature, out bool isStackTraceHidden, out bool isAsync, out int hashCodeForLineInfo)
{
IntPtr moduleStartAddress = RuntimeAugments.GetOSModuleFromPointer(methodStartAddress);
int rva = (int)((byte*)methodStartAddress - (byte*)moduleStartAddress);
Expand All @@ -62,6 +62,7 @@ public static unsafe string GetMethodNameFromStartAddressIfAvailable(IntPtr meth
if (resolver.TryGetStackTraceData(rva, out var data))
{
isStackTraceHidden = data.IsHidden;
isAsync = data.IsAsync;
if (data.OwningType.IsNil)
{
Debug.Assert(data.Name.IsNil && data.Signature.IsNil);
Expand All @@ -80,6 +81,7 @@ public static unsafe string GetMethodNameFromStartAddressIfAvailable(IntPtr meth
}

isStackTraceHidden = false;
isAsync = false;

// We haven't found information in the stack trace metadata tables, but maybe reflection will have this
if (ReflectionExecution.TryGetMethodMetadataFromStartAddress(methodStartAddress,
Expand Down Expand Up @@ -331,9 +333,9 @@ public override DiagnosticMethodInfo TryGetDiagnosticMethodInfoFromStartAddress(
return GetDiagnosticMethodInfoFromStartAddressIfAvailable(methodStartAddress);
}

public override string TryGetMethodStackFrameInfo(IntPtr methodStartAddress, int offset, bool needsFileInfo, out string owningType, out string genericArgs, out string methodSignature, out bool isStackTraceHidden, out string fileName, out int lineNumber)
public override string TryGetMethodStackFrameInfo(IntPtr methodStartAddress, int offset, bool needsFileInfo, out string owningType, out string genericArgs, out string methodSignature, out bool isStackTraceHidden, out bool isAsync, out string fileName, out int lineNumber)
{
string methodName = GetMethodNameFromStartAddressIfAvailable(methodStartAddress, out owningType, out genericArgs, out methodSignature, out isStackTraceHidden, out int hashCode);
string methodName = GetMethodNameFromStartAddressIfAvailable(methodStartAddress, out owningType, out genericArgs, out methodSignature, out isStackTraceHidden, out isAsync, out int hashCode);

if (needsFileInfo)
{
Expand Down Expand Up @@ -474,6 +476,7 @@ private unsafe void PopulateRvaToTokenMap(TypeManagerHandle handle, byte* pMap,
{
Rva = methodRva,
IsHidden = (command & StackTraceDataCommand.IsStackTraceHidden) != 0,
IsAsync = (command & StackTraceDataCommand.IsAsync) != 0,
OwningType = currentOwningType,
Name = currentName,
Signature = currentSignature,
Expand Down Expand Up @@ -516,25 +519,36 @@ public bool TryGetStackTraceData(int rva, out StackTraceData data)
public struct StackTraceData : IComparable<StackTraceData>
{
private const int IsHiddenFlag = 0x2;
private const int IsAsyncFlag = 0x1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bit 0 collides with the ARM32 Thumb bit. The original code intentionally used IsHiddenFlag = 0x2 (bit 1) and reserved bit 0 as part of the RVA. On ARM32, method entrypoint symbols are defined with the Thumb bit set (+1) per the AAELF ABI — see ObjectWriter.EmitObjectFile where thumbBit = _nodeFactory.Target.Architecture == TargetArchitecture.ARM && isMethod ? 1 : 0. The StackTraceMethodMappingNode emits IMAGE_REL_BASED_RELPTR32 relocs against factory.MethodEntrypoint(...), so the decoded methodRva in PopulateRvaToTokenMap will have bit 0 set on ARM32.

Repurposing bit 0 as IsAsyncFlag breaks this on ARM32:

  • The Rva { init } Debug.Assert((value & FlagsMask) == 0) will fire in debug/checked builds because methodRva is odd.
  • IsAsync would be spuriously true for essentially every ARM32 method (bit 0 is always set).
  • Rva now masks off bit 0 on the stored side, so if the runtime lookup RVA still carries the Thumb bit, the Array.BinarySearch in TryGetStackTraceData mismatches and stack-trace metadata (method names, hidden/async flags) is lost.

Use a spare high bit instead of bit 0. StackTraceDataCommand.IsAsync = 0x20 is a distinct value in the emitted command byte and is unrelated to this packed-RVA field, so pick a flag bit here that does not overlap the RVA's low bit — e.g. keep the async flag in a bit that ARM32 RVAs never use, or store the flags outside the RVA word entirely.

private const int FlagsMask = IsHiddenFlag | IsAsyncFlag;
Comment on lines 521 to +523

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This concern is legitimate, I commented on this in Tom's PR.


private readonly int _rvaAndIsHiddenBit;
private readonly int _rvaAndFlags;

public int Rva
{
get => _rvaAndIsHiddenBit & ~IsHiddenFlag;
get => _rvaAndFlags & ~FlagsMask;
init
{
Debug.Assert((value & IsHiddenFlag) == 0);
_rvaAndIsHiddenBit = value | (_rvaAndIsHiddenBit & IsHiddenFlag);
Debug.Assert((value & FlagsMask) == 0);
_rvaAndFlags = value | (_rvaAndFlags & FlagsMask);
}
}
public bool IsHidden
{
get => (_rvaAndIsHiddenBit & IsHiddenFlag) != 0;
get => (_rvaAndFlags & IsHiddenFlag) != 0;
init
{
if (value)
_rvaAndIsHiddenBit |= IsHiddenFlag;
_rvaAndFlags |= IsHiddenFlag;
}
}
public bool IsAsync
{
get => (_rvaAndFlags & IsAsyncFlag) != 0;
init
{
if (value)
_rvaAndFlags |= IsAsyncFlag;
}
}
public Handle OwningType { get; init; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ internal static class StackTraceDataCommand
public const byte UpdateGenericSignature = 0x08; // Just a shortcut - sig metadata has the info

public const byte IsStackTraceHidden = 0x10;
public const byte IsAsync = 0x20;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)
command |= StackTraceDataCommand.IsStackTraceHidden;
}

if ((entry.Flags & StackTraceRecordFlags.IsAsync) != 0)
{
command |= StackTraceDataCommand.IsAsync;
}

objData.EmitByte(commandReservation, command);
objData.EmitReloc(factory.MethodEntrypoint(entry.Method), RelocType.IMAGE_REL_BASED_RELPTR32);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -780,6 +780,8 @@ protected void ComputeMetadata<TPolicy>(
flags |= StackTraceRecordFlags.IsHidden;
if ((stackVisibility & MethodStackTraceVisibilityFlags.HasLineNumbers) != 0)
flags |= StackTraceRecordFlags.HasLineNumbers;
if ((stackVisibility & MethodStackTraceVisibilityFlags.IsAsync) != 0)
flags |= StackTraceRecordFlags.IsAsync;

if ((stackVisibility & MethodStackTraceVisibilityFlags.HasMetadata) != 0)
{
Expand Down Expand Up @@ -1351,6 +1353,7 @@ public enum StackTraceRecordFlags
None = 0,
IsHidden = 1,
HasLineNumbers = 2,
IsAsync = 4,
}

public readonly struct StackTraceRecordData
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ public override MethodStackTraceVisibilityFlags GetMethodVisibility(MethodDesc m
public class EcmaMethodStackTraceEmissionPolicy : StackTraceEmissionPolicy
{
private readonly MethodStackTraceVisibilityFlags _flags;
private MetadataType _iAsyncStateMachineType;
private bool _iAsyncStateMachineTypeComputed;

public EcmaMethodStackTraceEmissionPolicy(bool includeLineNumbers)
{
Expand All @@ -46,10 +48,54 @@ public override MethodStackTraceVisibilityFlags GetMethodVisibility(MethodDesc m
result |= MethodStackTraceVisibilityFlags.IsHidden;
}

if (IsAsyncFrame(method))
{
result |= MethodStackTraceVisibilityFlags.IsAsync;
}

return (method.GetTypicalMethodDefinition() is Internal.TypeSystem.Ecma.EcmaMethod || (method.IsAsync && method.IsAsyncCall()))
? result | MethodStackTraceVisibilityFlags.HasMetadata
: result;
}

// Determines whether a frame for this method should be treated as "async" when formatting a
// stack trace. Async frames suppress the "--- End of stack trace from previous location ---"
// delimiter. This covers both runtime-async (V2) methods and the MoveNext method of a
// compiler-generated (V1) async state machine.
private bool IsAsyncFrame(MethodDesc method)
{
if (method.IsAsync)
{
return true;
}

// Async state machines only expose their exception-throwing code through IAsyncStateMachine.MoveNext.
if (method.Name != "MoveNext"u8)
{
return false;
}

if (!_iAsyncStateMachineTypeComputed)
{
_iAsyncStateMachineType = method.Context.SystemModule.GetType("System.Runtime.CompilerServices"u8, "IAsyncStateMachine"u8, throwIfNotFound: false);
_iAsyncStateMachineTypeComputed = true;
}

if (_iAsyncStateMachineType == null)
{
return false;
}

foreach (DefType interfaceType in method.OwningType.RuntimeInterfaces)
{
if (interfaceType == _iAsyncStateMachineType)
{
return true;
}
}
Comment on lines +72 to +95

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this meaningfully improve the stack trace experience in async v1? The EDI separator being present falls out from all of this reflection based code that we can't use with native AOT:

#if !NATIVEAOT
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "ToString is best effort when it comes to available information.")]
internal void ToString(TraceFormat traceFormat, StringBuilder sb)
{
// Passing a default string for "at" in case SR.UsingResourceKeys() is true
// as this is a special case and we don't want to have "Word_At" on stack traces.
string word_At = SR.UsingResourceKeys() ? "at" : SR.Word_At;
// We also want to pass in a default for inFileLineNumber.
string inFileLineNum = SR.UsingResourceKeys() ? "in {0}:line {1}" : SR.StackTrace_InFileLineNumber;
string inFileILOffset = SR.UsingResourceKeys() ? "in {0}:token 0x{1:x}+0x{2:x}" : SR.StackTrace_InFileILOffset;
bool fFirstFrame = true;
for (int iFrameIndex = 0; iFrameIndex < _numOfFrames; iFrameIndex++)
{
StackFrame? sf = GetFrame(iFrameIndex);
MethodBase? mb = sf?.GetMethod();
if (mb != null && (ShowInStackTrace(mb) ||
(iFrameIndex == _numOfFrames - 1))) // Don't filter last frame
{
// We want a newline at the end of every line except for the last
if (fFirstFrame)
fFirstFrame = false;
else
sb.AppendLine();
sb.Append(" ").Append(word_At).Append(' ');
bool isAsync = (mb.MethodImplementationFlags & MethodImplAttributes.Async) != 0;
Type? declaringType = mb.DeclaringType;
string methodName = mb.Name;
bool methodChanged = false;
if (!isAsync && declaringType != null && IsDefinedSafe(declaringType, typeof(CompilerGeneratedAttribute), inherit: false))
{
isAsync = declaringType.IsAssignableTo(typeof(IAsyncStateMachine));
if (isAsync || declaringType.IsAssignableTo(typeof(IEnumerator)))
{
methodChanged = TryResolveStateMachineMethod(ref mb, out declaringType);
}
}
// if there is a type (non global method) print it
// ResolveStateMachineMethod may have set declaringType to null
if (declaringType != null)
{
// Append t.FullName, replacing '+' with '.'
string fullName = declaringType.FullName!;
for (int i = 0; i < fullName.Length; i++)
{
char ch = fullName[i];
sb.Append(ch == '+' ? '.' : ch);
}
sb.Append('.');
}
sb.Append(mb.Name);
// deal with the generic portion of the method
if (mb is MethodInfo mi && mi.IsGenericMethod)
{
Type[] typars = mi.GetGenericArguments();
sb.Append('[');
int k = 0;
bool fFirstTyParam = true;
while (k < typars.Length)
{
if (!fFirstTyParam)
sb.Append(',');
else
fFirstTyParam = false;
sb.Append(typars[k].Name);
k++;
}
sb.Append(']');
}
ReadOnlySpan<ParameterInfo> pi = default;
bool appendParameters = true;
try
{
pi = mb.GetParametersAsSpan();
}
catch
{
// The parameter info cannot be loaded, so we don't
// append the parameter list.
appendParameters = false;
}
if (appendParameters)
{
// arguments printing
sb.Append('(');
bool fFirstParam = true;
for (int j = 0; j < pi.Length; j++)
{
if (!fFirstParam)
sb.Append(", ");
else
fFirstParam = false;
string typeName = "<UnknownType>";
if (pi[j].ParameterType != null)
typeName = pi[j].ParameterType.Name;
sb.Append(typeName);
string? parameterName = pi[j].Name;
if (parameterName != null)
{
sb.Append(' ');
sb.Append(parameterName);
}
}
sb.Append(')');
}
if (methodChanged)
{
// Append original method name e.g. +MoveNext()
sb.Append('+');
sb.Append(methodName);
sb.Append('(').Append(')');
}
// source location printing
if (sf!.GetILOffset() != -1)
{
// If we don't have a PDB or PDB-reading is disabled for the module,
// then the file name will be null.
string? fileName = sf.GetFileName();
if (fileName != null)
{
// tack on " in c:\tmp\MyFile.cs:line 5"
sb.Append(' ');
sb.AppendFormat(CultureInfo.InvariantCulture, inFileLineNum, fileName, sf.GetFileLineNumber());
}
else if (LocalAppContextSwitches.ShowILOffsets && mb.ReflectedType != null)
{
string assemblyName = mb.ReflectedType.Module.ScopeName;
try
{
int token = mb.MetadataToken;
sb.Append(' ');
sb.AppendFormat(CultureInfo.InvariantCulture, inFileILOffset, assemblyName, token, sf.GetILOffset());
}
catch (InvalidOperationException) { }
}
}
// Skip EDI boundary for async
if (sf.IsLastFrameFromForeignExceptionStackTrace && !isAsync)
{
sb.AppendLine();
// Passing default for Exception_EndStackTraceFromPreviousThrow in case SR.UsingResourceKeys is set.
sb.Append(SR.UsingResourceKeys() ? "--- End of stack trace from previous location ---" : SR.Exception_EndStackTraceFromPreviousThrow);
}
}
}
if (traceFormat == TraceFormat.TrailingNewLine)
sb.AppendLine();
}
#endif // !NATIVEAOT

It is likely that when/if we decide we want to fix asyncv1 stack traces, the EDI separators disappearing will also naturally fall out from all the extra metadata we'll need to emit and all of this special casing will be reverted (or... if we let copilot do it, the special casing will probably stay because copilot is usually not capable of identifying redundant code).

I'd delete this code.


return false;
}
}

[Flags]
Expand All @@ -58,5 +104,6 @@ public enum MethodStackTraceVisibilityFlags
HasMetadata = 0x1,
IsHidden = 0x2,
HasLineNumbers = 0x4,
IsAsync = 0x8,
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -709,11 +709,7 @@ public async Task ToString_Async(Func<Task> asyncMethod, string[] expectedPatter
startIndex = match.Index + match.Length;
}

// [ActiveIssue("https://github.com/dotnet/runtime/issues/129155", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot))]
if (!PlatformDetection.IsNativeAot)
{
Assert.DoesNotContain("--- End of stack trace from previous location ---", exceptionText);
}
Assert.DoesNotContain("--- End of stack trace from previous location ---", exceptionText);
}

[MethodImpl(MethodImplOptions.NoOptimization | MethodImplOptions.NoInlining)]
Expand Down
Loading