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.

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

Convert HELPER_METHOD_FRAME in objectnative.cpp to QCall #97641

Merged
merged 17 commits into from
Feb 26, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
Equals
  • Loading branch information
huoyaoyuan committed Jan 29, 2024
commit d10863806942d413aeba9ff663dd27f4cb96ae1d
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,33 @@ public static unsafe void PrepareMethod(RuntimeMethodHandle method, RuntimeTypeH
[MethodImpl(MethodImplOptions.InternalCall)]
internal static extern int TryGetHashCode(object o);

[MethodImpl(MethodImplOptions.InternalCall)]
public static extern new bool Equals(object? o1, object? o2);
public static new unsafe bool Equals(object? o1, object? o2)
Copy link
Member

Choose a reason for hiding this comment

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

We do not seem to have any test coverage for this method. Could you please add some?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure how to express the mentioned issue about padding bits.

Copy link
Member

Choose a reason for hiding this comment

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

If we had any regular test coverage for this API, we would likely see the tests fail intermittently due to the padding issue.

I do not think you need to bother with adding a test that fails deterministically due the padding issue.

{
// Compare by ref for normal classes, by value for value types.

if (ReferenceEquals(o1, o2))
return true;

if (o1 is null || o2 is null)
return false;

MethodTable* pMT = GetMethodTable(o1);

// If it's not a value class, don't compare by value
if (!pMT->IsValueType)
return false;

// Make sure they are the same type.
if (pMT != GetMethodTable(o2))
return false;

// Compare the contents
uint size = pMT->GetNumInstanceFieldBytes();
return SpanHelpers.SequenceEqual(
ref GetRawData(o1),
ref GetRawData(o2),
size);
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm really not sure if this is GC safe for object field.

Copy link
Member

Choose a reason for hiding this comment

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

I presume it needs a nogc region for that

Copy link
Member

Choose a reason for hiding this comment

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

Right, this does not work. It needs to stay as FCall or QCall w/ SuppressGCTransition.

}

[Obsolete("OffsetToStringData has been deprecated. Use string.GetPinnableReference() instead.")]
public static int OffsetToStringData
Expand Down
49 changes: 0 additions & 49 deletions src/coreclr/classlibnative/bcltype/objectnative.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -123,55 +123,6 @@ FCIMPL1(INT32, ObjectNative::TryGetHashCode, Object* obj) {
}
FCIMPLEND

//
// Compare by ref for normal classes, by value for value types.
//
// <TODO>@todo: it would be nice to customize this method based on the
// defining class rather than doing a runtime check whether it is
// a value type.</TODO>
//

FCIMPL2(FC_BOOL_RET, ObjectNative::Equals, Object *pThisRef, Object *pCompareRef)
{
CONTRACTL
{
FCALL_CHECK;
INJECT_FAULT(FCThrow(kOutOfMemoryException););
}
CONTRACTL_END;

if (pThisRef == pCompareRef)
FC_RETURN_BOOL(TRUE);

// Since we are in FCALL, we must handle NULL specially.
if (pThisRef == NULL || pCompareRef == NULL)
FC_RETURN_BOOL(FALSE);

MethodTable *pThisMT = pThisRef->GetMethodTable();

// If it's not a value class, don't compare by value
if (!pThisMT->IsValueType())
FC_RETURN_BOOL(FALSE);

// Make sure they are the same type.
if (pThisMT != pCompareRef->GetMethodTable())
FC_RETURN_BOOL(FALSE);

// Compare the contents (size - vtable - sync block index).
DWORD dwBaseSize = pThisMT->GetBaseSize();
if(pThisMT == g_pStringClass)
dwBaseSize -= sizeof(WCHAR);
BOOL ret = memcmp(
(void *) (pThisRef+1),
(void *) (pCompareRef+1),
dwBaseSize - sizeof(Object) - sizeof(int)) == 0;

FC_GC_POLL_RET();

FC_RETURN_BOOL(ret);
}
FCIMPLEND

NOINLINE static Object* GetClassHelper(OBJECTREF objRef)
{
FC_INNER_PROLOG(ObjectNative::GetClass);
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/classlibnative/bcltype/objectnative.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ class ObjectNative

static FCDECL1(INT32, GetHashCode, Object* vThisRef);
static FCDECL1(INT32, TryGetHashCode, Object* vThisRef);
static FCDECL2(FC_BOOL_RET, Equals, Object *pThisRef, Object *pCompareRef);
static FCDECL1(Object*, AllocateUninitializedClone, Object* pObjUNSAFE);
static FCDECL1(Object*, GetClass, Object* pThis);
static FCDECL1(FC_BOOL_RET, IsLockHeld, Object* pThisUNSAFE);
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/vm/ecalllist.h
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,6 @@ FCFuncStart(gRuntimeHelpers)
FCFuncElement("PrepareDelegate", ReflectionInvocation::PrepareDelegate)
FCFuncElement("GetHashCode", ObjectNative::GetHashCode)
FCFuncElement("TryGetHashCode", ObjectNative::TryGetHashCode)
FCFuncElement("Equals", ObjectNative::Equals)
FCFuncElement("AllocateUninitializedClone", ObjectNative::AllocateUninitializedClone)
FCFuncElement("EnsureSufficientExecutionStack", ReflectionInvocation::EnsureSufficientExecutionStack)
FCFuncElement("TryEnsureSufficientExecutionStack", ReflectionInvocation::TryEnsureSufficientExecutionStack)
Expand Down