Delete RETURNTYPE and change how we get ReturnKind for gccover.

[sandreenko]

There are several places where we need to know the return type for a call:

1. HijackFrame::GcScanRoots, it uses vm\threadsuspend.cpp::GetReturnKind that gets correct kind it from EECodeInfo that points to a compiled method (it can't be a prestub), so code in HijackFrame::GcScanRoots is our role model:

   coreclr/src/vm/frames.cpp

   Line 1080 in bdb9959

   void HijackFrame::GcScanRoots(promote_func *fn, ScanContext* sc)

question: can hijacking happen when we are in a compile stub and EECodeInfo is invalid?

2. GcCover.cpp needs to know when to protect call return values when imitating Hijacking, but it doesn't have EECodeInfo (because most MethodDescriptor point to uncompiled code and PCODEs point to compile stubs, so we can't get EECodeInfo->gcInfoToken), so we have to restore ReturnKind from method descriptor. This PR changes MethodDesc::ReturnsObject to return precise information using code that was in ReturnKind GetReturnKindFromMethodTable(Thread *pThread, EECodeInfo *codeInfo) as example.
   Using this ReturnKind we can correctly understand when we need to protect first/second/both return registers on x64 Unix/arm64 and, for example, do not use INTERRUPT_INSTR_PROTECT_RET in tricky cases.

However, ReturnKind GetReturnKindFromMethodTable supports only x64 Unix and doesn't support arm64. So if we call it on arm64 with a method that return 16 bytes struct it fails with assert:

coreclr/src/vm/method.cpp

Lines 1266 to 1278 in 221dc73

	#ifdef UNIX_AMD64_ABI
	if (pReturnTypeMT->IsRegPassedStruct())
	{
	return MetaSig::RETVALUETYPE;
	}
	#endif // !UNIX_AMD64_ABI
	if (pReturnTypeMT->ContainsPointers())
	{
	_ASSERTE(pReturnTypeMT->GetNumInstanceFieldBytes() == sizeof(void*));
	return MetaSig::RETOBJ;
	}
	}

and there is comment:

coreclr/src/vm/threadsuspend.cpp

Lines 5767 to 5769 in 883a271

	// 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).

that says that it is not a simple restriction.

question: how can we get the necessary information for arm64?

3. ComPlusMethodFrame::GcScanRoots needs to know which registers to protect but has the same issues as GCCoder, but this method is not stress testing specific, so I assume it can cause gc holes in a real app that uses COM methods that return struct on arm64 Windows (we can't have COM on Unix).

question: What should we do in this case?

This PR is preparation for https://github.com/dotnet/coreclr/issues/23199. With that change, I have correct kind in GCCover and can use different instruction to mark which registers I need to restore.

Changes by commits:
0af5c96: Move GetReturnKindFromMethodTable to method.hpp.

We would need this in other places in the next commits.

2b0b08d: Delete unnecessary checks from callhelpers.

56e4ad8: Do not check return types in CanDeduplicateCode.

GC info v.2 has this information and it is checked in another place.

f96c1a0: Change ComPlusMethodFrame to use the new function.

92708d4: Change gccover.cpp to use GetReturnKindFromMethodTable.

6930780: Delete RETURNTYPE.

fcb3ee4: Add check to ComPlusMethodFrame.

2e6011d: Delete check from threadsuspend.

codeInfo->GetCodeManager()->GetReturnKind(gcInfoToken) must always return a valid kind nowdays (it could return an invalid lind only when GC Info v2 was not available).

[sandreenko]

PTAL @jkotas @BruceForstall @dotnet/jit-contrib

[jkotas]

Are all the questions in your PR description addressed, or do you still need answers to some of them?

[BruceForstall]

LGTM after addressing @jkotas comments, plus minor questions/nits.

[sandreenko]

Are all the questions in your PR description addressed, or do you still need answers to some of them?

I am not sure about these two:

question: can hijacking happen when we are in a compile stub and EECodeInfo is invalid?
My understanding is not, it can't, but I have not found how it is guaranteed.

ComPlusMethodFrame::GcScanRoots: What should we do in this case?
I have found an example where we use COM method that returns struct, but it did not have two pointers.
I will try to write my own test case to expose this issue, I still do not know how to fix it if I expose the issue.

[jkotas]

can hijacking happen when we are in a compile stub and EECodeInfo is invalid?

It cannot happen. Hijacking is only done for JITed/R2R code.

ComPlusMethodFrame::GcScanRoots:

ComPlusMethodFrame is only used for a few special cases, like COM events. These special cases cannot return structs.

[sandreenko]

ComPlusMethodFrame is only used for a few special cases, like COM events. These special cases cannot return structs.

Interop\COM\NETClients\IDispatch\NETClientIDispatch has a test that returns struct from COM method frame (its signature is instance valuetype Server.Contract.HFA_4 Server.Contract.IDispatchTesting::DoubleHVAValues(valuetype Server.Contract.HFA_4&)), we call it here:

coreclr/tests/src/Interop/COM/NETClients/IDispatch/Program.cs

Lines 147 to 152 in 6e3a85e

	Console.WriteLine($"IDispatch with structs not supported...");
	var dispatchTesting = (DispatchTesting)new DispatchTestingClass();
	var input = new HFA_4() { x = 1f, y = 2f, z = 3f, w = 4f };
	Assert.Throws<NotSupportedException>(() => dispatchTesting.DoubleHVAValues(ref input));
	}

but because this struct doesn't have pointer fields we do not need to report it and return just RT_Scalar.

Thanks for the explanation, now I am sure that the current variant is safe and if in the future somebody adds a case with return RT_*_Obj they will see the new assert.