From 5d14a8dfcaff43d525f51f24a7b33da162c3537b Mon Sep 17 00:00:00 2001 From: Huo Yaoyuan Date: Tue, 27 Feb 2024 07:14:36 +0800 Subject: [PATCH] Convert HELPER_METHOD_FRAME in objectnative.cpp to QCall (#97641) * Equals * AllocateUninitializedClone * Compare object content in native code * Do not compare padding in Equals * Add test coverage for Equals --- .../src/System/Object.CoreCLR.cs | 5 +- .../RuntimeHelpers.CoreCLR.cs | 30 ++++++- .../classlibnative/bcltype/objectnative.cpp | 80 ++++++------------- .../classlibnative/bcltype/objectnative.h | 4 +- src/coreclr/vm/ecalllist.h | 3 +- src/coreclr/vm/qcallentrypoints.cpp | 1 + .../CompilerServices/RuntimeHelpersTests.cs | 20 +++++ 7 files changed, 81 insertions(+), 62 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Object.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Object.CoreCLR.cs index 88c929dbe74cb..940d1622bad18 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Object.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Object.CoreCLR.cs @@ -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 @@ -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 diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs index 02ecf96568927..733e3a664bcc5 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs @@ -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) + { + // 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 @@ -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); /// true if given type is reference type or value type that contains references [Intrinsic] diff --git a/src/coreclr/classlibnative/bcltype/objectnative.cpp b/src/coreclr/classlibnative/bcltype/objectnative.cpp index 4622955b44adf..afbda5fad9916 100644 --- a/src/coreclr/classlibnative/bcltype/objectnative.cpp +++ b/src/coreclr/classlibnative/bcltype/objectnative.cpp @@ -123,48 +123,22 @@ FCIMPL1(INT32, ObjectNative::TryGetHashCode, Object* obj) { } FCIMPLEND -// -// Compare by ref for normal classes, by value for value types. -// -// @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. -// - -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(); @@ -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 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) { diff --git a/src/coreclr/classlibnative/bcltype/objectnative.h b/src/coreclr/classlibnative/bcltype/objectnative.h index d8948922dd0b7..418fd2561d7cc 100644 --- a/src/coreclr/classlibnative/bcltype/objectnative.h +++ b/src/coreclr/classlibnative/bcltype/objectnative.h @@ -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); diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index 55b93f1f1ab68..9da9ebdb102a6 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -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) diff --git a/src/coreclr/vm/qcallentrypoints.cpp b/src/coreclr/vm/qcallentrypoints.cpp index 8e6a670297943..2d6466fc7d7c7 100644 --- a/src/coreclr/vm/qcallentrypoints.cpp +++ b/src/coreclr/vm/qcallentrypoints.cpp @@ -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) diff --git a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Runtime/CompilerServices/RuntimeHelpersTests.cs b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Runtime/CompilerServices/RuntimeHelpersTests.cs index 7359e11283f74..0aea60fd1921e 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Runtime/CompilerServices/RuntimeHelpersTests.cs +++ b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Runtime/CompilerServices/RuntimeHelpersTests.cs @@ -68,6 +68,26 @@ public static unsafe void GetObjectValue() Assert.Equal(i, (int)iOV); } + [Fact] + public static void EqualsTest() + { + // Boolean RuntimeHelpers.Equals(Object, Object) + + Assert.True(RuntimeHelpers.Equals(Guid.Empty, Guid.Empty)); + Assert.False(RuntimeHelpers.Equals(Guid.Empty, Guid.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() {