Thread abort exceptions, do not preallocate + some cleanup.

[VSadov]

• no need to preallocate thread abort and base exceptions at initialization
• cleaned up some tracking of state whether thread abort was requested from the user code (no longer possible) vs. internally - from the debugger cancelling expression evaluation.

Main assumptions:

• it is no longer possible for a thread abort to happen directly or indirectly because of the user code.
• the only scenario is the internal use of thread abort by debugger to tear down runaway expression evaluations.
• thread abort in debug scenario is a "best effort" feature. In theory it is unsafe, especially the rude variant. In practice it typically works ok, and is better than terminating the debuggee.

Resolves:#44442

[VSadov]

CC: @tommcdon @hoyosjs

[VSadov]

I have run diagnostics tests with these changes, a few times to be sure - no new failures were observed.

[janvorli]

I like the cleanup of the ThreadAbort handling. But is it really worth the saved size to not to preallocate the exceptions? It seems that an Exception instance takes ~100 bytes only.

[VSadov]

@janvorli - allocations were the original complaint that lead to this PR.

Allocating these instances and handles may not be a lot, but still measurable in the context of startup. @stephentoub may have more on how much they stick out in the profiles.

As I see this - these exceptions are now used only in an extreme corner case when an expression evaluation times out while debugging, and even then pre-allocating them is not really important, so why have that.

[janvorli]

As I see this - these exceptions are now used only in an extreme corner case when an expression evaluation times out while debugging

I keep hitting timeouts when viewing the locals window in VS very often. So I wouldn't call that extreme corner cases :-). But I am not worried about an impact of the thread abort exception allocation.

But I was a bit worried about the preallocated base exception removal. The comments at the following two places explicitly mentions that it was not desirable to get OOM exception there in case of OOM:

runtime/src/coreclr/vm/clrex.cpp

Lines 135 to 150 in 51f59f5

	// I am not convinced if this case is actually a fatal error in the runtime.
	// There have been two bugs in early 2006 (VSW 575647 and 575650) that came in here,
	// both because of OOM and resulted in the ThreadAbort clause above being added since
	// we were creating a ThreadAbort throwable that, due to OOM, got us on a path
	// which came here. Both were valid execution paths and scenarios and not a fatal condition.
	//
	// I am tempted to return preallocated OOM from here but my concern is that it *may*
	// result in fake OOM exceptions being thrown that could break valid scenarios.
	//
	// Hence, we return preallocated System.Exception instance. Lossy information is better
	// than wrong or no information (or even FailFast).
	_ASSERTE (!"Recursion in CLRException::GetThrowable");
	// We didn't recognize it, so use the preallocated System.Exception instance.
	STRESS_LOG0(LF_EH, LL_INFO100, "CLRException::GetThrowable: Recursion! Translating to preallocated System.Exception.\n");
	throwable = GetPreallocatedBaseException();

runtime/src/coreclr/vm/clrex.cpp

Lines 820 to 833 in 51f59f5

	// We *could* come here if one of the calls in the EX_TRY above throws an exception (e.g. MissingMethodException if we attempt
	// to invoke CreateThrowable for a type that does not have a default constructor) that is neither preallocated OOM nor a
	// CLRLastThrownObject type.
	//
	// Like the comment says above, we cannot afford to get the throwable lest we hit SO. In such a case, runtime is not in a bad shape
	// but we dont know what to return as well. A reasonable answer is to return something less appropriate than ripping down process
	// or returning an incorrect exception (e.g. OOM) that could break execution paths.
	//
	// Hence, we return preallocated System.Exception instance.
	if (oRetVal == NULL)
	{
	oRetVal = GetPreallocatedBaseException();
	STRESS_LOG0(LF_EH, LL_INFO100, "CLRException::GetThrowableFromException: Unknown Exception creating throwable; getting preallocated System.Exception.\n");
	}

The new GetBestBaseException would give us OOM exception in case we cannot allocate the Exception, contradicting those two comments.
On the other hand, the likelyhood that the runtime can survive the state when just allocating a new exception results in OOM seems to be very low, so it seems the new way would not make things worse in a general way.

[janvorli]

LGTM, thank you!

[VSadov]

Regarding:

// Hence, we return preallocated System.Exception instance. Lossy information is better
// than wrong or no information (or even FailFast).

My reasoning is that since we cannot provide the "right" exception, because we cannot allocate it, giving OOM would be fairly truthful - we are in a state where we cannot function normally because of memory shortage.

User can try handle the situation by trimming memory use or at least will see OOM in Watson/dump and have a bit more info about environment at the time of the crash.

[VSadov]

Thanks!!!

[stephentoub]

Thanks!