[Mono-dev] [PATCH] Implement guarded finally blocks

[Paolo Molaro]

On 03/24/10 Rodrigo Kumpera wrote:
> --- a/mono/mini/mini-exceptions.c
> +++ b/mono/mini/mini-exceptions.c
[...]
> +static gboolean
> +find_last_handler_block (MonoDomain *domain, MonoContext *ctx, MonoJitInfo *ji, gpointer data)
> +{
> +	int i;
> +	gpointer ip;
> +	FindHandlerBlockData *pdata = data;
> +
> +	if (ji->method->wrapper_type)
> +		return FALSE;
> +
> +	ip = MONO_CONTEXT_GET_IP (ctx);
> +
> +	for (i = 0; i < ji->num_clauses; ++i) {
> +		MonoJitExceptionInfo *ei = ji->clauses + i;
> +		if (ei->flags != MONO_EXCEPTION_CLAUSE_FINALLY)
> +			continue;
> +		/*If ip points to the first instruction it means the handler block didn't start
> +		 so we can leave its execution to the EH machinery*/
> +		if (ei->handler_start < ip && ip < ei->data.handler_end) {
> +			pdata->ji = ji;
> +			pdata->ei = ei;
> +			pdata->ctx = *ctx;
> +			break;

Shouldn't the stack walk be interrupted here once we found the finally
clause?

> --- a/mono/mini/mini-posix.c
> +++ b/mono/mini/mini-posix.c
> @@ -189,7 +190,15 @@ SIG_HANDLER_SIGNATURE (sigusr1_signal_handler)
>  
>  	if (mono_debugger_agent_thread_interrupt (ctx, ji))
>  		return;
> -	
> +
> +	/* We can't do handler block checking from metadata since it requires doing
> +	 * a stack walk with context.
> +	 */
> +	mono_arch_sigctx_to_monoctx (ctx, &mctx);
> +	if (mono_install_handler_block_guard (thread, &mctx)) {
> +		return;
> +	}
> +#

Leftover here.

> --- a/mono/mini/mini-x86.c
> +++ b/mono/mini/mini-x86.c
> @@ -5809,6 +5809,70 @@ mono_arch_decompose_long_opts (MonoCompile *cfg, MonoInst *long_ins)
>  #endif /* MONO_ARCH_SIMD_INTRINSICS */
>  }
>  
> +/*MONO_ARCH_HAVE_HANDLER_BLOCK_GUARD*/
> +gpointer
> +mono_arch_install_handler_block_guard (MonoJitInfo *ji, MonoContext *ctx, gpointer new_value)
> +{
> +	int i;
> +	int offset;
> +	MonoJitExceptionInfo *clause = NULL;
> +	gpointer ip, *sp, old_value;
> +	char *bp;
> +	const unsigned char *handler;
> +
> +	ip = MONO_CONTEXT_GET_IP (ctx);
> +
> +	for (i = 0; i < ji->num_clauses; ++i) {
> +		clause = &ji->clauses [i];
> +		if (clause->flags != MONO_EXCEPTION_CLAUSE_FINALLY)
> +			continue;
> +		if (clause->handler_start < ip && clause->data.handler_end > ip)
> +			break;
> +	}
> +
> +	/*no matching finally */
> +	if (i == ji->num_clauses)
> +		return NULL;
> +
> +	/*If we stopped on the instruction right before the try, we haven't actually started executing it*/
> +	if (ip == clause->handler_start)
> +		return NULL;
> +

Up to here there is nothing that is arch-specific. All this code should
be moved to the caller.

The rest of the changes look fine to me.
You might need to disable this code with aot, since I'm not sure the
complete clause data is saved in that case for this to work: did you
test it already? Or the aot changes would need to be implemented, anyway

Thanks!

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better