Skip to content

Commit

Permalink
Account for CORINFO_HELP_VIRTUAL_FUNC_PTR in GT_LABEL; avoid double r…
Browse files Browse the repository at this point in the history
…esolving (#87395)

In the helper-based tailcall mechanism it is possible that we expand the
target call into two actual calls: first, a call to
CORINFO_HELP_VIRTUAL_FUNC_PTR to compute the target, and second a call
to that target. We were not taking into account that the return address
needed for the tailcall mechanism needs to be from the second call.

In this particular case the runtime does not request the JIT to pass the
target; that means we end up resolving the target from both the caller
and from the CallTailCallTarget stub. Ideally the JIT would be able to
eliminate the CORINFO_HELP_VIRTUAL_FUNC_PTR call in the caller since it
turns out to be unused, but that requires changes in DCE (and is
somewhat non-trivial, as we have to preserve a null-check).

A simpler way to improve the case is to just change the runtime to
always request the target from the JIT for GVMs, which means the
CallTailCallTarget stub no longer needs to resolve it. That also has the
effect of fixing the original issue, but I have left the original fix in
as well.

Fix #87393
  • Loading branch information
jakobbotsch authored Jun 12, 2023
1 parent 4315b52 commit 98926e2
Show file tree
Hide file tree
Showing 10 changed files with 102 additions and 37 deletions.
1 change: 1 addition & 0 deletions src/coreclr/jit/codegen.h
Original file line number Diff line number Diff line change
Expand Up @@ -1240,6 +1240,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
regNumber getCallIndirectionCellReg(GenTreeCall* call);
void genCall(GenTreeCall* call);
void genCallInstruction(GenTreeCall* call X86_ARG(target_ssize_t stackArgBytes));
void genDefinePendingCallLabel(GenTreeCall* call);
void genJmpMethod(GenTree* jmp);
BasicBlock* genCallFinally(BasicBlock* block);
#if defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64)
Expand Down
10 changes: 1 addition & 9 deletions src/coreclr/jit/codegenarmarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3213,15 +3213,7 @@ void CodeGen::genCall(GenTreeCall* call)

genCallInstruction(call);

// for pinvoke/intrinsic/tailcalls we may have needed to get the address of
// a label. In case it is indirect with CFG enabled make sure we do not get
// the address after the validation but only after the actual call that
// comes after.
if (genPendingCallLabel && !call->IsHelperCall(compiler, CORINFO_HELP_VALIDATE_INDIRECT_CALL))
{
genDefineInlineTempLabel(genPendingCallLabel);
genPendingCallLabel = nullptr;
}
genDefinePendingCallLabel(call);

#ifdef DEBUG
// We should not have GC pointers in killed registers live around the call.
Expand Down
29 changes: 29 additions & 0 deletions src/coreclr/jit/codegencommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6373,6 +6373,35 @@ regNumber CodeGen::getCallIndirectionCellReg(GenTreeCall* call)
return result;
}

//------------------------------------------------------------------------
// genDefinePendingLabel - If necessary, define the pending call label after a
// call instruction was emitted.
//
// Arguments:
// call - the call node
//
void CodeGen::genDefinePendingCallLabel(GenTreeCall* call)
{
// for pinvoke/intrinsic/tailcalls we may have needed to get the address of
// a label.
if (!genPendingCallLabel)
{
return;
}

// For certain indirect calls we may introduce helper calls before that we need to skip:
// - CFG may introduce a call to the validator first
// - Generic virtual methods may compute the target dynamically through a separate helper call
if (call->IsHelperCall(compiler, CORINFO_HELP_VALIDATE_INDIRECT_CALL) ||
call->IsHelperCall(compiler, CORINFO_HELP_VIRTUAL_FUNC_PTR))
{
return;
}

genDefineInlineTempLabel(genPendingCallLabel);
genPendingCallLabel = nullptr;
}

/*****************************************************************************
*
* Generates code for all the function and funclet prologs and epilogs.
Expand Down
10 changes: 1 addition & 9 deletions src/coreclr/jit/codegenloongarch64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6499,15 +6499,7 @@ void CodeGen::genCall(GenTreeCall* call)

genCallInstruction(call);

// for pinvoke/intrinsic/tailcalls we may have needed to get the address of
// a label. In case it is indirect with CFG enabled make sure we do not get
// the address after the validation but only after the actual call that
// comes after.
if (genPendingCallLabel && !call->IsHelperCall(compiler, CORINFO_HELP_VALIDATE_INDIRECT_CALL))
{
genDefineInlineTempLabel(genPendingCallLabel);
genPendingCallLabel = nullptr;
}
genDefinePendingCallLabel(call);

#ifdef DEBUG
// We should not have GC pointers in killed registers live around the call.
Expand Down
10 changes: 1 addition & 9 deletions src/coreclr/jit/codegenriscv64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6139,15 +6139,7 @@ void CodeGen::genCall(GenTreeCall* call)

genCallInstruction(call);

// for pinvoke/intrinsic/tailcalls we may have needed to get the address of
// a label. In case it is indirect with CFG enabled make sure we do not get
// the address after the validation but only after the actual call that
// comes after.
if (genPendingCallLabel && !call->IsHelperCall(compiler, CORINFO_HELP_VALIDATE_INDIRECT_CALL))
{
genDefineInlineTempLabel(genPendingCallLabel);
genPendingCallLabel = nullptr;
}
genDefinePendingCallLabel(call);

#ifdef DEBUG
// We should not have GC pointers in killed registers live around the call.
Expand Down
10 changes: 1 addition & 9 deletions src/coreclr/jit/codegenxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5857,15 +5857,7 @@ void CodeGen::genCall(GenTreeCall* call)

genCallInstruction(call X86_ARG(stackArgBytes));

// for pinvoke/intrinsic/tailcalls we may have needed to get the address of
// a label. In case it is indirect with CFG enabled make sure we do not get
// the address after the validation but only after the actual call that
// comes after.
if (genPendingCallLabel && !call->IsHelperCall(compiler, CORINFO_HELP_VALIDATE_INDIRECT_CALL))
{
genDefineInlineTempLabel(genPendingCallLabel);
genPendingCallLabel = nullptr;
}
genDefinePendingCallLabel(call);

#ifdef DEBUG
// We should not have GC pointers in killed registers live around the call.
Expand Down
6 changes: 5 additions & 1 deletion src/coreclr/vm/tailcallhelp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,11 @@ void TailCallHelp::CreateTailCallHelperStubs(
LOG((LF_STUBS, LL_INFO1000, "TAILCALLHELP: Incoming sig %s\n", incSig.GetCString()));
#endif

*storeArgsNeedsTarget = pCalleeMD == NULL || pCalleeMD->IsSharedByGenericInstantiations();
// GVMs are not strictly "needs target" for the unshared case, but the JIT
// is going to resolve the target anyway so we may as well take it in the
// stub to avoid computing it in both places.
*storeArgsNeedsTarget = pCalleeMD == NULL || pCalleeMD->IsSharedByGenericInstantiations() ||
(pCalleeMD->IsVirtual() && !pCalleeMD->IsStatic() && pCalleeMD->HasMethodInstantiation());

// The tailcall helper stubs are always allocated together with the caller.
// If we ever wish to share these stubs they should be allocated with the
Expand Down
43 changes: 43 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_87393/Runtime_87393.fs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Runtime_87393

open System.Runtime.CompilerServices

[<AbstractClass>]
type Foo() =
abstract M<'a> : 'a -> int -> int -> int -> int -> int -> int -> int -> int -> int -> int -> int -> int -> int -> int -> int -> int

type Bar() as this =
inherit Foo()

[<DefaultValue>]
static val mutable private _f : Foo

do
Bar._f <- this

override this.M<'a> (a0 : 'a) num acc _ _ _ _ _ _ _ _ _ _ _ _ _ =
if num <= 0 then
acc
else
Bar.M2 a0 (num - 1) (acc + num) 0 0 0 0 0 0 0 0 0 0 0 0 0

[<MethodImpl(MethodImplOptions.NoInlining)>]
static member M2 (a0 : 'a) (num : int) (acc : int) a3 a4 a5 a6 a7 a8 a9 a10 a11 a12 a13 a14 a15 =
Bar._f.M a0 num acc a3 a4 a5 a6 a7 a8 a9 a10 a11 a12 a13 a14 a15

module Main =

[<EntryPoint>]
let main _argv =
let f : Foo = Bar()
let v = f.M 0 65000 0 0 0 0 0 0 0 0 0 0 0 0 0 0
if v = 2112532500 then
printfn "PASS"
100
else
printfn "FAIL: Result was %A" v
-1

Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<!-- Needed for GCStressIncompatible -->
<RequiresProcessIsolation>true</RequiresProcessIsolation>
<NoStandardLib>True</NoStandardLib>
<Noconfig>True</Noconfig>
<Optimize>True</Optimize>
<TargetFramework>$(NetCoreAppToolCurrent)</TargetFramework>
<GCStressIncompatible>True</GCStressIncompatible>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).fs" />
</ItemGroup>
</Project>
6 changes: 6 additions & 0 deletions src/tests/issues.targets
Original file line number Diff line number Diff line change
Expand Up @@ -850,6 +850,9 @@
<ExcludeList Include="$(XunitTestBinBase)/JIT/Directed/tailcall/more_tailcalls/*">
<Issue>https://github.com/dotnet/runtimelab/issues/155: Tailcalls</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_87393/Runtime_87393/*">
<Issue>https://github.com/dotnet/runtimelab/issues/155: Tailcalls</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/JIT/Directed/throwbox/fault_throwbox/*">
<Issue>https://github.com/dotnet/runtimelab/issues/155: Non-exception throws</Issue>
</ExcludeList>
Expand Down Expand Up @@ -1599,6 +1602,9 @@
<ExcludeList Include="$(XunitTestBinBase)/JIT/Directed/tailcall/more_tailcalls/**">
<Issue>needs triage</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_87393/**">
<Issue>FSharp Test</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/JIT/Directed/tailcall/mutual_recursion/**">
<Issue>FSharp Test</Issue>
</ExcludeList>
Expand Down

0 comments on commit 98926e2

Please sign in to comment.