On Fri, Mar 26, 2010 at 12:49 PM, Paolo Molaro <span dir="ltr">&lt;<a href="mailto:lupus@ximian.com">lupus@ximian.com</a>&gt;</span> wrote:<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
On 03/24/10 Rodrigo Kumpera wrote:<br>
&gt; --- a/mono/mini/mini-exceptions.c<br>
&gt; +++ b/mono/mini/mini-exceptions.c<br>
[...]<br>
&gt; +static gboolean<br>
&gt; +find_last_handler_block (MonoDomain *domain, MonoContext *ctx, MonoJitInfo *ji, gpointer data)<br>
&gt; +{<br>
&gt; +     int i;<br>
&gt; +     gpointer ip;<br>
&gt; +     FindHandlerBlockData *pdata = data;<br>
&gt; +<br>
&gt; +     if (ji-&gt;method-&gt;wrapper_type)<br>
&gt; +             return FALSE;<br>
&gt; +<br>
&gt; +     ip = MONO_CONTEXT_GET_IP (ctx);<br>
&gt; +<br>
&gt; +     for (i = 0; i &lt; ji-&gt;num_clauses; ++i) {<br>
&gt; +             MonoJitExceptionInfo *ei = ji-&gt;clauses + i;<br>
&gt; +             if (ei-&gt;flags != MONO_EXCEPTION_CLAUSE_FINALLY)<br>
&gt; +                     continue;<br>
&gt; +             /*If ip points to the first instruction it means the handler block didn&#39;t start<br>
&gt; +              so we can leave its execution to the EH machinery*/<br>
&gt; +             if (ei-&gt;handler_start &lt; ip &amp;&amp; ip &lt; ei-&gt;data.handler_end) {<br>
&gt; +                     pdata-&gt;ji = ji;<br>
&gt; +                     pdata-&gt;ei = ei;<br>
&gt; +                     pdata-&gt;ctx = *ctx;<br>
&gt; +                     break;<br>
<br>
Shouldn&#39;t the stack walk be interrupted here once we found the finally<br>
clause?<br></blockquote><div><br></div><div><div>No, we must guard around the botton finally clause. Since we can only do</div><div>stackwalks in one direction this requires us to walk the whole stack. This could</div></div>
<div>be fixed by walking from bottom to top, but it&#39;s a significantly complex change for</div><div>something that is not performance critical AFAICT.</div><div><br></div><div>The protection works across all usual suspects such as runtime invokes and x-domain</div>
<div>calls, so the only criteria I could come up with is that is that has to be the bottom one.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

<br>
&gt; --- a/mono/mini/mini-posix.c<br>
&gt; +++ b/mono/mini/mini-posix.c<br>
&gt; @@ -189,7 +190,15 @@ SIG_HANDLER_SIGNATURE (sigusr1_signal_handler)<br>
&gt;<br>
&gt;       if (mono_debugger_agent_thread_interrupt (ctx, ji))<br>
&gt;               return;<br>
&gt; -<br>
&gt; +<br>
&gt; +     /* We can&#39;t do handler block checking from metadata since it requires doing<br>
&gt; +      * a stack walk with context.<br>
&gt; +      */<br>
&gt; +     mono_arch_sigctx_to_monoctx (ctx, &amp;mctx);<br>
&gt; +     if (mono_install_handler_block_guard (thread, &amp;mctx)) {<br>
&gt; +             return;<br>
&gt; +     }<br>
&gt; +#<br>
<br>
Leftover here.<br></blockquote><div><br></div><div>Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
&gt; --- a/mono/mini/mini-x86.c<br>
&gt; +++ b/mono/mini/mini-x86.c<br>
&gt; @@ -5809,6 +5809,70 @@ mono_arch_decompose_long_opts (MonoCompile *cfg, MonoInst *long_ins)<br>
&gt;  #endif /* MONO_ARCH_SIMD_INTRINSICS */<br>
&gt;  }<br>
&gt;<br>
&gt; +/*MONO_ARCH_HAVE_HANDLER_BLOCK_GUARD*/<br>
&gt; +gpointer<br>
&gt; +mono_arch_install_handler_block_guard (MonoJitInfo *ji, MonoContext *ctx, gpointer new_value)<br>
&gt; +{<br>
&gt; +     int i;<br>
&gt; +     int offset;<br>
&gt; +     MonoJitExceptionInfo *clause = NULL;<br>
&gt; +     gpointer ip, *sp, old_value;<br>
&gt; +     char *bp;<br>
&gt; +     const unsigned char *handler;<br>
&gt; +<br>
&gt; +     ip = MONO_CONTEXT_GET_IP (ctx);<br>
&gt; +<br>
&gt; +     for (i = 0; i &lt; ji-&gt;num_clauses; ++i) {<br>
&gt; +             clause = &amp;ji-&gt;clauses [i];<br>
&gt; +             if (clause-&gt;flags != MONO_EXCEPTION_CLAUSE_FINALLY)<br>
&gt; +                     continue;<br>
&gt; +             if (clause-&gt;handler_start &lt; ip &amp;&amp; clause-&gt;data.handler_end &gt; ip)<br>
&gt; +                     break;<br>
&gt; +     }<br>
&gt; +<br>
&gt; +     /*no matching finally */<br>
&gt; +     if (i == ji-&gt;num_clauses)<br>
&gt; +             return NULL;<br>
&gt; +<br>
&gt; +     /*If we stopped on the instruction right before the try, we haven&#39;t actually started executing it*/<br>
&gt; +     if (ip == clause-&gt;handler_start)<br>
&gt; +             return NULL;<br>
&gt; +<br>
<br>
Up to here there is nothing that is arch-specific. All this code should<br>
be moved to the caller.</blockquote><div><br></div><div>Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
The rest of the changes look fine to me.<br>
You might need to disable this code with aot, since I&#39;m not sure the<br>
complete clause data is saved in that case for this to work: did you<br>
test it already? Or the aot changes would need to be implemented, anyway<br></blockquote><div><br></div><div>The current code doesn&#39;t work under full-aot as we don&#39;t save the trampolines, I&#39;ll follow with Zoltan on this.</div>
<div>I just tested under AOT and a pair of issues arose. Both fixed now.</div><div><br></div><div><br></div><div><br></div><div><br></div></div>