Fix VS issue with unhandled exception on secondary threads

[janvorli]

When "just my code" is disabled, unhandled exceptions on secondary threads cause the VS to stop without any stack trace shown. This is similar to the recently fixed problem that was happening with user unhandled exception treatment.
The exception is rethrown as native exception after leaving the last managed frame and it propagates to the ManagedThreadBase_DispatchOuter where it is caught. The debugger gets notified from there, but all of the managed stack frames are gone at that point, so the debugger cannot show them.
The fix is to report exception on a secondary thread as unhandled earlier, right in the SfiNext where we report it for the primary threads. The secondary thread is different in having the DebuggerU2MCatchHandlerFrame on stack while in the primary thread case, there is no explicit frame.

Close #103385

[davidwrighton]

@janvorli I don't have a comment on the correctness of this fix, as I'm not familiar with the debugger interaction here, but I see that just a few lines before this fix is a TODO-NewEH comment about FastCallFinalize and COMToCLRDispatchHelperWithStack Is that comment obsolete? Do we need to fix it for .NET 9?

[janvorli]

@davidwrighton I still need to investigate whether it is needed to do anything here. Thank you for noticing this / reminding me of this. None of the tests have ever hit a problem due to that and neither have customers reported any issues related to this. But it obviously doesn't mean it is fine as is, as a problem can be lurking waiting for someone to trigger it.

[janvorli]

Thanks @davidwrighton for the comment again. The finalizer case actually has similar problem as the one fixed by this PR, so I am going to add a fix to this PR too.
@jkotas, I've investigated possible fixes. One is to do the same thing as we did for the CallDescrWorkerInternal, but I've noticed that the FastCallFinalizeWorker exists only for x64 (besides x86). arm64 calls the finalizers via CALL_MANAGED_METHOD_NORET. I've tried to use the same for x64 and it also gets rid of the problem. I wonder - does the "fast call" still make sense or do you think that switching x64 to the same plan as other non-x86 architectures would be fine?

[jkotas]

I wonder - does the "fast call" still make sense or do you think that switching x64 to the same plan as other non-x86 architectures would be fine?

I think it should be fine. CALL_MANAGED_METHOD_NORET reverse-FCall path is not that expensive. We did not have that path originally. You can try to build a small ad-hoc microbenchmark to see whether there is going to be significant measurable regression. If there is a significant measurable regression, it should be possible to fix by changing the polarity of the finalization loop. Run the finalization loop in managed code, and invoke the individual finalizers from managed code like we do it in naot:

runtime/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/__Finalizer.cs

Lines 55 to 67 in f8f4509

	while (true)
	{
	object target = InternalCalls.RhpGetNextFinalizableObject();
	if (target == null)
	return finalizerCount;
	finalizerCount++;
	// Call the finalizer on the current target object. If the finalizer throws we'll fail
	// fast via normal Redhawk exception semantics (since we don't attempt to catch
	// anything).
	((delegate*<object, void>)target.GetMethodTable()->FinalizerCode)(target);
	}

I would get rid of the x86 special path as well.

[jkotas]

Can DebuggerU2MCatchHandlerFrame be in the middle of the stack, with a bunch more managed frame behind it?

[janvorli]

I have believed when I've searched some time ago that the only place where it is used is on top of secondary threads. But I have searched for it again to be 100% sure now and I have actually missed on in DispatchInfo::InvokeMember. So it seems that both this check and a similar one in the NotifyExceptionPassStarted should also check if that frame is also the topmost one.

[janvorli]

@jkotas I have added check for the DebuggerU2MCatchHandlerFrame being the topmost one.

[janvorli]

@jkotas I have added some instrumentation into the FinalizerThread::FinalizeAllObjects, summing the time it took to run the loop and number of items and printing it out every now and then. Then ran it with a test that creates tens of millions of finalizable objects with finalizer methods being empty.
The current FastCallFinalizeWorker way seems to be 15% faster than the CALL_MANAGED_METHOD_NORET way.

[jkotas]

I think it is acceptable regression. We can open a tracking issue to make the perf better by porting the naot scheme for finalizers.

[jkotas]

We can open a tracking issue to make the perf better by porting the naot scheme for finalizers.

Actually, it is a nice cleanup. I have just done it: #103501

[janvorli]

We don't propagate managed exceptions through the COMToCLRDispatchHelper, they are caught and transformed into HRESULT, so after the FastCallFinalize was removed by another PR, this comment is now obsolete.