Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
30 changes: 15 additions & 15 deletions src/coreclr/jit/gcencode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
#include "gcinfotypes.h"
#include "patchpointinfo.h"

#ifdef JIT32_GCENCODER

ReturnKind VarTypeToReturnKind(var_types type)
{
switch (type)
Expand All @@ -29,40 +31,38 @@ ReturnKind VarTypeToReturnKind(var_types type)
return RT_Object;
case TYP_BYREF:
return RT_ByRef;
#ifdef TARGET_X86
case TYP_FLOAT:
case TYP_DOUBLE:
return RT_Float;
#endif // TARGET_X86
default:
return RT_Scalar;
}
}

ReturnKind GCInfo::getReturnKind()
{
// Note the GCInfo representation only supports structs with up to 2 GC pointers.
// Note the JIT32 GCInfo representation only supports structs with 1 GC pointer.
ReturnTypeDesc retTypeDesc = compiler->compRetTypeDesc;
const unsigned regCount = retTypeDesc.GetReturnRegCount();

switch (regCount)
if (regCount == 1)
{
return VarTypeToReturnKind(retTypeDesc.GetReturnRegType(0));
}
else
{
case 1:
return VarTypeToReturnKind(retTypeDesc.GetReturnRegType(0));
case 2:
return GetStructReturnKind(VarTypeToReturnKind(retTypeDesc.GetReturnRegType(0)),
VarTypeToReturnKind(retTypeDesc.GetReturnRegType(1)));
default:
#ifdef DEBUG
for (unsigned i = 0; i < regCount; i++)
{
assert(!varTypeIsGC(retTypeDesc.GetReturnRegType(i)));
}
for (unsigned i = 0; i < regCount; i++)
{
assert(!varTypeIsGC(retTypeDesc.GetReturnRegType(i)));
}
#endif // DEBUG
return RT_Scalar;
return RT_Scalar;
}
}

#endif // JIT32_GCENCODER

// gcMarkFilterVarsPinned - Walk all lifetimes and make it so that anything
// live in a filter is marked as pinned (often by splitting the lifetime
// so that *only* the filter region is pinned). This should only be
Expand Down
5 changes: 2 additions & 3 deletions src/coreclr/jit/jitgcinfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,8 @@ class GCInfo

private:
static size_t gcRecordEpilog(void* pCallBackData, unsigned offset);

ReturnKind getReturnKind();
#else // JIT32_GCENCODER
void gcInfoBlockHdrSave(GcInfoEncoder* gcInfoEncoder, unsigned methodSize, unsigned prologSize);

Expand Down Expand Up @@ -398,9 +400,6 @@ class GCInfo
public:
// This method updates the appropriate reg masks when a variable is moved.
void gcUpdateForRegVarMove(regMaskTP srcMask, regMaskTP dstMask, LclVarDsc* varDsc);

private:
ReturnKind getReturnKind();
};

inline unsigned char encodeUnsigned(BYTE* dest, unsigned value)
Expand Down
76 changes: 21 additions & 55 deletions src/coreclr/vm/method.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1234,8 +1234,9 @@ COR_ILMETHOD* MethodDesc::GetILHeader()
#endif // !DACCESS_COMPILE
}

#if defined(TARGET_X86) || defined(FEATURE_COMINTEROP)
//*******************************************************************************
ReturnKind MethodDesc::ParseReturnKindFromSig(INDEBUG(bool supportStringConstructors))
ReturnKind MethodDesc::GetReturnKind(INDEBUG(bool supportStringConstructors))
{
CONTRACTL
{
Expand All @@ -1247,13 +1248,31 @@ ReturnKind MethodDesc::ParseReturnKindFromSig(INDEBUG(bool supportStringConstruc

ENABLE_FORBID_GC_LOADER_USE_IN_THIS_SCOPE();

// For simplicity, we don't hijack in funclets, but if you ever change that,
// be sure to choose the OnHijack... callback type to match that of the FUNCLET
// not the main method (it would probably be Scalar).

// Mark that we are performing a stackwalker like operation on the current thread.
// This is necessary to allow the signature parsing functions to work without triggering any loads
StackWalkerWalkingThreadHolder threadStackWalking(GetThread());

TypeHandle thValueType;

MetaSig sig(this);
CorElementType et = sig.GetReturnTypeNormalized(&thValueType);

switch (et)
{
#ifdef TARGET_X86
case ELEMENT_TYPE_R4:
case ELEMENT_TYPE_R8:
// Figuring out whether the function returns FP or not is hard to do
// on-the-fly, so we use a different callback helper on x86 where this
// piece of information is needed in order to perform the right save &
// restore of the return value around the call to OnHijackScalarWorker.
return RT_Float;
#endif

case ELEMENT_TYPE_STRING:
case ELEMENT_TYPE_CLASS:
case ELEMENT_TYPE_SZARRAY:
Expand All @@ -1275,34 +1294,6 @@ ReturnKind MethodDesc::ParseReturnKindFromSig(INDEBUG(bool supportStringConstruc
if (!thValueType.IsTypeDesc())
{
MethodTable * pReturnTypeMT = thValueType.AsMethodTable();
#ifdef UNIX_AMD64_ABI
if (pReturnTypeMT->IsRegPassedStruct())
{
// The Multi-reg return case using the classhandle is only implemented for AMD64 SystemV ABI.
// On other platforms, multi-reg return is not supported with GcInfo v1.
// So, the relevant information must be obtained from the GcInfo tables (which requires version2).
EEClass* eeClass = pReturnTypeMT->GetClass();
ReturnKind regKinds[2] = { RT_Unset, RT_Unset };
int orefCount = 0;
for (int i = 0; i < 2; i++)
{
if (eeClass->GetEightByteClassification(i) == SystemVClassificationTypeIntegerReference)
{
regKinds[i] = RT_Object;
}
else if (eeClass->GetEightByteClassification(i) == SystemVClassificationTypeIntegerByRef)
{
regKinds[i] = RT_ByRef;
}
else
{
regKinds[i] = RT_Scalar;
}
}
ReturnKind structReturnKind = GetStructReturnKind(regKinds[0], regKinds[1]);
return structReturnKind;
}
#endif // UNIX_AMD64_ABI

if (pReturnTypeMT->ContainsGCPointers() || pReturnTypeMT->IsByRefLike())
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 have pReturnTypeMT->IsByRefLike(), should we return RT_ByRef ?
Or, if we cannot distinguish RT_Object from RT_ByRef, would RT_ByRef be safer to return?

Copy link
Member

Choose a reason for hiding this comment

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

Does pReturnTypeMT->IsByRefLike() actually imply that there must be a GC ref?

I think at C# level you can make any struct to be stack-only, it does not need to contain pointers or refs. If IsByRefLike() is true for all ref structs, there may not be a pointer.

Perhaps
pReturnTypeMT->ContainsGCPointers() should return RT_Object and
pReturnTypeMT->ContainsGCPointers() && pReturnTypeMT->IsByRefLike() should return RT_ByRef

Or maybe for simplicity just return RT_ByRef for pReturnTypeMT->ContainsGCPointers()?

Copy link
Member

Choose a reason for hiding this comment

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

It is not the part that is being fixed, but since we are touching this, and it looks a bit suspicious,...

Copy link
Member

@jkotas jkotas Jun 9, 2025

Choose a reason for hiding this comment

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

Ok, I have done more cleanup. This method is only used for GC stress on x86 now. GC stress can tolerate RT_Illegal return (it did that before this change as well, so it is not a coverage regression).

{
Expand Down Expand Up @@ -1347,32 +1338,7 @@ ReturnKind MethodDesc::ParseReturnKindFromSig(INDEBUG(bool supportStringConstruc

return RT_Scalar;
}

ReturnKind MethodDesc::GetReturnKind(INDEBUG(bool supportStringConstructors))
{
// For simplicity, we don't hijack in funclets, but if you ever change that,
// be sure to choose the OnHijack... callback type to match that of the FUNCLET
// not the main method (it would probably be Scalar).

ENABLE_FORBID_GC_LOADER_USE_IN_THIS_SCOPE();
// Mark that we are performing a stackwalker like operation on the current thread.
// This is necessary to allow the signature parsing functions to work without triggering any loads
StackWalkerWalkingThreadHolder threadStackWalking(GetThread());

#ifdef TARGET_X86
MetaSig msig(this);
if (msig.HasFPReturn())
{
// Figuring out whether the function returns FP or not is hard to do
// on-the-fly, so we use a different callback helper on x86 where this
// piece of information is needed in order to perform the right save &
// restore of the return value around the call to OnHijackScalarWorker.
return RT_Float;
}
#endif // TARGET_X86

return ParseReturnKindFromSig(INDEBUG(supportStringConstructors));
}
#endif // TARGET_X86 || FEATURE_COMINTEROP

#ifdef FEATURE_COMINTEROP

Expand Down
17 changes: 7 additions & 10 deletions src/coreclr/vm/method.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1688,18 +1688,15 @@ class MethodDesc
// corresponding instantiation of the target of a call.
MethodDesc *ResolveGenericVirtualMethod(OBJECTREF *orThis);


private:
ReturnKind ParseReturnKindFromSig(INDEBUG(bool supportStringConstructors = false));

#if defined(TARGET_X86) || defined(FEATURE_COMINTEROP)
public:
// This method is used to restore ReturnKind using the class handle, it is fully supported only on x64 Ubuntu,
// other platforms do not support multi-reg return case with pointers.
// Use this method only when you can't hit this case
// (like CLRToCOMMethodFrame::GcScanRoots) or when you can tolerate RT_Illegal return.
// Also, on the other platforms for a single field struct return case
// the function can't distinguish RT_Object and RT_ByRef.
// This method is used to restore ReturnKind using the class handle,e
// It does not support multi-reg return case with pointers. Use this
// method only when you can't hit
// this case (like CLRToCOMMethodFrame::GcScanRoots) or when you can tolerate RT_Illegal return.
// Also, for a single field struct return case the function can't distinguish RT_Object and RT_ByRef.
ReturnKind GetReturnKind(INDEBUG(bool supportStringConstructors = false));
#endif // TARGET_X86 || FEATURE_COMINTEROP

public:
// In general you don't want to call GetCallTarget - you want to
Expand Down
Loading