Account for CORINFO_HELP_VIRTUAL_FUNC_PTR in GT_LABEL; avoid double resolving (#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

diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h
index 945b082a2dfb39cec3268ef56a427b6c943261ec..eb190f97f0e479dcdf8ce79a17d5862934b0780d 100644 (file)
--- a/src/coreclr/jit/codegen.h
+++ b/src/coreclr/jit/codegen.h
@@ -1240,6 +1240,7 @@ protected:
     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)

diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp
index ce5b8d547503dfe81b008b3bb65f829fdb19f28a..c0581ee7f7f16221201e2568ac4378365f9cd79c 100644 (file)
--- a/src/coreclr/jit/codegenarmarch.cpp
+++ b/src/coreclr/jit/codegenarmarch.cpp
@@ -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.

diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp
index eaf16b49be7ee340f41e0ae132c46cd0ee86aaaf..b4611c97ffbb6ae59be7d25ff4b4f12937420f2c 100644 (file)
--- a/src/coreclr/jit/codegencommon.cpp
+++ b/src/coreclr/jit/codegencommon.cpp
@@ -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.

diff --git a/src/coreclr/jit/codegenloongarch64.cpp b/src/coreclr/jit/codegenloongarch64.cpp
index 3135507a05c217dc506aa86ef3d455ad937908ee..6198764c91839e84fe1c445f85a8b20326e760e3 100644 (file)
--- a/src/coreclr/jit/codegenloongarch64.cpp
+++ b/src/coreclr/jit/codegenloongarch64.cpp
@@ -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.

diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp
index e85a7336ccd014a51b1cdc83f38b63d94865e143..ba2e3e3f912501a1095b6506cdb617c4041b2d84 100644 (file)
--- a/src/coreclr/jit/codegenriscv64.cpp
+++ b/src/coreclr/jit/codegenriscv64.cpp
@@ -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.

diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp
index 6d7a973ad07520799dff24276d47d0dadb6bd302..8dea6b12769bba7571206c39d55d3ecc2dbeca67 100644 (file)
--- a/src/coreclr/jit/codegenxarch.cpp
+++ b/src/coreclr/jit/codegenxarch.cpp
@@ -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.

diff --git a/src/coreclr/vm/tailcallhelp.cpp b/src/coreclr/vm/tailcallhelp.cpp
index 40a25053b2746a1403088f5c7c937357817d1de2..03e35ca73364d5c0ec5282ed436baec79062bd76 100644 (file)
--- a/src/coreclr/vm/tailcallhelp.cpp
+++ b/src/coreclr/vm/tailcallhelp.cpp
@@ -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

diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_87393/Runtime_87393.fs b/src/tests/JIT/Regression/JitBlue/Runtime_87393/Runtime_87393.fs
new file mode 100644 (file)
index 0000000..5c884d8
--- /dev/null
+++ b/src/tests/JIT/Regression/JitBlue/Runtime_87393/Runtime_87393.fs
@@ -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
+

diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_87393/Runtime_87393.fsproj b/src/tests/JIT/Regression/JitBlue/Runtime_87393/Runtime_87393.fsproj
new file mode 100644 (file)
index 0000000..5da8e9a
--- /dev/null
+++ b/src/tests/JIT/Regression/JitBlue/Runtime_87393/Runtime_87393.fsproj
@@ -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>

diff --git a/src/tests/issues.targets b/src/tests/issues.targets
index 8d1f6458a48bac6589d1b718dc85125708313734..8d8c37253730f04e77cdcbd94b27a3e60aec24f5 100644 (file)
--- a/src/tests/issues.targets
+++ b/src/tests/issues.targets
@@ -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>
@@ -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>