Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libraries-jitstress failure in System.Diagnostics.Tests.StackFrameTests.Ctor_FNeedFileInfo #69154

Closed
jakobbotsch opened this issue May 10, 2022 · 18 comments
Assignees
Labels
arch-arm64 arch-x64 area-System.Reflection JitStress CLR JIT issues involving JIT internal stress modes os-linux Linux OS (any supported distro) os-windows

Comments

@jakobbotsch
Copy link
Member

Seen on testing for #69117.
Pipeline: https://dev.azure.com/dnceng/public/_build/results?buildId=1762407&view=results
Test: https://dev.azure.com/dnceng/public/_build/results?buildId=1762407&view=ms.vss-test-web.build-test-results-tab&runId=47434070&resultId=110968&paneView=debug
Log: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-69117-merge-a31de8e595db4aa8af/System.Diagnostics.StackTrace.Tests/1/console.b93af7d3.log?helixlogtype=result

C:\h\w\BB8A09D2\w\AA4A094A\e>set COMPlus 
COMPlus_JitStress=1
COMPlus_TieredCompilation=0

C:\h\w\BB8A09D2\w\AA4A094A\e>call RunTests.cmd --runtime-path C:\h\w\BB8A09D2\p 
----- start Tue 05/10/2022 15:49:12.15 ===============  To repro directly: ===================================================== 
pushd C:\h\w\BB8A09D2\w\AA4A094A\e\
"C:\h\w\BB8A09D2\p\dotnet.exe" exec --runtimeconfig System.Diagnostics.StackTrace.Tests.runtimeconfig.json --depsfile System.Diagnostics.StackTrace.Tests.deps.json xunit.console.dll System.Diagnostics.StackTrace.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing 
popd
===========================================================================================================

C:\h\w\BB8A09D2\w\AA4A094A\e>"C:\h\w\BB8A09D2\p\dotnet.exe" exec --runtimeconfig System.Diagnostics.StackTrace.Tests.runtimeconfig.json --depsfile System.Diagnostics.StackTrace.Tests.deps.json xunit.console.dll System.Diagnostics.StackTrace.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
  Discovering: System.Diagnostics.StackTrace.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Diagnostics.StackTrace.Tests (found 53 test cases)
  Starting:    System.Diagnostics.StackTrace.Tests (parallel test collections = on, max threads = 4)
    System.Diagnostics.Tests.StackFrameTests.Ctor_FNeedFileInfo(fNeedFileInfo: False) [FAIL]
      Expected GetILOffset() -1 for InvokeStub_StackFrameTests.Ctor_FNeedFileInfo at offset 44 in file:line:column <filename unknown>:0:0
       to be greater or equal to zero.
      Expected: True
      Actual:   False
      Stack Trace:
        /_/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs(171,0): at System.Diagnostics.Tests.StackFrameTests.VerifyStackFrameSkipFrames(StackFrame stackFrame, Boolean isFileConstructor, Int32 skipFrames, MethodInfo expectedMethod, Boolean isCurrentFrame)
           at InvokeStub_StackFrameTests.Ctor_FNeedFileInfo(Object, Object, IntPtr*)
        /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs(60,0): at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
  Finished:    System.Diagnostics.StackTrace.Tests
=== TEST EXECUTION SUMMARY ===
   System.Diagnostics.StackTrace.Tests  Total: 121, Errors: 0, Failed: 1, Skipped: 0, Time: 10.512s
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 10, 2022
@jakobbotsch jakobbotsch added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 10, 2022
@ghost
Copy link

ghost commented May 10, 2022

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

Seen on testing for #69117.
Pipeline: https://dev.azure.com/dnceng/public/_build/results?buildId=1762407&view=results
Test: https://dev.azure.com/dnceng/public/_build/results?buildId=1762407&view=ms.vss-test-web.build-test-results-tab&runId=47434070&resultId=110968&paneView=debug
Log: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-69117-merge-a31de8e595db4aa8af/System.Diagnostics.StackTrace.Tests/1/console.b93af7d3.log?helixlogtype=result

C:\h\w\BB8A09D2\w\AA4A094A\e>set COMPlus 
COMPlus_JitStress=1
COMPlus_TieredCompilation=0

C:\h\w\BB8A09D2\w\AA4A094A\e>call RunTests.cmd --runtime-path C:\h\w\BB8A09D2\p 
----- start Tue 05/10/2022 15:49:12.15 ===============  To repro directly: ===================================================== 
pushd C:\h\w\BB8A09D2\w\AA4A094A\e\
"C:\h\w\BB8A09D2\p\dotnet.exe" exec --runtimeconfig System.Diagnostics.StackTrace.Tests.runtimeconfig.json --depsfile System.Diagnostics.StackTrace.Tests.deps.json xunit.console.dll System.Diagnostics.StackTrace.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing 
popd
===========================================================================================================

C:\h\w\BB8A09D2\w\AA4A094A\e>"C:\h\w\BB8A09D2\p\dotnet.exe" exec --runtimeconfig System.Diagnostics.StackTrace.Tests.runtimeconfig.json --depsfile System.Diagnostics.StackTrace.Tests.deps.json xunit.console.dll System.Diagnostics.StackTrace.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
  Discovering: System.Diagnostics.StackTrace.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Diagnostics.StackTrace.Tests (found 53 test cases)
  Starting:    System.Diagnostics.StackTrace.Tests (parallel test collections = on, max threads = 4)
    System.Diagnostics.Tests.StackFrameTests.Ctor_FNeedFileInfo(fNeedFileInfo: False) [FAIL]
      Expected GetILOffset() -1 for InvokeStub_StackFrameTests.Ctor_FNeedFileInfo at offset 44 in file:line:column <filename unknown>:0:0
       to be greater or equal to zero.
      Expected: True
      Actual:   False
      Stack Trace:
        /_/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs(171,0): at System.Diagnostics.Tests.StackFrameTests.VerifyStackFrameSkipFrames(StackFrame stackFrame, Boolean isFileConstructor, Int32 skipFrames, MethodInfo expectedMethod, Boolean isCurrentFrame)
           at InvokeStub_StackFrameTests.Ctor_FNeedFileInfo(Object, Object, IntPtr*)
        /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs(60,0): at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
  Finished:    System.Diagnostics.StackTrace.Tests
=== TEST EXECUTION SUMMARY ===
   System.Diagnostics.StackTrace.Tests  Total: 121, Errors: 0, Failed: 1, Skipped: 0, Time: 10.512s
Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr, untriaged

Milestone: -

@jakobbotsch jakobbotsch added area-VM-coreclr and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels May 10, 2022
@jakobbotsch
Copy link
Member Author

jakobbotsch commented May 10, 2022

@steveharter this does not repro if I revert 5195418.
FWIW, in jitstress mode we make different inlining decisions, so that might be affecting things here.

cc @dotnet/jit-contrib

@jakobbotsch jakobbotsch added JitStress CLR JIT issues involving JIT internal stress modes blocking-clean-ci-optional Blocking optional rolling runs labels May 10, 2022
@AndyAyersMS
Copy link
Member

@steveharter this does not repro if I revert 5195418.
FWIW, in jitstress mode we make different inlining decisions, so that might be affecting things here.

Right, if you want to depend on certain frames showing up in stack traces make sure those frames involve NoInline methods. Otherwise random inlining (via jit stress), profile driven inlining, or changes to the jit can lead to spurious test failures.

@VincentBu VincentBu added arch-arm64 os-linux Linux OS (any supported distro) os-windows arch-x64 labels May 11, 2022
@VincentBu
Copy link
Contributor

Failed again in: runtime-coreclr libraries-jitstress 20220510.1

Failed test:

net7.0-windows-Release-x64-CoreCLR_checked-jitstress2_tiered-Windows.10.Amd64.Open

- System.Diagnostics.Tests.StackFrameTests.Ctor_SkipFrames(skipFrames: 1)
- System.Diagnostics.Tests.StackFrameTests.Ctor_SkipFrames(skipFrames: 0)
- System.Diagnostics.Tests.StackFrameTests.Ctor_Filename_LineNumber_ColNumber(fileName: \"FileName\", lineNumber: 1, columnNumber: 2)
- System.Diagnostics.Tests.StackFrameTests.Ctor_Filename_LineNumber_ColNumber(fileName: \"\", lineNumber: 0, columnNumber: -1)

net7.0-Linux-Release-x64-CoreCLR_checked-jitstress2_tiered-Ubuntu.1804.Amd64.Open

- System.Diagnostics.Tests.StackFrameTests.Ctor_SkipFrames(skipFrames: 1)
- System.Diagnostics.Tests.StackFrameTests.Ctor_SkipFrames(skipFrames: 0)

net7.0-Linux-Release-x64-CoreCLR_checked-jitstress2-Ubuntu.1804.Amd64.Open

- System.Diagnostics.Tests.StackFrameTests.Ctor_SkipFrames(skipFrames: 1)
- System.Diagnostics.Tests.StackFrameTests.Ctor_SkipFrames(skipFrames: 0)
- System.Diagnostics.Tests.StackFrameTests.Ctor_Filename_LineNumber_ColNumber(fileName: \"FileName\", lineNumber: 1, columnNumber: 2)
- System.Diagnostics.Tests.StackFrameTests.Ctor_Filename_LineNumber_ColNumber(fileName: \"\", lineNumber: 0, columnNumber: -1)

net7.0-windows-Release-arm64-CoreCLR_checked-jitstress1_tiered-Windows.10.Arm64v8.Open

- System.Diagnostics.Tests.StackFrameTests.Ctor_FNeedFileInfo(fNeedFileInfo: False)

net7.0-windows-Release-arm64-CoreCLR_checked-jitstress1-Windows.10.Arm64v8.Open

- System.Diagnostics.Tests.StackFrameTests.Ctor_FNeedFileInfo(fNeedFileInfo: False)

net7.0-Linux-Release-x64-CoreCLR_checked-jitstress1_tiered-Ubuntu.1804.Amd64.Open

- System.Diagnostics.Tests.StackFrameTests.Ctor_FNeedFileInfo(fNeedFileInfo: False)

net7.0-windows-Release-x64-CoreCLR_checked-jitstress1_tiered-Windows.10.Amd64.Open

- System.Diagnostics.Tests.StackFrameTests.Ctor_FNeedFileInfo(fNeedFileInfo: False)

net7.0-Linux-Release-x64-CoreCLR_checked-jitstress1-Ubuntu.1804.Amd64.Open

- System.Diagnostics.Tests.StackFrameTests.Ctor_FNeedFileInfo(fNeedFileInfo: False)

net7.0-windows-Release-x64-CoreCLR_checked-jitstress1-Windows.10.Amd64.Open

- System.Diagnostics.Tests.StackFrameTests.Ctor_FNeedFileInfo(fNeedFileInfo: False)

Error message:

Assert.Equal() Failure
Expected: -1
Actual:   68


Stack trace
   at System.Diagnostics.Tests.StackFrameTests.VerifyStackFrameSkipFrames(StackFrame stackFrame, Boolean isFileConstructor, Int32 skipFrames, MethodInfo expectedMethod, Boolean isCurrentFrame) in /_/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs:line 167
   at System.Diagnostics.Tests.StackFrameTests.VerifyStackFrame(StackFrame stackFrame, Boolean hasFileInfo, Int32 skipFrames, MethodInfo expectedMethod, Boolean isCurrentFrame) in /_/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs:line 159
   at InvokeStub_StackFrameTests.Ctor_SkipFrames(Object, Object, IntPtr*)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr) in /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs:line 60

or

Expected GetILOffset() -1 for InvokeStub_StackFrameTests.Ctor_SkipFrames at offset 42 in file:line:column <filename unknown>:0:0
to be greater or equal to zero.
Expected: True
Actual:   False


Stack trace
   at System.Diagnostics.Tests.StackFrameTests.VerifyStackFrameSkipFrames(StackFrame stackFrame, Boolean isFileConstructor, Int32 skipFrames, MethodInfo expectedMethod, Boolean isCurrentFrame) in /_/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs:line 171
   at System.Diagnostics.Tests.StackFrameTests.VerifyStackFrame(StackFrame stackFrame, Boolean hasFileInfo, Int32 skipFrames, MethodInfo expectedMethod, Boolean isCurrentFrame) in /_/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs:line 159
   at InvokeStub_StackFrameTests.Ctor_SkipFrames(Object, Object, IntPtr*)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr) in /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs:line 60

@ghost
Copy link

ghost commented May 11, 2022

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Seen on testing for #69117.
Pipeline: https://dev.azure.com/dnceng/public/_build/results?buildId=1762407&view=results
Test: https://dev.azure.com/dnceng/public/_build/results?buildId=1762407&view=ms.vss-test-web.build-test-results-tab&runId=47434070&resultId=110968&paneView=debug
Log: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-69117-merge-a31de8e595db4aa8af/System.Diagnostics.StackTrace.Tests/1/console.b93af7d3.log?helixlogtype=result

C:\h\w\BB8A09D2\w\AA4A094A\e>set COMPlus 
COMPlus_JitStress=1
COMPlus_TieredCompilation=0

C:\h\w\BB8A09D2\w\AA4A094A\e>call RunTests.cmd --runtime-path C:\h\w\BB8A09D2\p 
----- start Tue 05/10/2022 15:49:12.15 ===============  To repro directly: ===================================================== 
pushd C:\h\w\BB8A09D2\w\AA4A094A\e\
"C:\h\w\BB8A09D2\p\dotnet.exe" exec --runtimeconfig System.Diagnostics.StackTrace.Tests.runtimeconfig.json --depsfile System.Diagnostics.StackTrace.Tests.deps.json xunit.console.dll System.Diagnostics.StackTrace.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing 
popd
===========================================================================================================

C:\h\w\BB8A09D2\w\AA4A094A\e>"C:\h\w\BB8A09D2\p\dotnet.exe" exec --runtimeconfig System.Diagnostics.StackTrace.Tests.runtimeconfig.json --depsfile System.Diagnostics.StackTrace.Tests.deps.json xunit.console.dll System.Diagnostics.StackTrace.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
  Discovering: System.Diagnostics.StackTrace.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Diagnostics.StackTrace.Tests (found 53 test cases)
  Starting:    System.Diagnostics.StackTrace.Tests (parallel test collections = on, max threads = 4)
    System.Diagnostics.Tests.StackFrameTests.Ctor_FNeedFileInfo(fNeedFileInfo: False) [FAIL]
      Expected GetILOffset() -1 for InvokeStub_StackFrameTests.Ctor_FNeedFileInfo at offset 44 in file:line:column <filename unknown>:0:0
       to be greater or equal to zero.
      Expected: True
      Actual:   False
      Stack Trace:
        /_/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs(171,0): at System.Diagnostics.Tests.StackFrameTests.VerifyStackFrameSkipFrames(StackFrame stackFrame, Boolean isFileConstructor, Int32 skipFrames, MethodInfo expectedMethod, Boolean isCurrentFrame)
           at InvokeStub_StackFrameTests.Ctor_FNeedFileInfo(Object, Object, IntPtr*)
        /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs(60,0): at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
  Finished:    System.Diagnostics.StackTrace.Tests
=== TEST EXECUTION SUMMARY ===
   System.Diagnostics.StackTrace.Tests  Total: 121, Errors: 0, Failed: 1, Skipped: 0, Time: 10.512s
Author: jakobbotsch
Assignees: -
Labels:

arch-arm64, area-System.Diagnostics, os-linux, os-windows, JitStress, arch-x64, area-VM-coreclr, blocking-clean-ci-optional

Milestone: -

@ghost
Copy link

ghost commented May 15, 2022

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Seen on testing for #69117.
Pipeline: https://dev.azure.com/dnceng/public/_build/results?buildId=1762407&view=results
Test: https://dev.azure.com/dnceng/public/_build/results?buildId=1762407&view=ms.vss-test-web.build-test-results-tab&runId=47434070&resultId=110968&paneView=debug
Log: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-69117-merge-a31de8e595db4aa8af/System.Diagnostics.StackTrace.Tests/1/console.b93af7d3.log?helixlogtype=result

C:\h\w\BB8A09D2\w\AA4A094A\e>set COMPlus 
COMPlus_JitStress=1
COMPlus_TieredCompilation=0

C:\h\w\BB8A09D2\w\AA4A094A\e>call RunTests.cmd --runtime-path C:\h\w\BB8A09D2\p 
----- start Tue 05/10/2022 15:49:12.15 ===============  To repro directly: ===================================================== 
pushd C:\h\w\BB8A09D2\w\AA4A094A\e\
"C:\h\w\BB8A09D2\p\dotnet.exe" exec --runtimeconfig System.Diagnostics.StackTrace.Tests.runtimeconfig.json --depsfile System.Diagnostics.StackTrace.Tests.deps.json xunit.console.dll System.Diagnostics.StackTrace.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing 
popd
===========================================================================================================

C:\h\w\BB8A09D2\w\AA4A094A\e>"C:\h\w\BB8A09D2\p\dotnet.exe" exec --runtimeconfig System.Diagnostics.StackTrace.Tests.runtimeconfig.json --depsfile System.Diagnostics.StackTrace.Tests.deps.json xunit.console.dll System.Diagnostics.StackTrace.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
  Discovering: System.Diagnostics.StackTrace.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Diagnostics.StackTrace.Tests (found 53 test cases)
  Starting:    System.Diagnostics.StackTrace.Tests (parallel test collections = on, max threads = 4)
    System.Diagnostics.Tests.StackFrameTests.Ctor_FNeedFileInfo(fNeedFileInfo: False) [FAIL]
      Expected GetILOffset() -1 for InvokeStub_StackFrameTests.Ctor_FNeedFileInfo at offset 44 in file:line:column <filename unknown>:0:0
       to be greater or equal to zero.
      Expected: True
      Actual:   False
      Stack Trace:
        /_/src/libraries/System.Diagnostics.StackTrace/tests/StackFrameTests.cs(171,0): at System.Diagnostics.Tests.StackFrameTests.VerifyStackFrameSkipFrames(StackFrame stackFrame, Boolean isFileConstructor, Int32 skipFrames, MethodInfo expectedMethod, Boolean isCurrentFrame)
           at InvokeStub_StackFrameTests.Ctor_FNeedFileInfo(Object, Object, IntPtr*)
        /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs(60,0): at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
  Finished:    System.Diagnostics.StackTrace.Tests
=== TEST EXECUTION SUMMARY ===
   System.Diagnostics.StackTrace.Tests  Total: 121, Errors: 0, Failed: 1, Skipped: 0, Time: 10.512s
Author: jakobbotsch
Assignees: -
Labels:

arch-arm64, area-System.Diagnostics, area-System.Reflection, os-linux, os-windows, JitStress, arch-x64, area-VM-coreclr, blocking-clean-ci-optional

Milestone: -

@jkotas jkotas removed the blocking-clean-ci-optional Blocking optional rolling runs label May 15, 2022
@jkotas
Copy link
Member

jkotas commented May 15, 2022

@steveharter The reflection invoke PR was reverted in 69373. Please make sure that this and other known stacktrace related issues are addressed before resubmitting.

@steveharter
Copy link
Member

steveharter commented May 19, 2022

FWIW, in jitstress mode we make different inlining decisions, so that might be affecting things here.

Right, if you want to depend on certain frames showing up in stack traces make sure those frames involve NoInline methods. Otherwise random inlining (via jit stress), profile driven inlining, or changes to the jit can lead to spurious test failures.

Those tests with COMPlus_JitStress=1 are missing the stack frame for the actual method invoked, but contains the stack frame for the reflection-created DynamicMethod that calls the actual method. Either DynamicMethods are being JITed with COMPlus_JitStress=1 (which they shouldn't be) or the stack frame isn't there for some other reason.

Also the test failures with COMPlus_JitStress=1, at least locally, only occur in Checked\Debug builds, not Release.

@jkotas
Copy link
Member

jkotas commented May 19, 2022

Those tests with COMPlus_JitStress=1 are missing the stack frame for the actual method invoked, but contains the stack frame for the reflection-created DynamicMethod that calls the actual method.

The JIT is free to inline the actual method invoked into the reflection-created DynamicMethod given how things are setup today.

If we want to see the actual method invoked on the stack, we should call it using a function pointer. Calling the actual method invoked using a function pointer would also allow us to reuse the invokers between different methods with the same signature.

@steveharter
Copy link
Member

The JIT is free to inline the actual method invoked into the reflection-created DynamicMethod given how things are setup today.

My understanding is that DynamicMethods are never inlined but has been considered.

FWIW here's a small repro. It fails in a Debug build of the runtime + COMPlus_JitStress=1:

using System.Reflection;
using System.Reflection.Emit;

namespace ConsoleApp
{
    internal class Program
    {
        static void Main(string[] args)
        {

            MethodInfo mi = typeof(Foo).GetMethod(nameof(Foo.CallMe), BindingFlags.Public | BindingFlags.Static)!;
            Action action = CreateInvokeDelegate(mi);

            try
            {
                action();
            }
            catch (Exception e)
            {
                Console.WriteLine(e.ToString());
                Console.WriteLine("Call stack contains CallMe():" + e.ToString().Contains("CallMe()")); // should be True
            }
        }

        static Action CreateInvokeDelegate(MethodInfo method)
        {
            var dm = new DynamicMethod(
                "InvokeStub",
                returnType: null,
                parameterTypes: null,
                restrictedSkipVisibility: true);

            ILGenerator il = dm.GetILGenerator();
            il.Emit(OpCodes.Call, method);
            il.Emit(OpCodes.Ret);

            return (Action)dm.CreateDelegate(typeof(Action), target: null);
        }
    }

    public class Foo
    {
        public static void CallMe()
        {
            throw new Exception("Here");
        }
    }
}

@jkotas
Copy link
Member

jkotas commented May 19, 2022

My understanding is that DynamicMethods are never inlined

That's correct.

However, other (non-dynamic) methods can be inlined into dynamic methods. It is what's happening here.

@steveharter
Copy link
Member

Thanks I didn't catch the meaning of the "into the reflection-created DynamicMethod"

@steveharter
Copy link
Member

Yes using calli does work. This is preferred since it is a reasonable change to avoid a breaking change.

-il.Emit(OpCodes.Call, method);
+il.Emit(OpCodes.Ldftn, method);
+il.EmitCalli(OpCodes.Calli, CallingConventions.Standard, null, null, null);

I'll work on doing this along with a hashtable to re-use the invokers.

@jakobbotsch
Copy link
Member Author

-il.Emit(OpCodes.Call, method);
+il.Emit(OpCodes.Ldftn, method);
+il.EmitCalli(OpCodes.Calli, CallingConventions.Standard, null, null, null);

If the plan is to reuse the invoke stubs then I assume in the final implementation the function pointer will be more opaque to the JIT?
Note that we are expecting to do JIT work to optimize patterns like this one and make them inlinable, see #44610.

@steveharter
Copy link
Member

Note that we that we are expecting to do JIT work to optimize patterns like this one and make them inlinable

I was wondering if using calli to avoid the stack issue will eventually stop working based on future JIT changes. If that's the case, and it isn't desirable or feasible to add something like a NoTargetInline decl attribute then we should just declare this as a breaking change for when using Invoke. The stackwalk APIs including Assembly.GetCallingAssembly() are already documented to pretty much say don't count on it unless you attribute methods with MethodImplOptions.NoInlining; we'd need to expand that to include both direct calls and invoked calls which isn't too much of a stretch IMO.

If the plan is to reuse the invoke stubs then I assume in the final implementation the function pointer will be more opaque to the JIT?

The way I see it is that ldftn won't be used and we need to pass in the pointer directly to the emitted method. Will that prevent any future target inlining via #44610? For example:

        static void Main(string[] args)
        {

            MethodInfo mi = typeof(Foo).GetMethod(nameof(Foo.CallMe), BindingFlags.Public | BindingFlags.Static)!;
            IntPtr methodPtr = mi.MethodHandle.GetFunctionPointer();
            Action<IntPtr> action = CreateInvokeDelegate(mi);

            try
            {
                action(methodPtr);
            }
            catch (Exception e)
            {
                Console.WriteLine(e.ToString());
                Console.WriteLine("Call stack contains CallMe():" + e.ToString().Contains("CallMe()")); // should be True
            }
        }

        static Action<IntPtr> CreateInvokeDelegate(MethodInfo method)
        {
            var dm = new DynamicMethod(
                "InvokeStub",
                returnType: null,
                parameterTypes: new Type[] { typeof(IntPtr) },
                restrictedSkipVisibility: true);

            ILGenerator il = dm.GetILGenerator();
            il.Emit(OpCodes.Ldarg_0);
            il.EmitCalli(OpCodes.Calli, CallingConventions.Standard, typeof(void), Type.EmptyTypes, null);
            il.Emit(OpCodes.Ret);

            return (Action<IntPtr>)dm.CreateDelegate(typeof(Action<IntPtr>), target: null);
        }

@jakobbotsch
Copy link
Member Author

jakobbotsch commented May 19, 2022

The way I see it is that ldftn won't be used and we need to pass in the pointer directly to the emitted method. Will that prevent any future target inlining via #44610? For example:

Yes, this should be fine. The only problem I can think of is guarded devirtualization, but we do not ever expect to get PGO data for dynamic methods.

If that's the case, and it isn't desirable or feasible to add something like a NoTargetInline decl attribute then we should just declare this as a breaking change for when using Invoke.

We also have the System.StubHelpers.StubHelpers.NextCallReturnAddress intrinsic. The JIT guarantees that the next IL call/callvirt/calli/newobj will result in a call instruction that is not inlined or tailcalled when this is used. For your case it would be sufficient to call it and discard its result. However, I think given that the function pointer will be opaque to the JIT you should be fine without this.

@jakobbotsch
Copy link
Member Author

Fixed by #69575

@ghost ghost locked as resolved and limited conversation to collaborators Jun 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm64 arch-x64 area-System.Reflection JitStress CLR JIT issues involving JIT internal stress modes os-linux Linux OS (any supported distro) os-windows
Projects
None yet
Development

No branches or pull requests

5 participants