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

Add System.Diagnostics.StackFrame.GetMethodFromNativeIP API for VS4Mac #61289

Merged
merged 6 commits into from
Nov 9, 2021
Merged

Add System.Diagnostics.StackFrame.GetMethodFromNativeIP API for VS4Mac #61289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6b7f5a0
Add System.Diagnostics.StackFrame.GetMethodInfoFromNativeIP API for V…
mikem8361 Nov 3, 2021
dbe2368
Code review feedback. Return MethodBase instead of MethodInfo. Rename…
mikem8361 Nov 7, 2021
9a14e5d
Fix Mono build break
mikem8361 Nov 7, 2021
b3a9a60
Better fix for Mono break
mikem8361 Nov 7, 2021
3aeaed6
Code review feedback
mikem8361 Nov 9, 2021
308f640
Remove unneccesary blank line
mikem8361 Nov 9, 2021
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
23 changes: 23 additions & 0 deletions src/coreclr/System.Private.CoreLib/src/System/Diagnostics/StackFrame.CoreCLR.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Text;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

namespace System.Diagnostics
{
Expand Down Expand Up @@ -50,5 +53,25 @@ private void BuildStackFrame(int skipFrames, bool needFileInfo)
}

private static bool AppendStackFrameWithoutMethodBase(StringBuilder sb) => false;

[DllImport(RuntimeHelpers.QCall, EntryPoint = "StackFrame_GetMethodDescFromNativeIP")]
private static extern RuntimeMethodHandleInternal GetMethodDescFromNativeIP(IntPtr ip);

/// <summary>
/// Returns the MethodBase instance for the managed code IP address.
///
/// Warning: The implementation of this method has race for dynamic and collectible methods.
/// </summary>
/// <param name="ip">code address</param>
/// <returns>MethodBase instance for the method or null if IP not found</returns>
internal static MethodBase? GetMethodFromNativeIP(IntPtr ip)
Copy link
Member

Choose a reason for hiding this comment

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

We should probably add a comment right here saying that the implementation of this method has known race conditions for unloadable code + MethodBase isn't a good design choice for working with NativeAOT, therefore don't make it public in the future without considering those issues. Reduces the chance we forget the history when we need a public API later.

Copy link
Member

Choose a reason for hiding this comment

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

Or even better - create issue that tracks adding the public API and link it from here

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 added a TODO describing most of this here: https://github.com/mikem8361/runtime/blob/b3a9a6086dcef769b82a5750489738254b6aee80/src/coreclr/vm/debugdebugger.cpp#L788

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that good enough?

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 did add a comment to the method's header.

Copy link
Member

Choose a reason for hiding this comment

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

I do think having the message that you added right here in the header is valuable. When people convert a private API to public nothing enforces that they will look at implementation code in a separate file. The comment you added calls out the race condition but doesn't mention the design issue for using MethodBase in NativeAOT. As @jkotas suggested filing an issue and linking to that is an improvement over my original suggestion.

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'm going to use issue #61186 to detail these limitations and to propose the new 7.0 API.

{
RuntimeMethodHandleInternal method = GetMethodDescFromNativeIP(ip);

if (method.Value == IntPtr.Zero)
return null;

return RuntimeType.GetMethodBase(null, method);
}
}
}
23 changes: 22 additions & 1 deletion src/coreclr/vm/debugdebugger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,6 @@ FCIMPL0(FC_BOOL_RET, DebugDebugger::IsLogging)
}
FCIMPLEND


FCIMPL4(void, DebugStackTrace::GetStackFramesInternal,
StackFrameHelper* pStackFrameHelperUNSAFE,
INT32 iSkip,
Expand Down Expand Up @@ -778,6 +777,28 @@ FCIMPL4(void, DebugStackTrace::GetStackFramesInternal,
}
FCIMPLEND

extern MethodDesc* QCALLTYPE StackFrame_GetMethodDescFromNativeIP(LPVOID ip)
{
QCALL_CONTRACT;

MethodDesc* pResult = nullptr;

BEGIN_QCALL;

// TODO: There is a race for dynamic and collectible methods here between getting
// the MethodDesc here and when the managed wrapper converts it into a MethodBase
// where the method could be collected.
EECodeInfo codeInfo((PCODE)ip);
if (codeInfo.IsValid())
{
pResult = codeInfo.GetMethodDesc();
mikem8361 marked this conversation as resolved.
Show resolved Hide resolved
}

END_QCALL;

return pResult;
}

FORCEINLINE void HolderDestroyStrongHandle(OBJECTHANDLE h) { if (h != NULL) DestroyStrongHandle(h); }
typedef Wrapper<OBJECTHANDLE, DoNothing<OBJECTHANDLE>, HolderDestroyStrongHandle, NULL> StrongHandleHolder;

Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/vm/debugdebugger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,4 +171,6 @@ class DebugStackTrace

};

extern "C" MethodDesc* QCALLTYPE StackFrame_GetMethodDescFromNativeIP(LPVOID ip);

#endif // __DEBUG_DEBUGGER_h__
1 change: 1 addition & 0 deletions src/coreclr/vm/qcallentrypoints.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ static const Entry s_QCall[] =
DllImportEntry(RuntimeModule_GetType)
DllImportEntry(RuntimeModule_GetScopeName)
DllImportEntry(RuntimeModule_GetFullyQualifiedName)
DllImportEntry(StackFrame_GetMethodDescFromNativeIP)
DllImportEntry(ModuleBuilder_GetStringConstant)
DllImportEntry(ModuleBuilder_GetTypeRef)
DllImportEntry(ModuleBuilder_GetTokenFromTypeSpec)
Expand Down
4 changes: 4 additions & 0 deletions src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Descriptors.LibraryBuild.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,9 @@
<!-- Internal API used by tests only. -->
<method name="GetICUVersion" />
</type>
<type fullname="System.Diagnostics.StackFrame">
<!-- Used by VS4Mac via reflection to symbolize stack traces -->
<method name="GetMethodFromNativeIP" />
</type>
</assembly>
</linker>