Skip to content

Commit

Permalink
Fix duplicate transition for reverse delegates and expand our profile…
Browse files Browse the repository at this point in the history 
…r tests for transitions to cover this case. (#69761)
  • Loading branch information
@jkoritzinsky
jkoritzinsky authored May 25, 2022
1 parent 0b57ba6 commit 6967978
Show file tree
Hide file tree
Showing 5 changed files with 143 additions and 67 deletions.
27 changes: 5 additions & 22 deletions src/coreclr/vm/dllimport.cpp
View file
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
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
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
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
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;
}
}
}

0 comments on commit 6967978

Please sign in to comment.