Bug 3399 - [PATCH] pending_exception support is incomplete
Summary: [PATCH] pending_exception support is incomplete
Alias: None
Product: Runtime
Classification: Mono
Component: General ()
Version: unspecified
Hardware: All All
: --- enhancement
Target Milestone: ---
Assignee: Bugzilla
Depends on:
Reported: 2012-02-10 17:43 UTC by Michael Mudge
Modified: 2012-02-14 11:57 UTC (History)
5 users (show)

Is this bug a regression?: ---
Last known good build:

Patch to implement pending exceptions. (14.69 KB, application/octet-stream)
2012-02-10 17:43 UTC, Michael Mudge

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.

Please create a new report on GitHub or Developer Community with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:

Description Michael Mudge 2012-02-10 17:43:23 UTC
Created attachment 1347 [details]
Patch to implement pending exceptions.

The existing framework to handle Thread.Abort, Thread.Interrupt, Thread.Suspend and Thread.Stop [wisely] will not interrupt native code and instead sets a flag indicating that the exception should be thrown at the native->managed transition.

Architecturally unrelated, the amd64 JIT has an unused mechanism that allows native code to set a flag that causes the given exception to be thrown at the next native->managed transition, which considerably safer than throwing immediately.

Since these two pieces are actually trying to achieve similar things, and the latter is incomplete, I've removed the amd64-specific code and moved the pending exception to occur at the native->managed transition in the same place as former the thread-related exceptions.

There should be no speed impact on code unrelated to pending exceptions, since a single flag [that was already present] is still used to determine if any exception should be thrown.

As far as usage, mono_set_pending_exception() should be used in place of mono_throw_exception() when you don't want the native stack to immediately unwind into managed code.

Sorry for the lousy patch format, this patch was clipped from the Mono 2.10.2 baseline we're working on for our embedded device.  This change was related to our ability to handle signal-like actions [something that happens to a thread where the current instruction doesn't directly expect it].  This includes NullReferenceException, ThreadAbortException, ThreadInterruptedException, OutOfMemoryException [we pend this when we're very low on memory], StackOverflowException [we pend this when we're very low on stack].

In order to support new architectures like ours, it may also be worth it to write a signal-safe function intended to handle these, so the signal handlers only have to handle the peculiarity of the type of signal, such as the format of the sigcontext.  Here is our implementation:

// Note: some processors under some circumstances (Linux Signals on ARM?)
//  will give you IP+4.  This function expects that the ctx contains
//  the IP on the exact instruction where the interrupt occurred.
void handle_thread_interrupt_request(MonoContext* ctx, MonoException* exc)
	static void (*restore_context) (MonoContext *);
	gboolean running_managed;

	if(exc != NULL)
		// If exc is null, we'll just see if an exception is already
		//  pending.

	// See if we interrupted managed or native code.
	running_managed = mono_jit_info_table_find (mono_domain_get (), ctx->eip) != NULL;
	// If managed, this will return an exception.  If native, this will
	//  just return NULL, but the exception [if any] will fire at the
	//  next native->managed transition.
	exc = mono_thread_request_interruption (running_managed); 
	if (exc)
		if (!restore_context)
			restore_context = mono_get_restore_context();

		mono_handle_exception(ctx, exc, ctx->eip, FALSE);
		restore_context (ctx);
Comment 1 Zoltan Varga 2012-02-13 16:45:56 UTC
The amd64 code is actually an optimization to avoid checking for the pending thread-abort etc. exception at every managed-to-native transition, so its not 'incomplete'.
Comment 2 Michael Mudge 2012-02-13 17:07:42 UTC
I assumed it was incomplete because set_pending_exception is MONO_INTERNAL and never called, and it only works on AMD64.

As far as I can tell, mono_thread_interruption_checkpoint_request is called at the native-to-managed transition, and it checks the one flag related to throwing ThreadAbort (thread->interruption_requested).  With this patch, pending exceptions fall under that flag now too.

If the AMD64 code never calls mono_thread_interruption_checkpoint_request, perhaps I took out too much code from the AMD64 parts...

Considering this patch, it would still be unnecessary for AMD64 to check pending_exception itself as long as it calls mono_thread_execute_interruption when thread->interruption_requested is true.
Comment 3 Zoltan Varga 2012-02-13 18:03:22 UTC
What the amd64 code does is that instead of checking the flag during managed-to-native transitions, it hijacks the return address on the stack mono_thread_get_and_clear_pending_exception () to obtain the exception which needs to be thrown, and throws it.

So the amd64 code can be kept, and the only thing needed is probably this part:

-	} else if (thread->thread_interrupt_requested) {
+	}
+	else if (thread->pending_exception)
+	{
+		MonoException * exc = mono_thread_get_and_clear_pending_exception();
+		LeaveCriticalSection (thread->synch_cs);
+		return exc;
+	}
Comment 4 Michael Mudge 2012-02-14 11:18:47 UTC
Ok sounds fair.

The patch would be like this then:

File: mono\mono\metadata\threads.c
--- mono\mono\metadata\threads.c;C23173  (server)    2/7/2012 3:35 PM
+++ mono\mono\metadata\threads.c  (local)    2/7/2012 3:35 PM
@@ -4124,7 +4117,14 @@
 		mono_thread_exit ();
 		return NULL;
-	} else if (thread->thread_interrupt_requested) {
+	}
+	else if (thread->pending_exception)
+	{
+		MonoException * exc = mono_thread_get_and_clear_pending_exception();
+		LeaveCriticalSection (thread->synch_cs);
+		return exc;
+	}
+	else if (thread->thread_interrupt_requested) {
 		thread->thread_interrupt_requested = FALSE;
 		LeaveCriticalSection (thread->synch_cs);
@@ -4293,12 +4284,8 @@
  * mono_set_pending_exception:
- *   Set the pending exception of the current thread to EXC. On platforms which 
- * support it, the exception will be thrown when execution returns to managed code. 
- * On other platforms, this function is equivalent to mono_raise_exception (). 
- * Internal calls which report exceptions using this function instead of 
- * raise_exception () might be called by JITted code using a more efficient calling 
- * convention.
+ *   Set the pending exception of the current thread to EXC.
+ * The exception will be thrown when execution returns to managed code.
 mono_set_pending_exception (MonoException *exc)
@@ -4309,14 +4296,8 @@
 	if (thread == NULL)
+	MONO_OBJECT_SETREF (thread, pending_exception, exc);
 	if (mono_thread_notify_pending_exc_fn) {
-		MONO_OBJECT_SETREF (thread, pending_exception, exc);
 		mono_thread_notify_pending_exc_fn ();
-	} else {
-		/* No way to notify the JIT about the exception, have to throw it now */
-		mono_raise_exception (exc);
+	mono_thread_request_interruption(/*running_managed=*/FALSE); // Make sure the thread knows there is an interruption waiting.
Comment 5 Zoltan Varga 2012-02-14 11:57:22 UTC
Applied a variant of this in 366d59462bc4925f010119922d5e5948901cb13e.