Re: PATCH for c++/15764 (adds new pre-lowering EH optimization pass)

["Richard Guenther" <richard dot guenther at gmail dot com>]

On 10/3/07, Jason Merrill <jason@redhat.com> wrote:
> The bug in 15764 was as follows: if we use a temporary in the
> initializer for a local variable, and the temporary destructor throws,
> we need to destroy the local variable, but we didn't because we haven't
> gotten to the CLEANUP_STMT yet.
>
> I fixed this by walking the initializer and wrapping temporary
> destructors with TRY_CATCH_EXPR to clean up the local variable, which
> fixed the bug.
>
> However, this resulted in us creating more exception regions than
> necessary.  Before the patch, we had something like
>
> temp()
> try { var() } finally { ~temp() }
> try { ... } finally { ~var() }
>
> after this fix, we get
>
> temp()
> try { var() } finally { try { ~temp() } catch { ~var() } }
> try { ... } finally { ~var() }
>
> which introduces another exception region which happens to have the
> exact same handler as the one that follows.
>
> I considered trying to fix up this situation in the front end, but that
> seems very complicated, as the cleanups are still tucked away in
> TARGET_EXPRs where they may or may not even get used, and CLEANUP_STMT
> sitting in the instruction stream.
>
> Conveniently, after control flow lowering but before EH lowering, we
> have a simple sequence of statements where the only control flow
> structures left are TRY_CATCH_EXPR and TRY_FINALLY_EXPR, so it's simple
> to shift things around to get
>
> temp()
> try { var() } catch { ~temp() }
> try { ~temp() ... } finally { ~var() }
>
> which removes the extra exception region.
>
> It should also be feasible to do this optimization after EH lowering, by
> recognizing that the two ~var handlers are the same and have the same
> successor block, and so either combining the two regions into one
> (giving the same result as above) or using the same handler for both.
> But this implementation seemed simpler.  If someone wants to argue that
> I should have done this the other way, I'm interested in hearing that.
>
> Tested x86_64-pc-linux-gnu, applied to trunk.

This seems to break the gmp testsuite which now complains about non-freed
memory.  In fact, for a reduced t-assign.cc

void
check_mpq (void)
{
  // operator=(const mpq_class &)
  {
    mpq_class a(1, 2), b;
    b = a; ASSERT_ALWAYS(b == 0.5);
  }
}

int
main (void)
{
  tests_start();

  check_mpq();

  tests_end();
  return 0;
}

we see the following difference between .011t.lower and .012t.ehopt:

--- tests/cxx/t-assign.cc.011t.lower    2007-10-10 16:48:29.000000000 +0200
+++ tests/cxx/t-assign.cc.012t.ehopt    2007-10-10 16:48:29.000000000 +0200
@@ -81,20 +81,14 @@ void check_mpq() ()
             }
         }
     }
-  finally
+  catch
     {
-      try
-        {
-          D.43374 = (struct __gmp_exprD.26074 *) &D.43336;
-          __comp_dtor  (D.43374);
-        }
-      catch
-        {
-          __comp_dtor  (&aD.43334);
-        }
+      D.43374 = (struct __gmp_exprD.26074 *) &D.43336;
+      __comp_dtor  (D.43374);
     }
   try
     {
+      D.43374 = (struct __gmp_exprD.26074 *) &D.43336;
       __comp_ctor  (&bD.43337);
       try
         {

where you can see that what previously was 'finally' now became
'catch' (and thus,
doesn't call the dtor as required and checked).

Richard.