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

[Rodrigo Kumpera]

On Fri, Mar 26, 2010 at 12:49 PM, Paolo Molaro <lupus at ximian.com> wrote:

> 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?
>

No, we must guard around the botton finally clause. Since we can only do
stackwalks in one direction this requires us to walk the whole stack. This
could
be fixed by walking from bottom to top, but it's a significantly complex
change for
something that is not performance critical AFAICT.

The protection works across all usual suspects such as runtime invokes and
x-domain
calls, so the only criteria I could come up with is that is that has to be
the bottom one.



>
> > --- 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.
>

Fixed.


>
> > --- 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.


Fixed.


>
> 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
>

The current code doesn't work under full-aot as we don't save the
trampolines, I'll follow with Zoltan on this.
I just tested under AOT and a pair of issues arose. Both fixed now.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20100329/3092efbe/attachment-0001.html 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: handler_block_guard_2.patch
Type: text/x-patch
Size: 19138 bytes
Desc: not available
Url : http://lists.ximian.com/pipermail/mono-devel-list/attachments/20100329/3092efbe/attachment-0001.bin