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 12 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,30 @@ 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;

// If it's not a value class, don't compare by value
if (!o1.GetType().IsValueType)
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to end up calling reflection? It would be a good idea to check for perf regression.

Note that this FCALL does not have a helper method frame and your change is not deleting, just making it a bit smaller. FCALLs without a helper method frame are not something we are trying to actively eliminate.

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 haven't checked if JIT treats this as intrinsic. Only typeof(T).IsValueType?

Copy link
Member

Choose a reason for hiding this comment

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

Right

return false;

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

// Compare the contents
return SequenceEqualWithReferences(o1, o2);
}

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

[Obsolete("OffsetToStringData has been deprecated. Use string.GetPinnableReference() instead.")]
public static int OffsetToStringData
Expand Down Expand Up @@ -194,8 +216,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
71 changes: 22 additions & 49 deletions src/coreclr/classlibnative/bcltype/objectnative.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -123,44 +123,19 @@ 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::SequenceEqualWithReferences, 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);
BOOL ret = memcmp(
(void *) (pThisRef+1),
(void *) (pCompareRef+1),
Expand Down Expand Up @@ -215,36 +190,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);
_ASSERTE(objHandle.Get() != NULL); // Should be handled at managed side

HELPER_METHOD_FRAME_BEGIN_RET_1(refClone);
BEGIN_QCALL;

GCX_COOP();
OBJECTREF refClone = objHandle.Get();
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, SequenceEqualWithReferences, 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("Equals", ObjectNative::SequenceEqualWithReferences)
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
Loading