Skip to content

Fix FnPtrTypeDesc::GetManagedClassObject #82325

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

Merged
jkotas merged 2 commits into from
Feb 18, 2023
Merged

Fix FnPtrTypeDesc::GetManagedClassObject #82325

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
13f4e0e
Fix FnPtrTypeDesc::GetManagedClassObject
jkotas Feb 17, 2023
8a1ff58
Revert "Disable System.Reflection.Tests.TypeDelegatorTests.FunctionPo…
jkotas Feb 18, 2023
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: 3 additions & 20 deletions src/coreclr/vm/typedesc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,26 +522,9 @@ OBJECTREF FnPtrTypeDesc::GetManagedClassObject()
}
CONTRACTL_END;

if (m_hExposedClassObject == NULL) {
REFLECTCLASSBASEREF refClass = NULL;
GCPROTECT_BEGIN(refClass);
refClass = (REFLECTCLASSBASEREF) AllocateObject(g_pRuntimeTypeClass);

LoaderAllocator *pLoaderAllocator = GetLoaderAllocator();
TypeHandle th = TypeHandle(this);
((ReflectClassBaseObject*)OBJECTREFToObject(refClass))->SetType(th);
((ReflectClassBaseObject*)OBJECTREFToObject(refClass))->SetKeepAlive(pLoaderAllocator->GetExposedObject());

// Let all threads fight over who wins using InterlockedCompareExchange.
// Only the winner can set m_hExposedClassObject from NULL.
LOADERHANDLE hExposedClassObject = pLoaderAllocator->AllocateHandle(refClass);

if (InterlockedCompareExchangeT(&m_hExposedClassObject, hExposedClassObject, static_cast<LOADERHANDLE>(NULL)))
{
pLoaderAllocator->FreeHandle(hExposedClassObject);
}

GCPROTECT_END();
if (m_hExposedClassObject == NULL)
Copy link
Contributor

@steveharter steveharter Feb 17, 2023

Choose a reason for hiding this comment

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

I see that this pattern was recently changed to support frozen heaps, so this copy-paste above was stale.

Copy link
Member Author

@jkotas jkotas Feb 17, 2023

Choose a reason for hiding this comment

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

Right

{
TypeHandle(this).AllocateManagedClassObject(&m_hExposedClassObject);
}
return GetManagedClassObjectIfExists();
}
Expand Down
33 changes: 24 additions & 9 deletions src/coreclr/vm/typedesc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,7 @@ class FnPtrTypeDesc : public TypeDesc
void EnumMemoryRegions(CLRDataEnumMemoryFlags flags);
#endif //DACCESS_COMPILE

#ifndef DACCESS_COMPILE
OBJECTREF GetManagedClassObject();

OBJECTREF GetManagedClassObjectIfExists()
Expand All @@ -552,23 +553,37 @@ class FnPtrTypeDesc : public TypeDesc
}
CONTRACTL_END;

OBJECTREF objRet = NULL;
GET_LOADERHANDLE_VALUE_FAST(GetLoaderAllocator(), m_hExposedClassObject, &objRet);
return objRet;
const RUNTIMETYPEHANDLE handle = m_hExposedClassObject;

OBJECTREF retVal;
if (!TypeHandle::GetManagedClassObjectFromHandleFast(handle, &retVal) &&
!GetLoaderAllocator()->GetHandleValueFastPhase2(handle, &retVal))
{
return NULL;
}

COMPILER_ASSUME(retVal != NULL);
return retVal;
}

OBJECTREF GetManagedClassObjectFast()
{
LIMITED_METHOD_CONTRACT;

OBJECTREF objRet = NULL;
LoaderAllocator::GetHandleValueFast(m_hExposedClassObject, &objRet);
return objRet;
}
OBJECTREF objRef;
if (!TypeHandle::GetManagedClassObjectFromHandleFast(m_hExposedClassObject, &objRef))
{
return FALSE;
}
COMPILER_ASSUME(objRef != NULL);
return objRef;
}
#endif

protected:
// Handle back to the internal reflection Type object
LOADERHANDLE m_hExposedClassObject;
// Non-unloadable context: internal RuntimeType object handle
// Unloadable context: slot index in LoaderAllocator's pinned table
RUNTIMETYPEHANDLE m_hExposedClassObject;

// Number of arguments
DWORD m_NumArgs;
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/vm/typehandle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,8 +349,7 @@ bool TypeHandle::IsManagedClassObjectPinned() const
{
LIMITED_METHOD_DAC_CONTRACT;

// Function pointers are always mapped to typeof(IntPtr)
return !GetLoaderAllocator()->CanUnload() || IsFnPtrType();
return !GetLoaderAllocator()->CanUnload();
Copy link
Contributor

@steveharter steveharter Feb 17, 2023

Choose a reason for hiding this comment

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

Missed this -- prevents FPs from being moved during GC \ causing GC holes perhaps?

Copy link
Member Author

@jkotas jkotas Feb 17, 2023

Choose a reason for hiding this comment

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

The logic in this method has to be in sync with what GetManagedClassObject returns. If it is not in sync, it will lead to all sorts of problems.

}

void TypeHandle::AllocateManagedClassObject(RUNTIMETYPEHANDLE* pDest)
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.Runtime/tests/System/Reflection/TypeDelegatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ public void Properties()

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/71095", TestRuntimes.Mono)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/82252", TestRuntimes.CoreCLR)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/71883", typeof(PlatformDetection), nameof(PlatformDetection.IsNativeAot))]
public void FunctionPointers()
{
Expand Down