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

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


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

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)
	{
		mono_set_pending_exception(exc);
		// 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.
  */
 void
 mono_set_pending_exception (MonoException *exc)
@@ -4309,14 +4296,8 @@
 	if (thread == NULL)
 		return;
 
+	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.

Note You need to log in before you can comment on or make changes to this bug.