Fix behavior of inlined NDirect methods with MulticoreJit #55185

gbalykov · 2021-07-05T19:00:02Z

NDirect methods can be inlined in other methods during jitting (see impCheckForPInvokeCall). If this happens in mcj thread, it results in NDirect methods being loaded. This should not happen in mcj thread, because user can set custom native library search callbacks. In order to fix this, explicit flag JIT_FLAG_NO_PINVOKE_INLINE is set in mcj.
NDirect methods can be inlined in other methods during aot compilation. If such method is loaded in mcj thread, it results in NDirect method being loaded. Additionally, there's no way to control inlining during aot from runtime side, because r2r ni.dll might already exist. In order to fix this, runtime aborts loading if method has NDirect methods inlined in it and this all happens in mcj thread.

Without this change console helloworld segfaults in debug mode at least on x64.

kouvel · 2021-07-15T18:58:55Z

If this happens in mcj thread, it results in NDirect methods being loaded. This should not happen in mcj thread, because user can set custom native library search callbacks.
Without this change console helloworld segfaults in debug mode at least on x64.

What is different about NDirect methods being loaded on the MCJ thread as opposed to being loaded on other threads? I thought that the only difference should be in timing of when things are loaded.

Are the custom search paths for native libraries being set after the profile is started? Would it be possible to set them before starting the profile?

jkotas · 2021-07-15T19:07:38Z

Are the custom search paths for native libraries being set after the profile is started?

The user code can install handlers to resolve the native libraries using NativeLibrary.SetDllImportResolver or AssemblyLoadContext. We have no control over these.

kouvel · 2021-07-16T15:03:34Z

I see, I guess that would normally be done before the native libraries are loaded but MCJ could attempt that out of order.

Followup fix to #55185

#55864) - Port of #55839 to preview 7 - Followup fix to #55185

gbalykov · 2021-07-19T14:57:22Z

Actually, I've previously faced only the second part of the problem (when ni.dll exist). Recently, during mcj testing in debug mode I've faced this problem without ni.dll. On x64/armel JIT/Methodical/gc_poll/InsertGCPoll segfaults with next backtrace:

Thread 9 "corerun" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff0961700 (LWP 12024)]
0x00007ffff5e07303 in Frame::GetVTablePtr (this=0xffffffffffffffff) at /home/z/Dev/runtime/src/coreclr/vm/frames.h:682
682	        return VPTR_HOST_VTABLE_TO_TADDR(*(LPVOID*)this);
(gdb) bt
#0  0x00007ffff5e07303 in Frame::GetVTablePtr (this=0xffffffffffffffff) at /home/z/Dev/runtime/src/coreclr/vm/frames.h:682
#1  0x00007ffff6017847 in NDirectImportWorker (pMD=0x7fff7d091448) at /home/z/Dev/runtime/src/coreclr/vm/dllimport.cpp:6050
#2  0x00007ffff5e8d8c5 in NDirectMethodDesc::ResolveAndSetNDirectTarget (pMD=0x7fff7d091448) at /home/z/Dev/runtime/src/coreclr/vm/method.cpp:5199
#3  0x00007ffff5e8d9a7 in NDirectMethodDesc::TryResolveNDirectTargetForNoGCTransition (pMD=0x7fff7d091448, ndirectTarget=0x7ffff095dc60) at /home/z/Dev/runtime/src/coreclr/vm/method.cpp:5227
#4  0x00007ffff5e5c9b6 in CEEInfo::getAddressOfPInvokeTarget (this=0x7ffff095f790, method=0x7fff7d091448, pLookup=0x7ffff095dd08) at /home/z/Dev/runtime/src/coreclr/vm/jitinterface.cpp:10280
#5  0x00007ffff0c029c0 in Lowering::LowerNonvirtPinvokeCall (this=0x55555584e4b8, call=0x555555845238) at /home/z/Dev/runtime/src/coreclr/jit/lower.cpp:4502
#6  0x00007ffff0bfe219 in Lowering::LowerCall (this=0x55555584e4b8, node=0x555555845238) at /home/z/Dev/runtime/src/coreclr/jit/lower.cpp:1611
#7  0x00007ffff0bfbb37 in Lowering::LowerNode (this=0x55555584e4b8, node=0x555555845238) at /home/z/Dev/runtime/src/coreclr/jit/lower.cpp:169
#8  0x00007ffff0c09f27 in Lowering::LowerBlock (this=0x55555584e4b8, block=0x5555558443a8) at /home/z/Dev/runtime/src/coreclr/jit/lower.cpp:6134
#9  0x00007ffff0c09d39 in Lowering::DoPhase (this=0x55555584e4b8) at /home/z/Dev/runtime/src/coreclr/jit/lower.cpp:5901
#10 0x00007ffff0c93a15 in Phase::Run (this=0x55555584e4b8) at /home/z/Dev/runtime/src/coreclr/jit/phase.cpp:61
#11 0x00007ffff0a9a4a3 in Compiler::compCompile (this=0x555555842a28, methodCodePtr=0x7ffff095f140, methodCodeSize=0x7ffff095f57c, compileFlags=0x7ffff095f168)
    at /home/z/Dev/runtime/src/coreclr/jit/compiler.cpp:5172
#12 0x00007ffff0a9dcf0 in Compiler::compCompileHelper (this=0x555555842a28, classPtr=0x7fff7cf5a708, compHnd=0x7ffff095f790, methodInfo=0x7ffff095f5d8, methodCodePtr=0x7ffff095f140, 
    methodCodeSize=0x7ffff095f57c, compileFlags=0x7ffff095f168) at /home/z/Dev/runtime/src/coreclr/jit/compiler.cpp:6398
#13 0x00007ffff0a9c65c in Compiler::compCompile(CORINFO_MODULE_STRUCT_*, void**, unsigned int*, JitFlags*)::$_11::operator()(Compiler::compCompile(CORINFO_MODULE_STRUCT_*, void**, unsigned int*, JitFlags*)::__JITParam*) const (this=0x7ffff095e860, __JITpParam=0x7ffff095e868) at /home/z/Dev/runtime/src/coreclr/jit/compiler.cpp:5678
#14 0x00007ffff0a9c17b in Compiler::compCompile (this=0x555555842a28, classPtr=0x7fff7cf5a708, methodCodePtr=0x7ffff095f140, methodCodeSize=0x7ffff095f57c, compileFlags=0x7ffff095f168)
    at /home/z/Dev/runtime/src/coreclr/jit/compiler.cpp:5697
#15 0x00007ffff0aa39e3 in jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_14::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const::{lambda(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_14::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const::__JITParam*)#1}::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_14::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const::__JITParam*) const (this=0x7ffff095e9f0, __JITpParam=0x7ffff095e9f8) at /home/z/Dev/runtime/src/coreclr/jit/compiler.cpp:7044
#16 0x00007ffff0a9f104 in jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_14::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const (this=0x7ffff095eb08, __JITpParam=0x7ffff095eb18)
    at /home/z/Dev/runtime/src/coreclr/jit/compiler.cpp:7069
#17 0x00007ffff0a9ee43 in jitNativeCode (methodHnd=0x7fff7cf5d528, classPtr=0x7fff7cf5a708, compHnd=0x7ffff095f790, methodInfo=0x7ffff095f5d8, methodCodePtr=0x7ffff095f140, 
    methodCodeSize=0x7ffff095f57c, compileFlags=0x7ffff095f168, inlineInfoPtr=0x0) at /home/z/Dev/runtime/src/coreclr/jit/compiler.cpp:7071
#18 0x00007ffff0ab3378 in CILJit::compileMethod (this=0x7ffff11bac09 <operator new(unsigned long, CILJitSingletonAllocator const&)::CILJitBuff>, compHnd=0x7ffff095f790, methodInfo=0x7ffff095f5d8, 
    flags=4294967295, entryAddress=0x7ffff095f580, nativeSizeOfCode=0x7ffff095f57c) at /home/z/Dev/runtime/src/coreclr/jit/ee_il_dll.cpp:276
#19 0x00007ffff5e6611a in invokeCompileMethodHelper (jitMgr=0x5555557df480, comp=0x7ffff095f790, info=0x7ffff095f5d8, jitFlags=..., nativeEntry=0x7ffff095f580, nativeSizeOfCode=0x7ffff095f57c)
    at /home/z/Dev/runtime/src/coreclr/vm/jitinterface.cpp:12675
#20 0x00007ffff5e6624c in invokeCompileMethod (jitMgr=0x5555557df480, comp=0x7ffff095f790, info=0x7ffff095f5d8, jitFlags=..., nativeEntry=0x7ffff095f580, nativeSizeOfCode=0x7ffff095f57c)
    at /home/z/Dev/runtime/src/coreclr/vm/jitinterface.cpp:12740
#21 0x00007ffff5e67837 in UnsafeJitFunction (config=0x7ffff0960878, ILHeader=0x7ffff0960348, flags=..., pSizeOfCode=0x7ffff09604d4) at /home/z/Dev/runtime/src/coreclr/vm/jitinterface.cpp:13252
#22 0x00007ffff5ee4d96 in MethodDesc::JitCompileCodeLocked (this=0x7fff7cf5d528, pConfig=0x7ffff0960878, pEntry=0x5555558af000, pSizeOfCode=0x7ffff09604d4, pFlags=0x7ffff09604c0)
    at /home/z/Dev/runtime/src/coreclr/vm/prestub.cpp:1045
#23 0x00007ffff5ee490c in MethodDesc::JitCompileCodeLockedEventWrapper (this=0x7fff7cf5d528, pConfig=0x7ffff0960878, pEntry=0x5555558af000) at /home/z/Dev/runtime/src/coreclr/vm/prestub.cpp:914
#24 0x00007ffff5ee3d0b in MethodDesc::JitCompileCode (this=0x7fff7cf5d528, pConfig=0x7ffff0960878) at /home/z/Dev/runtime/src/coreclr/vm/prestub.cpp:854
---Type <return> to continue, or q <return> to quit---
#25 0x00007ffff5ee315f in MethodDesc::PrepareILBasedCode (this=0x7fff7cf5d528, pConfig=0x7ffff0960878) at /home/z/Dev/runtime/src/coreclr/vm/prestub.cpp:433
#26 0x00007ffff5ee2d76 in MethodDesc::PrepareCode (this=0x7fff7cf5d528, pConfig=0x7ffff0960878) at /home/z/Dev/runtime/src/coreclr/vm/prestub.cpp:332
#27 0x00007ffff6107a44 in MulticoreJitProfilePlayer::CompileMethodDesc (this=0x5555558a8d90, pModule=0x7fff7cf5a708, pMD=0x7fff7cf5d528) at /home/z/Dev/runtime/src/coreclr/vm/multicorejitplayer.cpp:596
#28 0x00007ffff6108aaf in MulticoreJitProfilePlayer::CompileMethodInfoRecord (this=0x5555558a8d90, pModule=0x7fff7cf5a708, pMethod=0x7fff7cf5d528, isGeneric=false)
    at /home/z/Dev/runtime/src/coreclr/vm/multicorejitplayer.cpp:994
#29 0x00007ffff6108957 in MulticoreJitProfilePlayer::HandleNonGenericMethodInfoRecord (this=0x5555558a8d90, moduleIndex=1, token=100663308)
    at /home/z/Dev/runtime/src/coreclr/vm/multicorejitplayer.cpp:895
#30 0x00007ffff6109b5a in MulticoreJitProfilePlayer::PlayProfile (this=0x5555558a8d90) at /home/z/Dev/runtime/src/coreclr/vm/multicorejitplayer.cpp:1301
#31 0x00007ffff6109e86 in MulticoreJitProfilePlayer::JITThreadProc (this=0x5555558a8d90, pThread=0x5555558b16b0) at /home/z/Dev/runtime/src/coreclr/vm/multicorejitplayer.cpp:1370
#32 0x00007ffff610a26e in MulticoreJitProfilePlayer::StaticJITThreadProc (args=0x5555558a8d90) at /home/z/Dev/runtime/src/coreclr/vm/multicorejitplayer.cpp:1421
#33 0x00007ffff66b09d5 in CorUnix::CPalThread::ThreadEntry (pvParam=0x5555558b26e0) at /home/z/Dev/runtime/src/coreclr/pal/src/thread/thread.cpp:1862
#34 0x00007ffff79b76db in start_thread (arg=0x7ffff0961700) at pthread_create.c:463
#35 0x00007ffff6da171f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

With this PR segfault is fixed.

kouvel · 2021-07-19T17:59:40Z

src/coreclr/jit/importer.cpp

Suggested change

(!opts.compNoPInvokeInline) // profiler is preventing inline pinvoke

(!opts.compNoPInvokeInline) // JIT flag is preventing inline pinvoke

kouvel · 2021-07-19T18:22:33Z

src/coreclr/debug/daccess/nidump.h

It doesn't look like the new parameters are used, are they necessary?

These new parameters are indeed not used in NativeImageDumper::HandleFixupForHistogram, NativeImageDumper::HandleFixupForMethodDump. They are required, because Module::FixupDelayListAux should pass this flag to Module::FixupNativeEntry, and Module::FixupDelayListAux is used for all three to traverse over fixups.

Oh ok, it would be good to give it the same name so that it's clear what that parameter is a placeholder for, even if unused

kouvel

I think it would be good to have a config option that allows disabling preloading the p/invokes on the mcj thread. By default and when custom paths are not used for native libraries, it may be beneficial to do the previous thing. Could add an mcj config option, preload it in EEConfig, and use it via g_pConfig where the decisions are made such that the changes are opt-in.

Considering making it opt-in

jkotas · 2021-07-20T18:28:46Z

How are people going to tell whether PInvokes in all libraries that they use are compatible with preloading?

Note that multicore JIT was skipping PInvokes up until recently. This is fixing a functional regression introduced by adding PInvoke handling to multicore JIT. I do not think we need an configuration option to control this behavior.

gbalykov · 2021-07-20T18:36:43Z

@kouvel @jkotas I'm not 100% sure, but it seems that this is not directly related to ndirect handling in mcj, because ndirect handling was disabled back in #48326. This ndirect inlining issue (when there are no ni.dll) seems to be present in mcj for a long time. I also agree, that this change should at least be enabled by default.

jkotas · 2021-07-20T18:37:04Z

src/coreclr/vm/jitinterface.cpp

I do not think you need to suppress this one. Resolving ENCODE_INDIRECT_PINVOKE_TARGET should not be loading any unmanaged .dlls.

(The suppression for ENCODE_PINVOKE_TARGET should stay.)

kouvel · 2021-07-20T18:37:59Z

Sounds good, we can keep this on by default

kouvel · 2021-07-20T18:46:28Z

How are people going to tell whether PInvokes in all libraries that they use are compatible with preloading?

Was thinking for self-contained publish, would it be reasonable for a user to expect that all the native library dependencies would be in the default paths?

gbalykov · 2021-07-20T19:02:10Z

Was thinking for self-contained publish, would it be reasonable for a user to expect that all the native library dependencies would be in the default paths?

As far as I understand, no. For example, crossgen2 is published as self-contained and it does similar trick, when "clrjitilc" is replaced with actually used lib in runtime using NativeLibrary.SetDllImportResolver.

kouvel · 2021-07-20T19:45:41Z

I'm not familiar with the crossgen2 particulars. Would it be a common issue in customer scenarios with self-contained publish (or even with shared framework cases that I expect would resolve the native dependencies through default paths)? I would understand if it's not worthy of a config option, I'm just wondering. Don't want to block on this though.

kouvel · 2021-07-20T19:47:15Z

@gbalykov could you please add new changes to new commits in the future? It would make it much easier to review the changes.

gbalykov · 2021-07-20T19:53:19Z

@kouvel sure, sorry for inconvenience

jkotas · 2021-07-20T21:01:19Z

NDirect methods can be inlined in other methods during jitting (see impCheckForPInvokeCall). If this happens in mcj thread, it results in NDirect methods being loaded.

I do not actually understand how this is possible for regular PInvokes. Is it only a problem for PInvokes with SuppressGCTransitionAttribute?

gbalykov · 2021-07-20T21:44:04Z

Is it only a problem for PInvokes with SuppressGCTransitionAttribute?

Is doesn't look so. Compiler::impCheckForPInvokeCall has complicated condition in it and SuppressGCTransitionAttribute doesn't seem to have any effect on outcome. In order for inline to work out unmanaged calling convention should be supported, basic block should be supported and also no marshalling should be needed. Any return from Compiler::impCheckForPInvokeCall before call->gtFlags |= GTF_CALL_UNMANAGED; assignment forbids pinvoke inline.

jkotas · 2021-07-21T11:16:13Z

What is the callstack originating in Compiler::impCheckForPInvokeCall that ends up loading the unmanaged library?

gbalykov · 2021-07-21T12:30:57Z

@jkotas This is the call stack for JIT/Methodical/gc_poll/InsertGCPoll on x64 with mcj without this PR. This call stack happens in mcj thread:

Thread 9 "corerun" hit Breakpoint 1, Compiler::impCheckForPInvokeCall (this=0x555555868258, call=0x55555586aa68, methHnd=0x7fff7d081448, sig=0x7fffec826860, mflags=34078744, block=0x555555869bd8)
    at /home/z/Dev/runtime/src/coreclr/jit/importer.cpp:7322
7322	    call->gtFlags |= GTF_CALL_UNMANAGED;
(gdb) bt
#0  Compiler::impCheckForPInvokeCall (this=0x555555868258, call=0x55555586aa68, methHnd=0x7fff7d081448, sig=0x7fffec826860, mflags=34078744, block=0x555555869bd8)
    at /home/z/Dev/runtime/src/coreclr/jit/importer.cpp:7322
#1  0x00007fffec99afaf in Compiler::impImportCall (this=0x555555868258, opcode=CEE_CALL, pResolvedToken=0x7fffec826758, pConstrainedResolvedToken=0x0, newobjThis=0x0, prefixFlags=0, 
    callInfo=0x7fffec826850, rawILOffset=10) at /home/z/Dev/runtime/src/coreclr/jit/importer.cpp:8673
#2  0x00007fffec9ad04c in Compiler::impImportBlockCode (this=0x555555868258, block=0x555555869bd8) at /home/z/Dev/runtime/src/coreclr/jit/importer.cpp:14583
#3  0x00007fffec9b879d in Compiler::impImportBlock(BasicBlock*)::$_1::operator()(Compiler::impImportBlock(BasicBlock*)::FilterVerificationExceptionsParam*) const (this=0x7fffec826c40, 
    pParam=0x7fffec826c50) at /home/z/Dev/runtime/src/coreclr/jit/importer.cpp:17688
#4  0x00007fffec9b76b4 in Compiler::impImportBlock (this=0x555555868258, block=0x555555869bd8) at /home/z/Dev/runtime/src/coreclr/jit/importer.cpp:17698
#5  0x00007fffec9b9f36 in Compiler::impImport (this=0x555555868258) at /home/z/Dev/runtime/src/coreclr/jit/importer.cpp:18771
#6  0x00007fffec92a939 in Compiler::fgImport (this=0x555555868258) at /home/z/Dev/runtime/src/coreclr/jit/flowgraph.cpp:625
#7  0x00007fffec8c280b in CompilerPhaseWithStatus::DoPhase (this=0x7fffec826f38) at /home/z/Dev/runtime/src/coreclr/jit/phase.h:124
#8  0x00007fffecaaba15 in Phase::Run (this=0x7fffec826f38) at /home/z/Dev/runtime/src/coreclr/jit/phase.cpp:61
#9  0x00007fffec8bff3f in DoPhase (_compiler=0x555555868258, _phase=PHASE_IMPORTATION, _action=(PhaseStatus (Compiler::*)(Compiler * const)) 0x7fffec92a910 <Compiler::fgImport()>)
    at /home/z/Dev/runtime/src/coreclr/jit/phase.h:136
#10 0x00007fffec8b102a in Compiler::compCompile (this=0x555555868258, methodCodePtr=0x7fffec828140, methodCodeSize=0x7fffec82857c, compileFlags=0x7fffec828168)
    at /home/z/Dev/runtime/src/coreclr/jit/compiler.cpp:4505
#11 0x00007fffec8b5cf0 in Compiler::compCompileHelper (this=0x555555868258, classPtr=0x7fff7cf4a6c8, compHnd=0x7fffec828790, methodInfo=0x7fffec8285d8, methodCodePtr=0x7fffec828140, 
    methodCodeSize=0x7fffec82857c, compileFlags=0x7fffec828168) at /home/z/Dev/runtime/src/coreclr/jit/compiler.cpp:6398
#12 0x00007fffec8b465c in Compiler::compCompile(CORINFO_MODULE_STRUCT_*, void**, unsigned int*, JitFlags*)::$_11::operator()(Compiler::compCompile(CORINFO_MODULE_STRUCT_*, void**, unsigned int*, JitFlags*)::__JITParam*) const (this=0x7fffec827860, __JITpParam=0x7fffec827868) at /home/z/Dev/runtime/src/coreclr/jit/compiler.cpp:5678
#13 0x00007fffec8b417b in Compiler::compCompile (this=0x555555868258, classPtr=0x7fff7cf4a6c8, methodCodePtr=0x7fffec828140, methodCodeSize=0x7fffec82857c, compileFlags=0x7fffec828168)
    at /home/z/Dev/runtime/src/coreclr/jit/compiler.cpp:5697
#14 0x00007fffec8bb9e3 in jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_14::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const::{lambda(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_14::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const::__JITParam*)#1}::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_14::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const::__JITParam*) const (this=0x7fffec8279f0, __JITpParam=0x7fffec8279f8) at /home/z/Dev/runtime/src/coreclr/jit/compiler.cpp:7044
#15 0x00007fffec8b7104 in jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_14::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const (this=0x7fffec827b08, __JITpParam=0x7fffec827b18)
    at /home/z/Dev/runtime/src/coreclr/jit/compiler.cpp:7069
#16 0x00007fffec8b6e43 in jitNativeCode (methodHnd=0x7fff7cf4d4e8, classPtr=0x7fff7cf4a6c8, compHnd=0x7fffec828790, methodInfo=0x7fffec8285d8, methodCodePtr=0x7fffec828140, 
    methodCodeSize=0x7fffec82857c, compileFlags=0x7fffec828168, inlineInfoPtr=0x0) at /home/z/Dev/runtime/src/coreclr/jit/compiler.cpp:7071
#17 0x00007fffec8cb378 in CILJit::compileMethod (this=0x7fffecfd2c09 <operator new(unsigned long, CILJitSingletonAllocator const&)::CILJitBuff>, compHnd=0x7fffec828790, methodInfo=0x7fffec8285d8, 
    flags=4294967295, entryAddress=0x7fffec828580, nativeSizeOfCode=0x7fffec82857c) at /home/z/Dev/runtime/src/coreclr/jit/ee_il_dll.cpp:276
#18 0x00007ffff5e6611a in invokeCompileMethodHelper (jitMgr=0x5555557df480, comp=0x7fffec828790, info=0x7fffec8285d8, jitFlags=..., nativeEntry=0x7fffec828580, nativeSizeOfCode=0x7fffec82857c)
    at /home/z/Dev/runtime/src/coreclr/vm/jitinterface.cpp:12675
#19 0x00007ffff5e6624c in invokeCompileMethod (jitMgr=0x5555557df480, comp=0x7fffec828790, info=0x7fffec8285d8, jitFlags=..., nativeEntry=0x7fffec828580, nativeSizeOfCode=0x7fffec82857c)
    at /home/z/Dev/runtime/src/coreclr/vm/jitinterface.cpp:12740
#20 0x00007ffff5e67837 in UnsafeJitFunction (config=0x7fffec829878, ILHeader=0x7fffec829348, flags=..., pSizeOfCode=0x7fffec8294d4) at /home/z/Dev/runtime/src/coreclr/vm/jitinterface.cpp:13252
#21 0x00007ffff5ee4d96 in MethodDesc::JitCompileCodeLocked (this=0x7fff7cf4d4e8, pConfig=0x7fffec829878, pEntry=0x55555589b9e0, pSizeOfCode=0x7fffec8294d4, pFlags=0x7fffec8294c0)
---Type <return> to continue, or q <return> to quit---
    at /home/z/Dev/runtime/src/coreclr/vm/prestub.cpp:1045
#22 0x00007ffff5ee490c in MethodDesc::JitCompileCodeLockedEventWrapper (this=0x7fff7cf4d4e8, pConfig=0x7fffec829878, pEntry=0x55555589b9e0) at /home/z/Dev/runtime/src/coreclr/vm/prestub.cpp:914
#23 0x00007ffff5ee3d0b in MethodDesc::JitCompileCode (this=0x7fff7cf4d4e8, pConfig=0x7fffec829878) at /home/z/Dev/runtime/src/coreclr/vm/prestub.cpp:854
#24 0x00007ffff5ee315f in MethodDesc::PrepareILBasedCode (this=0x7fff7cf4d4e8, pConfig=0x7fffec829878) at /home/z/Dev/runtime/src/coreclr/vm/prestub.cpp:433
#25 0x00007ffff5ee2d76 in MethodDesc::PrepareCode (this=0x7fff7cf4d4e8, pConfig=0x7fffec829878) at /home/z/Dev/runtime/src/coreclr/vm/prestub.cpp:332
#26 0x00007ffff6107a44 in MulticoreJitProfilePlayer::CompileMethodDesc (this=0x5555558a8d90, pModule=0x7fff7cf4a6c8, pMD=0x7fff7cf4d4e8) at /home/z/Dev/runtime/src/coreclr/vm/multicorejitplayer.cpp:596
#27 0x00007ffff6108aaf in MulticoreJitProfilePlayer::CompileMethodInfoRecord (this=0x5555558a8d90, pModule=0x7fff7cf4a6c8, pMethod=0x7fff7cf4d4e8, isGeneric=false)
    at /home/z/Dev/runtime/src/coreclr/vm/multicorejitplayer.cpp:994
#28 0x00007ffff6108957 in MulticoreJitProfilePlayer::HandleNonGenericMethodInfoRecord (this=0x5555558a8d90, moduleIndex=1, token=100663308)
    at /home/z/Dev/runtime/src/coreclr/vm/multicorejitplayer.cpp:895
#29 0x00007ffff6109b5a in MulticoreJitProfilePlayer::PlayProfile (this=0x5555558a8d90) at /home/z/Dev/runtime/src/coreclr/vm/multicorejitplayer.cpp:1301
#30 0x00007ffff6109e86 in MulticoreJitProfilePlayer::JITThreadProc (this=0x5555558a8d90, pThread=0x5555558b21d0) at /home/z/Dev/runtime/src/coreclr/vm/multicorejitplayer.cpp:1370
#31 0x00007ffff610a26e in MulticoreJitProfilePlayer::StaticJITThreadProc (args=0x5555558a8d90) at /home/z/Dev/runtime/src/coreclr/vm/multicorejitplayer.cpp:1421
#32 0x00007ffff66b09d5 in CorUnix::CPalThread::ThreadEntry (pvParam=0x5555558b3200) at /home/z/Dev/runtime/src/coreclr/pal/src/thread/thread.cpp:1862
#33 0x00007ffff79b76db in start_thread (arg=0x7fffec82a700) at pthread_create.c:463
#34 0x00007ffff6da171f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
(gdb) fr 22
#22 0x00007ffff5ee490c in MethodDesc::JitCompileCodeLockedEventWrapper (this=0x7fff7cf4d4e8, pConfig=0x7fffec829878, pEntry=0x55555589b9e0) at /home/z/Dev/runtime/src/coreclr/vm/prestub.cpp:914
914	        pCode = JitCompileCodeLocked(pConfig, pEntry, &sizeOfCode, &flags);
(gdb) p *this
$1 = {static ALIGNMENT_SHIFT = 3, static ALIGNMENT = 8, static ALIGNMENT_MASK = 7, static s_ClassificationSizeTable = 
    0x7ffff675ca70 <MethodDesc::s_ClassificationSizeTable> "08h@@H0P8@pHHP8X@HxPPX@`HP\200XX`Hh8@pHHP8X@HxPPX@`HP\200XX`HhPX\210``hPp\326Hs\377\352Hs\377\tJs\377\tJs\377\tJs\377\376Hs\377\tJs\377\tJs\377\tJs\377\tJs\377\321Hs\377d", static m_GCCoverCrst = {<CrstBase> = {m_criticalsection = {DebugInfo = 0x555555805890, LockCount = 0, RecursionCount = 0, OwningThread = 0x0, SpinCount = 0, 
        dwInitState = 1, csnds = {rgNativeDataStorage = '\000' <repeats 95 times>, pvAlign = 0x0}}, m_dwFlags = 3221225472, m_entercount = 0, m_crstType = CrstGCCover, 
      m_tag = 0x7ffff674c3b5 "CrstGCCover", m_crstlevel = 10, m_holderthreadid = {m_FiberPtrId = 0x0}, m_next = 0x0, m_prev = 0x0, m_cannotLeave = {m_val = 0}, 
      m_countNoTriggerGC = 0}, <No data fields>}, m_pszDebugMethodName = 0x7fff7cfbd1a0 "LoopOn64", m_pszDebugClassName = 0x7fff7cf4d248 "InsertGCPoll", 
  m_pszDebugMethodSignature = 0x7fff7cfbd3d0 "void *()", m_pDebugMethodTable = {static isRelative = false, m_ptr = 140735289808320}, m_GcCover = 0x0, m_wFlags3AndTokenRemainder = 12, 
  m_chunkIndex = 74 'J', m_bFlags2 = 35 '#', m_wSlotNumber = 14, m_wFlags = 168}

After that segfault with call stack mentioned in #55185 (comment) happens.

jkotas · 2021-07-21T13:09:49Z

After that segfault with call stack mentioned in #55185 (comment)

This callstack is for SuppressGCTransitionAttribute method (NDirectMethodDesc::TryResolveNDirectTargetForNoGCTransition is only called for methods with SuppressGCTransitionAttribute).

So disabling the PInvoke inlining for multicore JIT solves this problem, but I think it is too conservative. It really only needs to be disabled for the methods with SuppressGCTransitionAttribute.

gbalykov · 2021-07-21T13:45:48Z

It really only needs to be disabled for the methods with SuppressGCTransitionAttribute

Can't this lead to problems in future? Approach with disallowed inlining only for ndirect methods with SuppressGCTransitionAttribute requires sync between code in CEEInfo::getAddressOfPInvokeTarget and Compiler::impCheckForPInvokeCall (Compiler::impCheckForPInvokeCall will return before call->gtFlags |= GTF_CALL_UNMANAGED if SuppressGCTransitionAttribute is set). If NDirectMethodDesc::ResolveAndSetNDirectTarget for some reason becomes invoked for ndirect methods without SuppressGCTransitionAttribute in mcj thread, behavior would be incorrect, and we might not see a problem for a while.

jkotas · 2021-07-21T14:23:07Z

There is a ton of code out there that depends on targets for regular PInvokes to be resolved right as the PInvoke is called for the first time, and no earlier. Changing the JIT to resolve targets of regular PInvokes eagerly would be a major breaking change.

gbalykov · 2021-07-21T14:26:56Z

@jkotas ok, thanks for clarification. I'll update first part of this PR by reverting JIT_FLAG_NO_PINVOKE_INLINE change and adding inlining skipping based on SuppressGCTransitionAttribute.

gbalykov · 2021-07-21T16:36:07Z

@jkotas I've updated PR, JIT_FLAG_MCJIT_BACKGROUND is used to identify mcj compilation and only ndirect methods with SuppressGCTransitionAttribute are disallowed for inlining.

src/coreclr/jit/importer.cpp

jkotas · 2021-07-21T20:49:46Z

src/coreclr/jit/importer.cpp

It is less than idea to be hardcoding policy like this in the JIT. I think it would be better to implement this policy on the VM side in the pInvokeMarshalingRequired method instead.

What about passing explicit flag that tells jit to skip inlining for pinvokes with SuppressGCTransitionAttribute in mcj thread like in gbalykov@124bcaf?

pInvokeMarshalingRequired approach will require change with additional arg passing, because on vm side we don't know whether this is mcj thread compilation or not:

bool CEEInfo::pInvokeMarshalingRequired(CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO* callSiteSig, bool isMCJ)

If it's useful it is possible to get the PrepareCodeConfig for the in-progress compilation in callbacks to the VM like this:

runtime/src/coreclr/vm/jitinterface.cpp

Lines 6910 to 6912 in 4bab779

PrepareCodeConfig *config = GetThread()->GetCurrentPrepareCodeConfig();

if (config != nullptr)

{

@kouvel thanks a lot, that's a piece of logic I was looking for! I've updated PR.

jkotas

LGTM

NDirect methods can be inlined in other methods during jitting (see `impCheckForPInvokeCall`). If this happens in mcj thread, it results in NDirect methods being loaded. This should not happen in mcj thread, because user can set custom native library search callbacks. In order to fix this, explicit flag JIT_FLAG_NO_PINVOKE_INLINE is set in mcj.

… they have NDirect methods inlined in them NDirect methods can be inlined in other methods during aot compilation. If such method is loaded in mcj thread, it results in NDirect method being loaded. Additionally, there's no way to control inlining during aot from runtime side, because r2r ni.dll might already exist. In order to fix this, runtime aborts loading if method has NDirect methods inlined in it and this all happens in mcj thread.

This reverts commit 59102a8.

… in MulticoreJit thread

gbalykov · 2021-08-02T12:23:30Z

I've rebased this on newest main, there were no conflicts and all tests have passed. Can this be merged?

jkotas · 2021-08-02T14:13:48Z

Thank you!

gbalykov · 2021-08-04T04:34:14Z

Thanks!

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 5, 2021

jkotas added area-TieredCompilation-coreclr and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Jul 6, 2021

jkotas requested a review from kouvel July 6, 2021 03:03

kouvel mentioned this pull request Jul 16, 2021

Fix debugger thread context validation after recent change #55839

Merged

kouvel added a commit that referenced this pull request Jul 17, 2021

Fix debugger thread context validation after recent change (#55839)

10f38d2

Followup fix to #55185

kouvel mentioned this pull request Jul 17, 2021

[Preview 7] Fix debugger thread context validation after recent change #55864

Merged

stephentoub pushed a commit that referenced this pull request Jul 18, 2021

[Preview 7] Fix debugger thread context validation after recent change (

8d3afa3

#55864) - Port of #55839 to preview 7 - Followup fix to #55185

terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021

kouvel previously approved these changes Jul 20, 2021

View reviewed changes

gbalykov force-pushed the fix-ndirect-inlined-usage branch from 8d9e881 to 370d1fc Compare July 20, 2021 07:12

kouvel reviewed Jul 20, 2021

View reviewed changes

kouvel self-requested a review July 20, 2021 18:23

gbalykov force-pushed the fix-ndirect-inlined-usage branch from 370d1fc to a0a98d3 Compare July 20, 2021 18:31

jkotas reviewed Jul 20, 2021

View reviewed changes

gbalykov force-pushed the fix-ndirect-inlined-usage branch from a0a98d3 to 2482af1 Compare July 20, 2021 19:09

jkotas approved these changes Jul 20, 2021

View reviewed changes

kouvel approved these changes Jul 21, 2021

View reviewed changes

kouvel reviewed Jul 21, 2021

View reviewed changes

src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved

kouvel closed this Jul 21, 2021

kouvel reopened this Jul 21, 2021

jkotas reviewed Jul 21, 2021

View reviewed changes

gbalykov force-pushed the fix-ndirect-inlined-usage branch 2 times, most recently from 013c32f to 3fe3a3a Compare July 22, 2021 19:29

jkotas approved these changes Jul 22, 2021

View reviewed changes

gbalykov added 4 commits August 1, 2021 15:04

Revert "Forbid inlining of pinvoke methods in MulticoreJit thread"

2e8f598

This reverts commit 59102a8.

Forbid inlining of pinvoke methods with SuppressGCTransitionAttribute…

ecc436f

… in MulticoreJit thread

gbalykov force-pushed the fix-ndirect-inlined-usage branch from 3fe3a3a to ecc436f Compare August 1, 2021 12:04

jkotas merged commit aa4290f into dotnet:main Aug 2, 2021

ghost locked as resolved and limited conversation to collaborators Sep 3, 2021

	(!opts.compNoPInvokeInline) // profiler is preventing inline pinvoke
	(!opts.compNoPInvokeInline) // JIT flag is preventing inline pinvoke

	PrepareCodeConfig *config = GetThread()->GetCurrentPrepareCodeConfig();
	if (config != nullptr)
	{

Fix behavior of inlined NDirect methods with MulticoreJit #55185

Fix behavior of inlined NDirect methods with MulticoreJit #55185

Uh oh!

Conversation

gbalykov commented Jul 5, 2021

Uh oh!

kouvel commented Jul 15, 2021

Uh oh!

jkotas commented Jul 15, 2021

Uh oh!

kouvel commented Jul 16, 2021

Uh oh!

gbalykov commented Jul 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kouvel left a comment

Choose a reason for hiding this comment

Uh oh!

jkotas commented Jul 20, 2021

Uh oh!

gbalykov commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kouvel commented Jul 20, 2021

Uh oh!

kouvel commented Jul 20, 2021

Uh oh!

gbalykov commented Jul 20, 2021

Uh oh!

kouvel commented Jul 20, 2021

Uh oh!

kouvel commented Jul 20, 2021

Uh oh!

gbalykov commented Jul 20, 2021

Uh oh!

jkotas commented Jul 20, 2021

Uh oh!

gbalykov commented Jul 20, 2021

Uh oh!

jkotas commented Jul 21, 2021

Uh oh!

gbalykov commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gbalykov commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Jul 21, 2021

Uh oh!

gbalykov commented Jul 21, 2021

Uh oh!

gbalykov commented Jul 21, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

gbalykov commented Jul 20, 2021 •

edited

Loading

gbalykov commented Jul 21, 2021 •

edited

Loading

jkotas commented Jul 21, 2021 •

edited

Loading

gbalykov commented Jul 21, 2021 •

edited

Loading