Skip to content

Cleanup dead delegate code #129662

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

Open
MichalPetryka wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Cleanup dead delegate code #129662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cfa940d
Cleanup dead delegate code
MichalPetryka Jun 21, 2026
9dcb1ce
Merge branch 'main' into dead-delegate
MichalPetryka Jun 22, 2026
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
3 changes: 1 addition & 2 deletions src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,8 +379,7 @@ internal static Delegate CreateDelegateNoSecurityCheck(Type type, object? target

// Initialize the method...

@jkotas jkotas Jun 21, 2026

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.

Suggested change
// Initialize the method...

Non-sensical comment

Delegate d = InternalAlloc(rtType);
// This is a new internal API added in Whidbey. Currently it's only
// used by the dynamic method code to generate a wrapper delegate.
// This is a new internal API added in Whidbey.

@jkotas jkotas Jun 21, 2026

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.

Suggested change
// This is a new internal API added in Whidbey.

Whibey shipped in 2005. This is not new anymore.

// Allow flexible binding options since the target method is
// unambiguously provided to us.

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ internal MulticastDelegate NewMulticastDelegate(object[] invocationList, int inv

internal void StoreDynamicMethod(MethodInfo dynamicMethod)
{
Debug.Assert(_invocationCount == 0);

@MichalPetryka MichalPetryka Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This assert was incorrect for oven virtual delegates.

@jkotas jkotas Jun 21, 2026

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 method is only ever used to create dynamic method delegate. I do not think there is a way to have open virtual delegate that points to dynamic method.

We may want to delete this method instead (see my other feedback).

Debug.Assert(HasSingleTarget);
_helperObject = dynamicMethod;
}

Expand Down
1 change: 0 additions & 1 deletion src/coreclr/debug/daccess/dacdbiimpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3268,7 +3268,6 @@ DacDbiInterfaceImpl::DelegateType DacDbiInterfaceImpl::GetDelegateType(VMPTR_Obj
{
// If this delegate points to a static function or this is a open virtual delegate, this should be non-null

@jkotas jkotas Jun 21, 2026

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.

Is this comment correct? Open virtual delegate is going to have _invocationCount != 0.

@jkotas jkotas Jun 21, 2026

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.

runtime/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Object_1.cs

Lines 166 to 170 in e556d5a

// Classify by invocation count first:
// anything other than 0 indicates a multicast/wrapper/special-sig delegate
// that this API does not interpret further. Only when invocationCount==0
// do MethodPtr/MethodPtrAux unambiguously identify a closed/open delegate.
DelegateType delegateType = DelegateType.Unknown;

runtime/docs/design/datacontracts/Object.md

Line 214 in e556d5a

0 => del.MethodPtrAux == TargetCodePointer.Null

should be update to match too

@MichalPetryka MichalPetryka Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've fixed that in the other PR so I'd rather not touch this here.

@jkotas jkotas Jun 21, 2026

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.

I thought that the primary goal of this PR is to delete all remnants of the wrapper stubs that I have missed.

// Special case: This might fail in a VSD delegate (instance open virtual)...
// TODO: There is the special signatures cases missing.
TADDR targetMethodPtr = PCODEToPINSTR(pDelObj->GetMethodPtrAux());
if (targetMethodPtr == (TADDR)NULL)
{
Expand Down
51 changes: 12 additions & 39 deletions src/coreclr/vm/comdelegate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1812,10 +1812,7 @@ MethodDesc *COMDelegate::GetMethodDesc(OBJECTREF orDelegate)

// If you modify this logic, please update cDAC IObject.GetDelegateInfo.

MethodDesc *pMethodHandle = NULL;

DELEGATEREF thisDel = (DELEGATEREF) orDelegate;
DELEGATEREF innerDel = NULL;

INT_PTR count = thisDel->GetInvocationCount();
if (count != 0)
Expand All @@ -1825,48 +1822,24 @@ MethodDesc *COMDelegate::GetMethodDesc(OBJECTREF orDelegate)
// - unamanaged ftn ptr - _invocationList == NULL && _invocationCount == -1
// - virtual delegate - _invocationList == null && _invocationCount == (target MethodDesc)
// or _invocationList points to a LoaderAllocator/DynamicResolver

@jkotas jkotas Jun 21, 2026

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.

I do not think _invocationList ever points to a LoaderAllocator/DynamicResolver in the current implementation. Do you agree? I think all comments and code (e.g. InvocationListLogicallyNull ) that deal with it can be deleted.

@MichalPetryka MichalPetryka Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah right I've only moved it there in my other PR. Since that makes it the case, I'd not touch this however.

@jkotas jkotas Jun 21, 2026

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.

Before this delta, the comment was referring to wrapper Delegate so one can guess it is obsolete. After this delta, the comment is simply incorrect that is not an improvement.

I understand that some of the changes will conflict. It is the cost of splitting large changes into multiple PRs. It is important that each of the split PRs is coherent on its own as much as possible.

innerDel = (DELEGATEREF) thisDel->GetInvocationList();
bool fOpenVirtualDelegate = false;

if (innerDel != NULL)
{
MethodTable *pMT = innerDel->GetMethodTable();
if (pMT->IsDelegate())
return GetMethodDesc(innerDel);
if (!pMT->IsArray())
{
// must be a virtual one
fOpenVirtualDelegate = true;
}
}
else
{
if (count != DELEGATE_MARKER_UNMANAGEDFPTR)
{
// must be a virtual one
fOpenVirtualDelegate = true;
}
}
// we return the method desc for the invoke

@jkotas jkotas Jun 21, 2026

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 looks like unfinished sentence. Should this say "... for the first two cases" or something?

OBJECTREF invocationList = thisDel->GetInvocationList();
if ((invocationList != NULL && invocationList->GetMethodTable()->IsArray()) || count == DELEGATE_MARKER_UNMANAGEDFPTR)
return FindDelegateInvokeMethod(thisDel->GetMethodTable());

if (fOpenVirtualDelegate)
pMethodHandle = GetMethodDescForOpenVirtualDelegate(thisDel);
else
pMethodHandle = FindDelegateInvokeMethod(thisDel->GetMethodTable());
return GetMethodDescForOpenVirtualDelegate(thisDel);
}
else
{
// Next, check for an open delegate
PCODE code = thisDel->GetMethodPtrAux();

if (code == (PCODE)NULL)
{
// Must be a normal delegate
code = thisDel->GetMethodPtr();
}

pMethodHandle = NonVirtualEntry2MethodDesc(code);
// Next, check for an open delegate
PCODE code = thisDel->GetMethodPtrAux();
if (code == (PCODE)NULL)
{
// Must be a normal delegate
code = thisDel->GetMethodPtr();
}

MethodDesc *pMethodHandle = NonVirtualEntry2MethodDesc(code);
_ASSERTE(pMethodHandle);
return pMethodHandle;
}
Expand Down
33 changes: 0 additions & 33 deletions src/coreclr/vm/stubmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1242,39 +1242,6 @@ BOOL StubLinkStubManager::TraceDelegateObject(BYTE* pbDel, TraceDestination *tra
return res;
}

// invocationList is not null, so it can be one of the following:
// Multicast, Static closed (special sig), Secure

// rule out the static with special sig
BYTE *pbCount = *(BYTE **)(pbDel + DelegateObject::GetOffsetOfInvocationCount());
if (pbCount == NULL)
{
// it's a static closed, the target lives in _methodAuxPtr
ppbDest = (BYTE **)(pbDel + DelegateObject::GetOffsetOfMethodPtrAux());

if (*ppbDest == NULL)
{
// it's not looking good, bail out
LOG((LF_CORDB,LL_INFO10000, "SLSM::TDO: can't trace into it\n"));
return FALSE;
}

LOG((LF_CORDB,LL_INFO10000, "SLSM::TDO: ppbDest: %p *ppbDest:%p\n", ppbDest, *ppbDest));

BOOL res = StubManager::TraceStub((PCODE) (*ppbDest), trace);

LOG((LF_CORDB,LL_INFO10000, "SLSM::TDO: res: %d, result type: %d\n", (res ? "true" : "false"), trace->GetTraceType()));

return res;
}

MethodTable *pType = *(MethodTable**)pbDelInvocationList;
if (pType->IsDelegate())
{
// this is a secure delegate. The target is hidden inside this field, so recurse.
return TraceDelegateObject(pbDelInvocationList, trace);
}

// Otherwise, we're going for the first invoke of the multi case.
// In order to go to the correct spot, we have just have to fish out
// slot 0 of the invocation list, and figure out where that's going to,
Expand Down