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 all commits
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
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;
using System.Runtime.CompilerServices;

namespace System
Expand All @@ -19,7 +20,9 @@ public partial class Object
[Intrinsic]
protected internal unsafe object MemberwiseClone()
{
object clone = RuntimeHelpers.AllocateUninitializedClone(this);
object clone = this;
RuntimeHelpers.AllocateUninitializedClone(ObjectHandleOnStack.Create(ref clone));
Debug.Assert(clone != this);

// copy contents of "this" to the clone

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,32 @@ public static unsafe void PrepareMethod(RuntimeMethodHandle method, RuntimeTypeH
[MethodImpl(MethodImplOptions.InternalCall)]
internal static extern int TryGetHashCode(object o);

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
return ContentEquals(o1, o2);
}

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

[Obsolete("OffsetToStringData has been deprecated. Use string.GetPinnableReference() instead.")]
public static int OffsetToStringData
Expand Down Expand Up @@ -194,8 +218,8 @@ public static object GetUninitializedObject(
return rt.GetUninitializedObject();
}

[MethodImpl(MethodImplOptions.InternalCall)]
internal static extern object AllocateUninitializedClone(object obj);
[LibraryImport(QCall, EntryPoint = "ObjectNative_AllocateUninitializedClone")]
internal static partial void AllocateUninitializedClone(ObjectHandleOnStack objHandle);

/// <returns>true if given type is reference type or value type that contains references</returns>
[Intrinsic]
Expand Down
80 changes: 26 additions & 54 deletions src/coreclr/classlibnative/bcltype/objectnative.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -123,48 +123,22 @@ 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)
FCIMPL2(FC_BOOL_RET, ObjectNative::ContentEquals, Object *pThisRef, Object *pCompareRef)
{
CONTRACTL
{
FCALL_CHECK;
INJECT_FAULT(FCThrow(kOutOfMemoryException););
}
CONTRACTL_END;

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

// Since we are in FCALL, we must handle NULL specially.
if (pThisRef == NULL || pCompareRef == NULL)
FC_RETURN_BOOL(FALSE);
// Should be ensured by caller
_ASSERTE(pThisRef != NULL);
_ASSERTE(pCompareRef != NULL);
_ASSERTE(pThisRef->GetMethodTable() == pCompareRef->GetMethodTable());

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);
// Compare the contents
BOOL ret = memcmp(
(void *) (pThisRef+1),
(void *) (pCompareRef+1),
dwBaseSize - sizeof(Object) - sizeof(int)) == 0;
pThisRef->GetData(),
pCompareRef->GetData(),
pThisMT->GetNumInstanceFieldBytes()) == 0;

FC_GC_POLL_RET();

Expand Down Expand Up @@ -215,36 +189,34 @@ FCIMPL1(Object*, ObjectNative::GetClass, Object* pThis)
}
FCIMPLEND

FCIMPL1(Object*, ObjectNative::AllocateUninitializedClone, Object* pObjUNSAFE)
extern "C" void QCALLTYPE ObjectNative_AllocateUninitializedClone(QCall::ObjectHandleOnStack objHandle)
{
FCALL_CONTRACT;

// Delegate error handling to managed side (it will throw NullReferenceException)
if (pObjUNSAFE == NULL)
return NULL;
QCALL_CONTRACT;

OBJECTREF refClone = ObjectToOBJECTREF(pObjUNSAFE);
BEGIN_QCALL;

HELPER_METHOD_FRAME_BEGIN_RET_1(refClone);
GCX_COOP();

OBJECTREF refClone = objHandle.Get();
_ASSERTE(refClone != NULL); // Should be handled at managed side
MethodTable* pMT = refClone->GetMethodTable();

// assert that String has overloaded the Clone() method
_ASSERTE(pMT != g_pStringClass);

if (pMT->IsArray()) {
refClone = DupArrayForCloning((BASEARRAYREF)refClone);
} else {

if (pMT->IsArray())
{
objHandle.Set(DupArrayForCloning((BASEARRAYREF)refClone));
}
else
{
// We don't need to call the <cinit> because we know
// that it has been called....(It was called before this was created)
refClone = AllocateObject(pMT);
objHandle.Set(AllocateObject(pMT));
}

HELPER_METHOD_FRAME_END();

return OBJECTREFToObject(refClone);
END_QCALL;
}
FCIMPLEND

extern "C" BOOL QCALLTYPE Monitor_Wait(QCall::ObjectHandleOnStack pThis, INT32 Timeout)
{
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/classlibnative/bcltype/objectnative.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ 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 FCDECL2(FC_BOOL_RET, ContentEquals, Object *pThisRef, Object *pCompareRef);
static FCDECL1(Object*, GetClass, Object* pThis);
static FCDECL1(FC_BOOL_RET, IsLockHeld, Object* pThisUNSAFE);
};

extern "C" void QCALLTYPE ObjectNative_AllocateUninitializedClone(QCall::ObjectHandleOnStack objHandle);
extern "C" BOOL QCALLTYPE Monitor_Wait(QCall::ObjectHandleOnStack pThis, INT32 Timeout);
extern "C" void QCALLTYPE Monitor_Pulse(QCall::ObjectHandleOnStack pThis);
extern "C" void QCALLTYPE Monitor_PulseAll(QCall::ObjectHandleOnStack pThis);
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/vm/ecalllist.h
Original file line number Diff line number Diff line change
Expand Up @@ -471,8 +471,7 @@ FCFuncStart(gRuntimeHelpers)
FCFuncElement("PrepareDelegate", ReflectionInvocation::PrepareDelegate)
FCFuncElement("GetHashCode", ObjectNative::GetHashCode)
FCFuncElement("TryGetHashCode", ObjectNative::TryGetHashCode)
FCFuncElement("Equals", ObjectNative::Equals)
FCFuncElement("AllocateUninitializedClone", ObjectNative::AllocateUninitializedClone)
FCFuncElement("ContentEquals", ObjectNative::ContentEquals)
FCFuncElement("EnsureSufficientExecutionStack", ReflectionInvocation::EnsureSufficientExecutionStack)
FCFuncElement("TryEnsureSufficientExecutionStack", ReflectionInvocation::TryEnsureSufficientExecutionStack)
FCFuncElement("AllocTailCallArgBuffer", TailCallHelp::AllocTailCallArgBuffer)
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/vm/qcallentrypoints.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,7 @@ static const Entry s_QCall[] =
DllImportEntry(GetFileLoadExceptionMessage)
DllImportEntry(FileLoadException_GetMessageForHR)
DllImportEntry(Interlocked_MemoryBarrierProcessWide)
DllImportEntry(ObjectNative_AllocateUninitializedClone)
DllImportEntry(Monitor_Wait)
DllImportEntry(Monitor_Pulse)
DllImportEntry(Monitor_PulseAll)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,26 @@ public static unsafe void GetObjectValue()
Assert.Equal(i, (int)iOV);
}

[Fact]
public static void EqualsTest()
{
// Boolean RuntimeHelpers.Equals(Object, Object)
Copy link
Member

Choose a reason for hiding this comment

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

This should also cover comparing non-valuetypes and other paths that are special-cased in the implementation.


Assert.True(RuntimeHelpers.Equals(Guid.Empty, Guid.Empty));
Assert.False(RuntimeHelpers.Equals(Guid.Empty, Guid.NewGuid()));
Copy link
Contributor

@MichalPetryka MichalPetryka Feb 25, 2024

Choose a reason for hiding this comment

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

Suggested change
Assert.False(RuntimeHelpers.Equals(Guid.Empty, Guid.NewGuid()));
Guid guid = Guid.NewGuid();
Assert.Equals(Guid.Empty == guid, RuntimeHelpers.Equals(Guid.Empty, guid));

In case we'd generate an empty GUID.

Copy link
Member Author

Choose a reason for hiding this comment

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

Guid.NewGuid won't return empty.
The real thing I'm concerned is that we are taking a dependency of the behavior/contract of NewGuid.


// Reference equal
object o = new object();
Assert.True(RuntimeHelpers.Equals(o, o));

// Type mismatch
Assert.False(RuntimeHelpers.Equals(Guid.Empty, string.Empty));

// Non value types
Assert.False(RuntimeHelpers.Equals(new object(), new object()));
Assert.False(RuntimeHelpers.Equals(new int[] { 1, 2, 3 }, new int[] { 1, 2, 3 }));
}

[Fact]
public static void InitializeArray()
{
Expand Down
Loading