[echesakov]

The issue manifests themselves when

1. methods Callee() and Caller() have the following simple structure

S Callee()
{
  // produce and return instance of S
}

S Caller()
{
    return Callee();
}

2. S is a value type with non-GC fields and has such size that the result values of Callee and Caller must passed via return buffer;
3. call to Callee is transformed to tail call with helpers;
4. Caller is invoked via Reflection.

JIT will transform Caller according to https://github.com/dotnet/runtime/blob/master/docs/design/features/tailcalls-with-helpers.md to

S result;
DispatchTailCalls(&IL_STUB_CallTailCallTarget, (IntPtr)&result, _AddressOfReturnAddress());
return result;

DispatchTailCalls expects result to be a pointer on the stack.

However, during reflection call the return buffer for S value type can be placed on the GC heap

runtime/src/coreclr/src/vm/reflectioninvocation.cpp

Lines 1123 to 1143 in 3665779

	if (fHasRetBuffArg)
	{
	// We stack-allocate this ret buff, to preserve the invariant that ret-buffs are always in the
	// caller's stack frame. We'll copy into gc.retVal later.
	TypeHandle retTH = gc.pSig->GetReturnTypeHandle();
	MethodTable* pMT = retTH.GetMethodTable();
	if (pMT->IsStructRequiringStackAllocRetBuf())
	{
	SIZE_T sz = pMT->GetNumInstanceFieldBytes();
	pRetBufStackCopy = _alloca(sz);
	memset(pRetBufStackCopy, 0, sz);
	pValueClasses = new (_alloca(sizeof(ValueClassInfo))) ValueClassInfo(pRetBufStackCopy, pMT, pValueClasses);
	*((LPVOID*) (pTransitionBlock + argit.GetRetBuffArgOffset())) = pRetBufStackCopy;
	}
	else
	{
	PVOID pRetBuff = gc.retVal->GetData();
	*((LPVOID*) (pTransitionBlock + argit.GetRetBuffArgOffset())) = pRetBuff;
	}
	}

JIT makes a wrong assumption that if Caller and Callee both have return buffer arguments it can be simple passed from the former to the latter in

runtime/src/coreclr/src/jit/morph.cpp

Lines 7907 to 7920 in 3665779

	// Use existing retbuf if there is one.
	if (origCall->HasRetBufArg())
	{
	JITDUMP("Transferring retbuf\n");
	GenTree* retBufArg = origCall->gtCallArgs->GetNode();
	assert((info.compRetBuffArg != BAD_VAR_NUM) && retBufArg->OperIsLocal() &&
	(retBufArg->AsLclVarCommon()->GetLclNum() == info.compRetBuffArg));
	retValArg = retBufArg;
	if (origCall->gtType != TYP_VOID)
	{
	retVal = gtClone(retValArg);
	}
	}

As a consequence, when GC is called at DispatchTailCalls the object corresponding to the return value buffer can be moved but pointers passed to DispatchTailCalls will not be update that leads to Callee using the old non-updated location when writing the return value.

Solution: JIT needs to account for IsStructRequiringStackAllocRetBuf in fgCreateCallDispatcherAndGetResult in the similar way as it is done in fgMorphCall in

runtime/src/coreclr/src/jit/morph.cpp

Lines 8908 to 8919 in 3665779

	if (dest->gtType == TYP_BYREF && !(dest->OperGet() == GT_ADDR && dest->AsOp()->gtOp1->OperGet() == GT_LCL_VAR))
	{
	// We'll exempt helper calls from this, assuming that the helper implementation
	// follows the old convention, and does whatever barrier is required.
	if (call->gtCallType != CT_HELPER)
	{
	structHnd = call->gtRetClsHnd;
	if (info.compCompHnd->isStructRequiringStackAllocRetBuf(structHnd) &&
	!(dest->OperGet() == GT_LCL_VAR && dest->AsLclVar()->GetLclNum() == info.compRetBuffArg))
	{
	// Force re-evaluating the argInfo as the return argument has changed.
	call->fgArgInfo = nullptr;

Thanks @erozenfeld for help with identifying the place with wrong logic in fgCreateCallDispatcherAndGetResult!

[echesakov]

@erozenfeld PTAL
cc @dotnet/jit-contrib

[erozenfeld]

LGTM with a few minor suggestions.

[jakobbotsch]

DispatchTailCalls expects result to be a pointer on the stack.

I don't think this needs to be a hard requirement, if the pointer can be a by-ref to GC heap then the mechanism could pass it through as such.

[echesakov]

I don't think this needs to be a hard requirement, if the pointer can be a by-ref to GC heap then the mechanism could pass it through as such.

@jakobbotsch retBufArg points to the stack in most of the cases. Even during Reflection call we might end up with the return buffer allocated on the stack (e.g., when the corresponding return value type has a GC pointer field) rather than GC heap.

I am not sure if making DispatchTailCalls and TailCallCallTargetStub aware that retVal can point to GC heap (i.e. passing this parameter as byref instead of IntPtr) will be more beneficial. GC will need to check that these byref pointers point to GC heap anyway and in most of the cases they will be pointing to the stack.

[echesakov]

@jakobbotsch Do you remember why this handling was needed here? I understand that on System-V the method that uses return buffer must return an address to the return buffer in rax at the end. But do we use the value in the caller somehow?

[jakobbotsch]

I don't know if JIT uses the returned pointer, but even if not it is presumably necessary for reverse pinvoke.
This is already what the JIT does for functions with ret buffers so I just did the same.

[echesakov]

All GCStress legs except win-arm64 are green with the change. The ones that failed in win-arm64 are due to #38518

[jakobbotsch]

@jakobbotsch retBufArg points to the stack in most of the cases. Even during Reflection call we might end up with the return buffer allocated on the stack (e.g., when the corresponding return value type has a GC pointer field) rather than GC heap.

I am not sure if making DispatchTailCalls and TailCallCallTargetStub aware that retVal can point to GC heap (i.e. passing this parameter as byref instead of IntPtr) will be more beneficial. GC will need to check that these byref pointers point to GC heap anyway and in most of the cases they will be pointing to the stack.

I don't think one more interior pointer should move the needle much and it should simplify the JIT's handling of this.
But the current approach is also fine to me.