Skip to content

Commit

Permalink
Cleanup delegate-related code (dotnet#107966)
Browse files Browse the repository at this point in the history 
* Cleanup delegate-related code
- Deleted unused and unreachable code
- Fix minor leak of delegate Stubs with collectible assemblies

---------

Co-authored-by: Juan Hoyos <[email protected]>
Co-authored-by: Noah Falk <[email protected]>
  • Loading branch information
@jkotas @hoyosjs @noahfalk
3 people authored Sep 18, 2024
1 parent 14c80b0 commit 7b5f7d4
Show file tree
Hide file tree
Showing 10 changed files with 45 additions and 259 deletions.
4 changes: 0 additions & 4 deletions src/coreclr/vm/class.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,6 @@ void EEClass::Destruct(MethodTable * pOwningMT)
ExecutableWriterHolder<Stub> stubWriterHolder(pDelegateEEClass->m_pInstRetBuffCallStub, sizeof(Stub));
stubWriterHolder.GetRW()->DecRef();
}
// While m_pMultiCastInvokeStub is also a member,
// it is owned by the m_pMulticastStubCache, not by the class
// - it is shared across classes. So we don't decrement
// its ref count here
}

#ifdef FEATURE_COMINTEROP
Expand Down
16 changes: 2 additions & 14 deletions src/coreclr/vm/class.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1895,11 +1895,9 @@ class DelegateEEClass : public EEClass
PTR_Stub m_pStaticCallStub;
PTR_Stub m_pInstRetBuffCallStub;
PTR_MethodDesc m_pInvokeMethod;
PTR_Stub m_pMultiCastInvokeStub;
PTR_Stub m_pWrapperDelegateInvokeStub;
PCODE m_pMultiCastInvokeStub;
PCODE m_pWrapperDelegateInvokeStub;
UMThunkMarshInfo* m_pUMThunkMarshInfo;
PTR_MethodDesc m_pBeginInvokeMethod;
PTR_MethodDesc m_pEndInvokeMethod;
Volatile<PCODE> m_pMarshalStub;

#ifdef FEATURE_COMINTEROP
Expand All @@ -1911,16 +1909,6 @@ class DelegateEEClass : public EEClass
return m_pInvokeMethod;
}

PTR_MethodDesc GetBeginInvokeMethod()
{
return m_pBeginInvokeMethod;
}

PTR_MethodDesc GetEndInvokeMethod()
{
return m_pEndInvokeMethod;
}

#ifndef DACCESS_COMPILE
DelegateEEClass() : EEClass()
{
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/clrtocomcall.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,13 +389,13 @@ static CallsiteDetails CreateCallsiteDetails(_In_ FramedMethodFrame *pFrame)
DelegateEEClass* delegateCls = (DelegateEEClass*)pMD->GetMethodTable()->GetClass();
_ASSERTE(pFrame->GetThis()->GetMethodTable()->IsDelegate());

if (pMD == delegateCls->m_pBeginInvokeMethod)
if (strcmp(pMD->GetName(), "BeginInvoke") == 0)
{
callsiteFlags |= CallsiteDetails::BeginInvoke;
}
else
{
_ASSERTE(pMD == delegateCls->m_pEndInvokeMethod);
_ASSERTE(strcmp(pMD->GetName(), "EndInvoke") == 0);
callsiteFlags |= CallsiteDetails::EndInvoke;
}

Expand Down
3 changes: 0 additions & 3 deletions src/coreclr/vm/codeman.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,6 @@ enum StubCodeBlockKind : int
STUB_CODE_BLOCK_NOCODE,
STUB_CODE_BLOCK_MANAGED,
STUB_CODE_BLOCK_STUBLINK,
// Placeholdes used by NGen images
STUB_CODE_BLOCK_VIRTUAL_METHOD_THUNK,
STUB_CODE_BLOCK_EXTERNAL_METHOD_THUNK,
// Placeholdes used by ReadyToRun images
STUB_CODE_BLOCK_METHOD_CALL_THUNK,
};
Expand Down
45 changes: 15 additions & 30 deletions src/coreclr/vm/comdelegate.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1945,19 +1945,8 @@ PCODE COMDelegate::GetInvokeMethodStub(EEImplMethodDesc* pMD)
}
else
{

// Since we do not support asynchronous delegates in CoreCLR, we much ensure that it was indeed a async delegate call
// and not an invalid-delegate-layout condition.
//
// If the call was indeed for async delegate invocation, we will just throw an exception.
if ((pMD == pClass->GetBeginInvokeMethod()) || (pMD == pClass->GetEndInvokeMethod()))
{
COMPlusThrow(kPlatformNotSupportedException);
}


_ASSERTE(!"Bad Delegate layout");
COMPlusThrow(kInvalidProgramException);
// We do not support asynchronous delegates in CoreCLR
COMPlusThrow(kPlatformNotSupportedException);
}

RETURN ret;
Expand Down Expand Up @@ -2137,8 +2126,7 @@ FCIMPL1(PCODE, COMDelegate::GetMulticastInvoke, MethodTable* pDelegateMT)
_ASSERTE(pDelegateMT != NULL);

DelegateEEClass* delegateEEClass = (DelegateEEClass*)pDelegateMT->GetClass();
Stub *pStub = delegateEEClass->m_pMultiCastInvokeStub;
return (pStub != NULL) ? pStub->GetEntryPoint() : (PCODE)NULL;
return delegateEEClass->m_pMultiCastInvokeStub;
}
FCIMPLEND

Expand All @@ -2147,13 +2135,13 @@ extern "C" PCODE QCALLTYPE Delegate_GetMulticastInvokeSlow(MethodTable* pDelegat
QCALL_CONTRACT;
_ASSERTE(pDelegateMT != NULL);

PCODE fptr = (PCODE)NULL;
PCODE pStub = (PCODE)NULL;

BEGIN_QCALL;

DelegateEEClass *delegateEEClass = (DelegateEEClass*)pDelegateMT->GetClass();
Stub *pStub = delegateEEClass->m_pMultiCastInvokeStub;
if (pStub == NULL)
pStub = delegateEEClass->m_pMultiCastInvokeStub;
if (pStub == (PCODE)NULL)
{
MethodDesc* pMD = delegateEEClass->GetInvokeMethod();

Expand Down Expand Up @@ -2263,17 +2251,15 @@ extern "C" PCODE QCALLTYPE Delegate_GetMulticastInvokeSlow(MethodTable* pDelegat
pSig, cbSig,
NULL,
&sl);
pStub = Stub::NewStub(JitILStub(pStubMD));
pStub = JitILStub(pStubMD);

InterlockedCompareExchangeT<PTR_Stub>(&delegateEEClass->m_pMultiCastInvokeStub, pStub, NULL);
InterlockedCompareExchangeT<PCODE>(&delegateEEClass->m_pMultiCastInvokeStub, pStub, (PCODE)NULL);
pStub = delegateEEClass->m_pMultiCastInvokeStub;
}

fptr = pStub->GetEntryPoint();

END_QCALL;

return fptr;
return pStub;
}

PCODE COMDelegate::GetWrapperInvoke(MethodDesc* pMD)
Expand All @@ -2288,11 +2274,10 @@ PCODE COMDelegate::GetWrapperInvoke(MethodDesc* pMD)

MethodTable * pDelegateMT = pMD->GetMethodTable();
DelegateEEClass* delegateEEClass = (DelegateEEClass*) pDelegateMT->GetClass();
Stub *pStub = delegateEEClass->m_pWrapperDelegateInvokeStub;
PCODE pStub = delegateEEClass->m_pWrapperDelegateInvokeStub;

if (pStub == NULL)
if (pStub == (PCODE)NULL)
{

GCX_PREEMP();

MetaSig sig(pMD);
Expand Down Expand Up @@ -2333,12 +2318,12 @@ PCODE COMDelegate::GetWrapperInvoke(MethodDesc* pMD)
NULL,
&sl);

pStub = Stub::NewStub(JitILStub(pStubMD));

InterlockedCompareExchangeT<PTR_Stub>(&delegateEEClass->m_pWrapperDelegateInvokeStub, pStub, NULL);
pStub = JitILStub(pStubMD);

InterlockedCompareExchangeT<PCODE>(&delegateEEClass->m_pWrapperDelegateInvokeStub, pStub, (PCODE)NULL);
pStub = delegateEEClass->m_pWrapperDelegateInvokeStub;
}
return pStub->GetEntryPoint();
return pStub;
}


Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/vm/comdelegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ class ShuffleThunkCache : public StubCacheBase
STANDARD_VM_CONTRACT;

((CPUSTUBLINKER*)pstublinker)->EmitShuffleThunk((ShuffleEntry*)pRawStub);
return NEWSTUB_FL_THUNK;
return NEWSTUB_FL_SHUFFLE_THUNK;
}

//---------------------------------------------------------
Expand Down
10 changes: 2 additions & 8 deletions src/coreclr/vm/methodtablebuilder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6138,15 +6138,9 @@ MethodTableBuilder::InitMethodDesc(
BAD_FORMAT_NOTHROW_ASSERT(((DelegateEEClass*)GetHalfBakedClass())->m_pInvokeMethod == NULL);
((DelegateEEClass*)GetHalfBakedClass())->m_pInvokeMethod = pNewMD;
}
else if (strcmp(pMethodName, "BeginInvoke") == 0)
else if (strcmp(pMethodName, "BeginInvoke") == 0 || strcmp(pMethodName, "EndInvoke") == 0)
{
BAD_FORMAT_NOTHROW_ASSERT(((DelegateEEClass*)GetHalfBakedClass())->m_pBeginInvokeMethod == NULL);
((DelegateEEClass*)GetHalfBakedClass())->m_pBeginInvokeMethod = pNewMD;
}
else if (strcmp(pMethodName, "EndInvoke") == 0)
{
BAD_FORMAT_NOTHROW_ASSERT(((DelegateEEClass*)GetHalfBakedClass())->m_pEndInvokeMethod == NULL);
((DelegateEEClass*)GetHalfBakedClass())->m_pEndInvokeMethod = pNewMD;
// Obsolete async methods
}
else
{
Expand Down
43 changes: 3 additions & 40 deletions src/coreclr/vm/stublink.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,6 @@ StubLinker::StubLinker()
m_pCodeElements = NULL;
m_pFirstCodeLabel = NULL;
m_pFirstLabelRef = NULL;
m_pPatchLabel = NULL;
m_pTargetMethod = NULL;
m_stackSize = 0;
m_fDataOnly = FALSE;
Expand Down Expand Up @@ -622,26 +621,6 @@ CodeLabel* StubLinker::EmitNewCodeLabel()
}


//---------------------------------------------------------------
// Creates & emits the patch offset label for the stub
//---------------------------------------------------------------
VOID StubLinker::EmitPatchLabel()
{
CONTRACTL
{
THROWS;
GC_NOTRIGGER;
}
CONTRACTL_END;

//
// Note that it's OK to have re-emit the patch label,
// just use the later one.
//

m_pPatchLabel = EmitNewCodeLabel();
}

//---------------------------------------------------------------
// Returns final location of label as an offset from the start
// of the stub. Can only be called after linkage.
Expand Down Expand Up @@ -1099,31 +1078,15 @@ bool StubLinker::EmitStub(Stub* pStub, int globalsize, int totalSize, LoaderHeap
ZeroMemory(pCodeRW + lastCodeOffset, globalsize - lastCodeOffset);
}

// Set additional stub data.
// - Fill in the target method for the Instantiating stub.
//
// - Fill in patch offset, if we have one
// Note that these offsets are relative to the start of the stub,
// not the code, so you'll have to add sizeof(Stub) to get to the
// right spot.
// Fill in the target method for the Instantiating stub.
if (pStubRW->IsInstantiatingStub())
{
_ASSERTE(m_pTargetMethod != NULL);
_ASSERTE(m_pPatchLabel == NULL);
pStubRW->SetInstantiatedMethodDesc(m_pTargetMethod);

LOG((LF_CORDB, LL_INFO100, "SL::ES: InstantiatedMethod fd:0x%x\n",
pStub->GetInstantiatedMethodDesc()));
}
else if (m_pPatchLabel != NULL)
{
UINT32 uLabelOffset = GetLabelOffset(m_pPatchLabel);
_ASSERTE(FitsIn<USHORT>(uLabelOffset));
pStubRW->SetPatchOffset(static_cast<USHORT>(uLabelOffset));

LOG((LF_CORDB, LL_INFO100, "SL::ES: patch offset:0x%x\n",
pStub->GetPatchOffset()));
}

#ifdef STUBLINKER_GENERATES_UNWIND_INFO
if (pStub->HasUnwindInfo())
Expand Down Expand Up @@ -2260,8 +2223,8 @@ void Stub::SetupStub(int numCodeBytes, DWORD flags
m_numCodeBytesAndFlags |= EXTERNAL_ENTRY_BIT;
if ((flags & NEWSTUB_FL_INSTANTIATING_METHOD) != 0)
m_numCodeBytesAndFlags |= INSTANTIATING_STUB_BIT;
if ((flags & NEWSTUB_FL_THUNK) != 0)
m_numCodeBytesAndFlags |= THUNK_BIT;
if ((flags & NEWSTUB_FL_SHUFFLE_THUNK) != 0)
m_numCodeBytesAndFlags |= SHUFFLE_THUNK_BIT;
}

#ifdef STUBLINKER_GENERATES_UNWIND_INFO
Expand Down
61 changes: 8 additions & 53 deletions src/coreclr/vm/stublink.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,12 +184,6 @@ class StubLinker
//---------------------------------------------------------------
VOID EmitLabel(CodeLabel* pCodeLabel);

//---------------------------------------------------------------
// Emits the patch label for the stub.
// Throws exception on failure.
//---------------------------------------------------------------
void EmitPatchLabel();

//---------------------------------------------------------------
// Create a new label to an external address.
// Throws exception on failure.
Expand Down Expand Up @@ -280,9 +274,6 @@ class StubLinker
CodeElement *m_pCodeElements; // stored in *reverse* order
CodeLabel *m_pFirstCodeLabel; // linked list of CodeLabels
LabelRef *m_pFirstLabelRef; // linked list of references
CodeLabel *m_pPatchLabel; // label of stub patch offset
// currently just for multicast
// frames.
PTR_MethodDesc m_pTargetMethod; // Used for instantiating stubs.
SHORT m_stackSize; // count of pushes/pops
CQuickHeap m_quickHeap; // throwaway heap for
Expand Down Expand Up @@ -452,7 +443,7 @@ enum NewStubFlags
NEWSTUB_FL_MULTICAST = 0x00000002,
NEWSTUB_FL_EXTERNAL = 0x00000004,
NEWSTUB_FL_LOADERHEAP = 0x00000008,
NEWSTUB_FL_THUNK = 0x00000010
NEWSTUB_FL_SHUFFLE_THUNK = 0x00000010
};


Expand All @@ -477,12 +468,12 @@ class Stub
LOADER_HEAP_BIT = 0x20000000,
INSTANTIATING_STUB_BIT = 0x10000000,
UNWIND_INFO_BIT = 0x08000000,
THUNK_BIT = 0x04000000,
SHUFFLE_THUNK_BIT = 0x04000000,

CODEBYTES_MASK = THUNK_BIT - 1,
CODEBYTES_MASK = SHUFFLE_THUNK_BIT - 1,
MAX_CODEBYTES = CODEBYTES_MASK + 1,
};
static_assert_no_msg(CODEBYTES_MASK < THUNK_BIT);
static_assert_no_msg(CODEBYTES_MASK < SHUFFLE_THUNK_BIT);

public:
//-------------------------------------------------------------------
Expand Down Expand Up @@ -530,46 +521,10 @@ class Stub
//-------------------------------------------------------------------
// Used by the debugger to help step through stubs
//-------------------------------------------------------------------
BOOL IsManagedThunk()
{
LIMITED_METHOD_CONTRACT;
return (m_numCodeBytesAndFlags & THUNK_BIT) != 0;
}

//-------------------------------------------------------------------
// For stubs which execute user code, a patch offset needs to be set
// to tell the debugger how far into the stub code the debugger has
// to step until the frame is set up.
//-------------------------------------------------------------------
void SetPatchOffset(USHORT offset)
{
LIMITED_METHOD_CONTRACT;
_ASSERTE(!IsInstantiatingStub());
m_data.PatchOffset = offset;
}

//-------------------------------------------------------------------
// For stubs which execute user code, a patch offset needs to be set
// to tell the debugger how far into the stub code the debugger has
// to step until the frame is set up.
//-------------------------------------------------------------------
USHORT GetPatchOffset()
{
LIMITED_METHOD_CONTRACT;
_ASSERTE(!IsInstantiatingStub());
return m_data.PatchOffset;
}

//-------------------------------------------------------------------
// For stubs which execute user code, a patch offset needs to be set
// to tell the debugger how far into the stub code the debugger has
// to step until the frame is set up.
//-------------------------------------------------------------------
TADDR GetPatchAddress()
BOOL IsShuffleThunk()
{
LIMITED_METHOD_CONTRACT;
_ASSERTE(!IsInstantiatingStub());
return dac_cast<TADDR>(GetEntryPointInternal()) + GetPatchOffset();
return (m_numCodeBytesAndFlags & SHUFFLE_THUNK_BIT) != 0;
}

//-------------------------------------------------------------------
Expand Down Expand Up @@ -834,8 +789,8 @@ class Stub
UINT32 m_numCodeBytesAndFlags;
union
{
USHORT PatchOffset;
PTR_MethodDesc InstantiatedMethod;
// Stub kind specific data
PTR_MethodDesc InstantiatedMethod; // Valid for IsInstantiatingStub() only
} m_data;

#ifdef _DEBUG
Expand Down
Loading

0 comments on commit 7b5f7d4

Please sign in to comment.