Stepping into multidimensional array of tuples in debug causes internal CLR error

[Zidbrain]

Description

Trying to step into any statement involving getting value from multidimensional array of tuples causes System.ExecutionEngineException and subsequently internal CLR error. However stepping over or checking the value through locals window does not.

Configuration

• .Net Core 3.1 (also happens in 3.0)
• Windows
• Any CPU

Code example

var array = new (int a, bool b)[2, 2];

array[0, 0] = (1, true); // Stepping in causes Internal CLR error. (0x80131506)
var k = array[0, 0]; // Stepping in causes Internal CLR error. (0x80131506)
var c = array[1, 1].a; // Stepping in causes Internal CLR error. (0x80131506)

[Dotnet-GitSync-Bot]

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.

Tagging subscribers to this area: @tommcdon
Notify danmosemsft if you want to be subscribed.

[tommcdon]

@sdmaclea

[Gnbrkm41]

Can also reproduce with 5.0.100-preview.8.20351.5 with VS2019 16.7 P3.1. This happens with "Just My Code" disabled.

[Gnbrkm41]

From what I can see, it enters the ILStubClass.IL_STUB_Array_Set method and there's a breakpoint (INT 3) at the very beginning of the method; I presume this is because we asked to step in? From what I see CLR tries to hand this over to the debugger, but in ILStubManager::TraceManager it looks like it tries to dereference an invalid pointer and this gets propagated as an EEE. I can provide dump of this thing if it can't be repro'd.

[hoyosjs]

Thanks @Gnbrkm41 I also see this and the int 3 in that stub. I see this too:

Stack for assert

16 000000ea`0dfb6560 00007ffe`54e927fc     coreclr!EEPolicy::LogFatalError+0x7d9 [E:\repos\runtime3\src\coreclr\src\vm\eepolicy.cpp @ 568] 
17 000000ea`0dfb7600 00007ffe`54b5d2db     coreclr!EEPolicy::HandleFatalError+0x1ec [E:\repos\runtime3\src\coreclr\src\vm\eepolicy.cpp @ 779] 
18 000000ea`0dfb7cd0 00007ffe`54b5cb34     coreclr!CLRVectoredExceptionHandlerPhase3+0x42b [E:\repos\runtime3\src\coreclr\src\vm\excep.cpp @ 7508] 
19 000000ea`0dfb8020 00007ffe`54b5ca56     coreclr!CLRVectoredExceptionHandlerPhase2+0xa4 [E:\repos\runtime3\src\coreclr\src\vm\excep.cpp @ 7220] 
1a 000000ea`0dfb82c0 00007ffe`54b5d66b     coreclr!CLRVectoredExceptionHandler+0x2a6 [E:\repos\runtime3\src\coreclr\src\vm\excep.cpp @ 7181] 
1b 000000ea`0dfb8490 00007ffe`f1207f4c     coreclr!CLRVectoredExceptionHandlerShim+0x2cb [E:\repos\runtime3\src\coreclr\src\vm\excep.cpp @ 7922] 
1c 000000ea`0dfb8590 00007ffe`f11db296     ntdll!RtlpCallVectoredHandlers+0x108 [minkernel\ntdll\vectxcpt.c @ 204] 
1d (Inline Function) --------`--------     ntdll!RtlCallVectoredExceptionHandlers+0xe [minkernel\ntdll\vectxcpt.c @ 345] 
1e 000000ea`0dfb8630 00007ffe`f122ec1e     ntdll!RtlDispatchException+0x66 [minkernel\ntos\rtl\amd64\exdsptch.c @ 357] 
1f 000000ea`0dfb8840 00007ffe`546d56c3     ntdll!KiUserExceptionDispatch+0x2e [minkernel\ntos\rtl\amd64\trampoln.asm @ 749] 
20 000000ea`0dfb8fc0 00007ffe`549cb2a0     coreclr!MethodDesc::GetClassification+0x13 [E:\repos\runtime3\src\coreclr\src\vm\method.hpp @ 1872] 
21 000000ea`0dfb8fd0 00007ffe`54d46169     coreclr!MethodDesc::IsNDirect+0x30 [E:\repos\runtime3\src\coreclr\src\vm\method.hpp @ 656] 
22 000000ea`0dfb9010 00007ffe`54b48bbd     coreclr!ILStubManager::TraceManager+0x3e9 [E:\repos\runtime3\src\coreclr\src\vm\stubmgr.cpp @ 1801] 
23 000000ea`0dfb90b0 00007ffe`546ea31c     coreclr!EEDbgInterfaceImpl::TraceManager+0x2fd [E:\repos\runtime3\src\coreclr\src\vm\eedbginterfaceimpl.cpp @ 1262] 
24 000000ea`0dfb9420 00007ffe`546df6dc     coreclr!DebuggerStepper::TriggerPatch+0x5dc [E:\repos\runtime3\src\coreclr\src\debug\ee\controller.cpp @ 7101] 
25 000000ea`0dfbb4d0 00007ffe`546d23e4     coreclr!DebuggerController::ScanForTriggers+0xabc [E:\repos\runtime3\src\coreclr\src\debug\ee\controller.cpp @ 2643] 
26 000000ea`0dfbba20 00007ffe`546d16f1     coreclr!DebuggerController::DispatchPatchOrSingleStep+0xaa4 [E:\repos\runtime3\src\coreclr\src\debug\ee\controller.cpp @ 2927] 
27 000000ea`0dfbc080 00007ffe`54704df8     coreclr!DebuggerController::DispatchNativeException+0xaf1 [E:\repos\runtime3\src\coreclr\src\debug\ee\controller.cpp @ 4250] 
28 000000ea`0dfbc540 00007ffe`54b6fdd8     coreclr!Debugger::FirstChanceNativeException+0x4e8 [E:\repos\runtime3\src\coreclr\src\debug\ee\debugger.cpp @ 5684] 
29 000000ea`0dfbc850 00007ffe`54b5cc5f     coreclr!IsDebuggerFault+0x98 [E:\repos\runtime3\src\coreclr\src\vm\excep.cpp @ 6564] 
2a 000000ea`0dfbc8a0 00007ffe`54b5ca56     coreclr!CLRVectoredExceptionHandlerPhase2+0x1cf [E:\repos\runtime3\src\coreclr\src\vm\excep.cpp @ 7271] 
2b 000000ea`0dfbcb40 00007ffe`54b5d66b     coreclr!CLRVectoredExceptionHandler+0x2a6 [E:\repos\runtime3\src\coreclr\src\vm\excep.cpp @ 7181] 
2c 000000ea`0dfbcd10 00007ffe`f1207f4c     coreclr!CLRVectoredExceptionHandlerShim+0x2cb [E:\repos\runtime3\src\coreclr\src\vm\excep.cpp @ 7922] 
2d 000000ea`0dfbce10 00007ffe`f11db296     ntdll!RtlpCallVectoredHandlers+0x108 [minkernel\ntdll\vectxcpt.c @ 204] 
2e (Inline Function) --------`--------     ntdll!RtlCallVectoredExceptionHandlers+0xe [minkernel\ntdll\vectxcpt.c @ 345] 
2f 000000ea`0dfbceb0 00007ffe`f122ec1e     ntdll!RtlDispatchException+0x66 [minkernel\ntos\rtl\amd64\exdsptch.c @ 357] 
30 000000ea`0dfbd0c0 00007ffd`f5d7b330     ntdll!KiUserExceptionDispatch+0x2e [minkernel\ntos\rtl\amd64\trampoln.asm @ 749] 
31 000000ea`0dfbd868 00007ffd`f5d7b253     System_Private_CoreLib!ILStubClass.IL_STUB_Array_Set(Int32, Int32, System.ValueTuple`2<Int32,Boolean>)
32 000000ea`0dfbd870 00007ffe`553fbaf3     rt38777!rt38777.Program.Main(System.String[])+0xa3 [E:\mocks\rt38777\Program.cs @ 12] 

Looks like this line checking if a MD corresponds to an NDirect call is triggering an AV because it was called with a bogus MethodDesc* :

runtime/src/coreclr/src/vm/method.hpp

Line 1872 in 312c758

return (m_wFlags & mdcClassification);

This happened while dispatching the notification for the breakpoint for the step into, and we reach DispatchPatchOrSingleStep and start looking for possible triggers for this, finding one which gets classified as a break at a stub at offset 0. When we started looking for possible triggers, we had a context with R10 was already set to a value that the StubManager would use as the MethodDesc* that caused the AV. We get the context from GetThreadFilterContext, and I see the correct IP for the stub, and that Entry2MethodDesc gives back the correct MD. The extended flags for it are 0x00010006 which is nomdILStub | mcArray | mcNDirect, so it's going to need to take the branch that it's failing on. I just don't know why R10 is not valid (not sure if it's related to being at the pure start of the stub and us getting TRACE_MGR_PUSH meaning that the patch's trace is left to the manager instead of the frame that gets us here).

Maybe @jkoritzinsky or @AaronRobinsonMSFT may have some tips for us on why R10 is not what's expected here? I am not very familiar with ABI's here so any pointers are appreciated

[jkoritzinsky]

It looks like ILStubManager::TraceManager doesn't special-case array IL stubs. Considering we had to fix #36249 a few months ago, it's possible that the debugger changed a code path that causes more stub types to go down this route. I think ILStubManager::TraceManager needs to either be updated to handle array IL stubs, or it should be updated to not assume that NDirect is the fallback and instead explicitly check for NDirect methods and otherwise assume that an unknown IL stub type needs to return false.

Also, it looks like the array IL stubs shouldn't be marked as mcNDirect since they don't call into a "target" native method, but instead should be just marked as nomdILStub | mcArray.

[hoyosjs]

I realized I misread the flags. Flags are mcDynamic | mdcRequiresFullSlotNumber | mdcHasNonVtableSlot and extended flags are mdPublic | DynamicMethodDesc::nomdILStub, all of which make sense, except I would have expected to see mcArray in there as well.

@noahfalk what's the expected behavior for these stubs? If I add this we step over and there's no AV:

index 3477154b664..92e3908dc8e 100644
--- a/src/coreclr/src/vm/stubmgr.cpp
+++ b/src/coreclr/src/vm/stubmgr.cpp
@@ -1793,6 +1793,14 @@ BOOL ILStubManager::TraceManager(Thread *thread,
         // so we have nowhere to tell the debugger to move the breakpoint.
         return FALSE;
     }
+#ifdef FEATURE_ARRAYSTUB_AS_IL
+    else if (pStubMD->GetILStubResolver()->GetStubType() == ILStubResolver::ArrayOpStub)
+    {
+        // These stubs are generated by the JIT. There's no code backing
+        // them, so there's no target to relocate the breakpoint to.
+        return FALSE;
+    }
+#endif
     else
     {
         // This is either direct forward P/Invoke or a CLR-to-COM call, the argument is MD

Is there any case where we'd want to let the user step through the native stub (say disassembly mode if they do a step in)?

[jkotas]

Debugger should be treating these stubs as black box ie return FALSE from this method.

It would be better to check for the forward P/Invoke or a CLR-to-COM call stub explicitly before going into "This is either direct forward P/Invoke or a CLR-to-COM call, the argument is MD" path. Once you do that, you can also delete the existing IsStructMarshalStub check and the code would be robust against unexpected types of stubs.

[noahfalk]

Agreed with @jkotas. Not certain if it was clear but I think Jan's suggestion assumes that we add an else { return FALSE } at the end of the chain to avoid the return TRUE that is lurking after it. The logic would look like this:

if(pStubMD->IsMulticastStub()) { ... }
else if(pStubMD->IsReverseStub()) { ... }
else if(pStubMD->IsDelegateStub()) { ... }
else if(pStubMD->IsCALLIStub()) { ... }
else if(pStubMD->IsPInvokeStub()) { pMD = arg; _ASSERTE(pMD->IsNDirect()); ... }
else if(pStubMD->IsCLRToCOMStub()) { pMD = arg; _ASSERTE(pMD->IsComPlusCall()); ... }
else
{
    return FALSE; // Any unrecognized stub we should assume it doesn't have managed code
                  // Even if we are miscategorizing it the worst case is we step over code we should
                  // have stepped into. Much better than a crash.
}
return TRUE;