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

Fix duplicate transition for reverse delegates and expand our profiler tests for transitions to cover this case. #69761

Merged
merged 1 commit into from
May 25, 2022
Merged

Fix duplicate transition for reverse delegates and expand our profiler tests for transitions to cover this case. #69761

Show file tree
Hide file tree
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
27 changes: 5 additions & 22 deletions src/coreclr/vm/dllimport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,7 @@ class ILStubState : public StubState
DWORD dwMethodDescLocalNum = (DWORD)-1;

// Notify the profiler of call out of the runtime
if (!SF_IsReverseCOMStub(m_dwStubFlags) && !SF_IsStructMarshalStub(m_dwStubFlags) && CORProfilerTrackTransitions())
if (!SF_IsReverseCOMStub(m_dwStubFlags) && !SF_IsReverseDelegateStub(m_dwStubFlags) && !SF_IsStructMarshalStub(m_dwStubFlags) && CORProfilerTrackTransitions())
{
dwMethodDescLocalNum = m_slIL.EmitProfilerBeginTransitionCallback(pcsDispatch, m_dwStubFlags);
_ASSERTE(dwMethodDescLocalNum != (DWORD)-1);
Expand Down Expand Up @@ -2251,18 +2251,8 @@ DWORD NDirectStubLinker::EmitProfilerBeginTransitionCallback(ILCodeStream* pcsEm
// in StubHelpers::ProfilerEnterCallback().
if (SF_IsDelegateStub(dwStubFlags))
{
if (SF_IsForwardStub(dwStubFlags))
{
pcsEmit->EmitLoadThis();
}
else
{
EmitLoadStubContext(pcsEmit, dwStubFlags); // load UMEntryThunk*
pcsEmit->EmitLDC(offsetof(UMEntryThunk, m_pObjectHandle));
pcsEmit->EmitADD();
pcsEmit->EmitLDIND_I(); // get OBJECTHANDLE
pcsEmit->EmitLDIND_REF(); // get Delegate object
}
_ASSERTE(SF_IsForwardStub(dwStubFlags));
pcsEmit->EmitLoadThis();
}
else
{
Expand All @@ -2282,15 +2272,8 @@ void NDirectStubLinker::EmitProfilerEndTransitionCallback(ILCodeStream* pcsEmit,
STANDARD_VM_CONTRACT;

pcsEmit->EmitLDLOC(dwMethodDescLocalNum);
if (SF_IsReverseStub(dwStubFlags))
{
// we use a null pThread to indicate reverse interop
pcsEmit->EmitLoadNullPtr();
}
else
{
pcsEmit->EmitLDLOC(GetThreadLocalNum());
}
_ASSERTE(SF_IsForwardStub(dwStubFlags));
pcsEmit->EmitLDLOC(GetThreadLocalNum());
pcsEmit->EmitCALL(METHOD__STUBHELPERS__PROFILER_END_TRANSITION_CALLBACK, 2, 0);
}
#endif // PROFILING_SUPPPORTED
Expand Down
38 changes: 2 additions & 36 deletions src/coreclr/vm/stubhelpers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,17 +569,6 @@ FCIMPL3(SIZE_T, StubHelpers::ProfilerBeginTransitionCallback, SIZE_T pSecretPara
DELEGATEREF dref = (DELEGATEREF)ObjectToOBJECTREF(unsafe_pThis);
HELPER_METHOD_FRAME_BEGIN_RET_1(dref);

bool fReverseInterop = false;

if (NULL == pThread)
{
// This is our signal for the reverse interop cases.
fReverseInterop = true;
pThread = GET_THREAD();
// the secret param in this casee is the UMEntryThunk
pRealMD = ((UMEntryThunk*)pSecretParam)->GetMethod();
}
else
if (pSecretParam == 0)
{
// Secret param is null. This is the calli pinvoke case or the unmanaged delegate case.
Expand Down Expand Up @@ -611,14 +600,7 @@ FCIMPL3(SIZE_T, StubHelpers::ProfilerBeginTransitionCallback, SIZE_T pSecretPara
{
GCX_PREEMP_THREAD_EXISTS(pThread);

if (fReverseInterop)
{
ProfilerUnmanagedToManagedTransitionMD(pRealMD, COR_PRF_TRANSITION_CALL);
}
else
{
ProfilerManagedToUnmanagedTransitionMD(pRealMD, COR_PRF_TRANSITION_CALL);
}
ProfilerManagedToUnmanagedTransitionMD(pRealMD, COR_PRF_TRANSITION_CALL);
}

HELPER_METHOD_FRAME_END();
Expand Down Expand Up @@ -646,25 +628,9 @@ FCIMPL2(void, StubHelpers::ProfilerEndTransitionCallback, MethodDesc* pRealMD, T
// and the transition requires us to set up a HMF.
HELPER_METHOD_FRAME_BEGIN_0();
{
bool fReverseInterop = false;

if (NULL == pThread)
{
// if pThread is null, we are doing reverse interop
pThread = GET_THREAD();
fReverseInterop = true;
}

GCX_PREEMP_THREAD_EXISTS(pThread);

if (fReverseInterop)
{
ProfilerManagedToUnmanagedTransitionMD(pRealMD, COR_PRF_TRANSITION_RETURN);
}
else
{
ProfilerUnmanagedToManagedTransitionMD(pRealMD, COR_PRF_TRANSITION_RETURN);
}
ProfilerUnmanagedToManagedTransitionMD(pRealMD, COR_PRF_TRANSITION_RETURN);
}
HELPER_METHOD_FRAME_END();

Expand Down
29 changes: 27 additions & 2 deletions src/tests/profiler/native/transitions/transitions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,20 @@ HRESULT Transitions::Initialize(IUnknown* pICorProfilerInfoUnk)
return hr;
}

constexpr ULONG bufferSize = 1024;
ULONG envVarLen = 0;
WCHAR envVar[bufferSize];
if (FAILED(hr = pCorProfilerInfo->GetEnvironmentVariable(WCHAR("PInvoke_Transition_Expected_Name"), bufferSize, &envVarLen, envVar)))
{
return E_FAIL;
}
expectedPinvokeName = envVar;
if (FAILED(hr = pCorProfilerInfo->GetEnvironmentVariable(WCHAR("ReversePInvoke_Transition_Expected_Name"), bufferSize, &envVarLen, envVar)))
{
return E_FAIL;
}
expectedReversePInvokeName = envVar;

return S_OK;
}

Expand Down Expand Up @@ -55,13 +69,19 @@ extern "C" EXPORT void STDMETHODCALLTYPE DoPInvoke(int(*callback)(int), int i)
printf("PInvoke received i=%d\n", callback(i));
}


HRESULT Transitions::UnmanagedToManagedTransition(FunctionID functionID, COR_PRF_TRANSITION_REASON reason)
{
SHUTDOWNGUARD();

TransitionInstance* inst;
if (FunctionIsTargetFunction(functionID, &inst))
{
if (inst->UnmanagedToManaged != NO_TRANSITION)
{
// Report a failure for duplicate transitions.
_failures++;
}
inst->UnmanagedToManaged = reason;
}

Expand All @@ -75,6 +95,11 @@ HRESULT Transitions::ManagedToUnmanagedTransition(FunctionID functionID, COR_PRF
TransitionInstance* inst;
if (FunctionIsTargetFunction(functionID, &inst))
{
if (inst->ManagedToUnmanaged != NO_TRANSITION)
{
// Report a failure for duplicate transitions.
_failures++;
}
inst->ManagedToUnmanaged = reason;
}

Expand All @@ -85,11 +110,11 @@ bool Transitions::FunctionIsTargetFunction(FunctionID functionID, TransitionInst
{
String name = GetFunctionIDName(functionID);

if (name == WCHAR("DoPInvoke"))
if (name == expectedPinvokeName)
{
*inst = &_pinvoke;
}
else if (name == WCHAR("DoReversePInvoke"))
else if (name == expectedReversePInvokeName)
{
*inst = &_reversePinvoke;
}
Expand Down
8 changes: 6 additions & 2 deletions src/tests/profiler/native/transitions/transitions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

#include "../profiler.h"

#define NO_TRANSITION ((COR_PRF_TRANSITION_REASON)-1)

class Transitions : public Profiler
{
public:
Expand All @@ -23,8 +25,8 @@ class Transitions : public Profiler
struct TransitionInstance
{
TransitionInstance()
: UnmanagedToManaged{ (COR_PRF_TRANSITION_REASON)-1 }
, ManagedToUnmanaged{ (COR_PRF_TRANSITION_REASON)-1 }
: UnmanagedToManaged{ NO_TRANSITION }
, ManagedToUnmanaged{ NO_TRANSITION }
{ }

COR_PRF_TRANSITION_REASON UnmanagedToManaged;
Expand All @@ -33,6 +35,8 @@ class Transitions : public Profiler

TransitionInstance _pinvoke;
TransitionInstance _reversePinvoke;
String expectedPinvokeName;
String expectedReversePInvokeName;

bool FunctionIsTargetFunction(FunctionID functionID, TransitionInstance** inst);
};
108 changes: 103 additions & 5 deletions src/tests/profiler/transitions/transitions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,53 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Runtime.InteropServices;

namespace Profiler.Tests
{
unsafe class Transitions
{
static readonly string PInvokeExpectedNameEnvVar = "PInvoke_Transition_Expected_Name";
static readonly string ReversePInvokeExpectedNameEnvVar = "ReversePInvoke_Transition_Expected_Name";
static readonly Guid TransitionsGuid = new Guid("027AD7BB-578E-4921-B29F-B540363D83EC");

[UnmanagedFunctionPointer(CallingConvention.Winapi)]
delegate int InteropDelegate(int i);

[UnmanagedFunctionPointer(CallingConvention.Winapi)]
delegate int InteropDelegateNonBlittable(bool b);

private static int DoDelegateReversePInvoke(int i)
{
return i;
}

private static int DoDelegateReversePInvokeNonBlittable(bool b)
{
return b ? 1 : 0;
}

public static int BlittablePInvokeToBlittableInteropDelegate()
{
InteropDelegate del = DoDelegateReversePInvoke;

DoPInvoke((delegate* unmanaged<int,int>)Marshal.GetFunctionPointerForDelegate(del), 13);
GC.KeepAlive(del);

return 100;
}

public static int NonBlittablePInvokeToNonBlittableInteropDelegate()
{
InteropDelegateNonBlittable del = DoDelegateReversePInvokeNonBlittable;

DoPInvokeNonBlitable((delegate* unmanaged<int,int>)Marshal.GetFunctionPointerForDelegate(del), true);
GC.KeepAlive(del);

return 100;
}

[UnmanagedCallersOnly]
private static int DoReversePInvoke(int i)
{
Expand All @@ -19,23 +58,82 @@ private static int DoReversePInvoke(int i)
[DllImport("Profiler")]
public static extern void DoPInvoke(delegate* unmanaged<int,int> callback, int i);

public static int RunTest(String[] args)
[DllImport("Profiler", EntryPoint = nameof(DoPInvoke))]
public static extern void DoPInvokeNonBlitable(delegate* unmanaged<int,int> callback, bool i);

public static int BlittablePInvokeToUnmanagedCallersOnly()
{
DoPInvoke(&DoReversePInvoke, 13);

return 100;
}

public static int NonBlittablePInvokeToUnmanagedCallersOnly()
{
DoPInvokeNonBlitable(&DoReversePInvoke, true);

return 100;
}

public static int Main(string[] args)
{
if (args.Length > 0 && args[0].Equals("RunTest", StringComparison.OrdinalIgnoreCase))
if (args.Length > 1 && args[0].Equals("RunTest", StringComparison.OrdinalIgnoreCase))
{
switch (args[1])
{
case nameof(BlittablePInvokeToUnmanagedCallersOnly):
return BlittablePInvokeToUnmanagedCallersOnly();
case nameof(BlittablePInvokeToBlittableInteropDelegate):
return BlittablePInvokeToBlittableInteropDelegate();
case nameof(NonBlittablePInvokeToUnmanagedCallersOnly):
return NonBlittablePInvokeToUnmanagedCallersOnly();
case nameof(NonBlittablePInvokeToNonBlittableInteropDelegate):
return NonBlittablePInvokeToNonBlittableInteropDelegate();
}
}

if (!RunProfilerTest(nameof(BlittablePInvokeToUnmanagedCallersOnly), nameof(DoPInvoke), nameof(DoReversePInvoke)))
{
return 101;
}

if (!RunProfilerTest(nameof(BlittablePInvokeToBlittableInteropDelegate), nameof(DoPInvoke), "Invoke"))
{
return 102;
}

if (!RunProfilerTest(nameof(NonBlittablePInvokeToUnmanagedCallersOnly), nameof(DoPInvokeNonBlitable), nameof(DoReversePInvoke)))
{
return RunTest(args);
return 101;
}

return ProfilerTestRunner.Run(profileePath: System.Reflection.Assembly.GetExecutingAssembly().Location,
if (!RunProfilerTest(nameof(NonBlittablePInvokeToNonBlittableInteropDelegate), nameof(DoPInvokeNonBlitable), "Invoke"))
{
return 102;
}

return 100;
}

private static bool RunProfilerTest(string testName, string pInvokeExpectedName, string reversePInvokeExpectedName)
{
try
{
return ProfilerTestRunner.Run(profileePath: System.Reflection.Assembly.GetExecutingAssembly().Location,
testName: "Transitions",
profilerClsid: TransitionsGuid);
profilerClsid: TransitionsGuid,
profileeArguments: testName,
envVars: new Dictionary<string, string>
{
{ PInvokeExpectedNameEnvVar, pInvokeExpectedName },
{ ReversePInvokeExpectedNameEnvVar, reversePInvokeExpectedName },
}) == 100;
}
catch (Exception ex)
{
Console.WriteLine(ex);
}
return false;
}
}
}