This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.
2007-10-03  Jason Merrill  <jason@redhat.com>

	PR c++/15764
	* cp/decl.c (wrap_cleanups_r): New fn.
	(wrap_temporary_cleanups): New fn.
	(initialize_local_var): Call it.
	* tree-eh.c (same_handler_p): New fn.
	(optimize_double_finally): New fn.
	(refactor_eh_r): New fn.
	(refactor_eh): New fn.
	(pass_refactor_eh): New pass.
	* tree-pass.h: Declare it.
	* passes.c (init_optimization_passes): Add it.

Index: cp/decl.c
===================================================================
*** cp/decl.c	(revision 128890)
--- cp/decl.c	(working copy)
*************** make_rtl_for_nonlocal_decl (tree decl, t
*** 5136,5141 ****
--- 5136,5176 ----
      rest_of_decl_compilation (decl, toplev, at_eof);
  }
  
+ /* walk_tree helper for wrap_temporary_cleanups, below.  */
+ 
+ static tree
+ wrap_cleanups_r (tree *stmt_p, int *walk_subtrees, void *data)
+ {
+   if (TYPE_P (*stmt_p))
+     {
+       *walk_subtrees = 0;
+       return NULL_TREE;
+     }
+ 
+   if (TREE_CODE (*stmt_p) == TARGET_EXPR)
+     {
+       tree guard = (tree)data;
+       tree tcleanup = TARGET_EXPR_CLEANUP (*stmt_p);
+ 
+       tcleanup = build2 (TRY_CATCH_EXPR, void_type_node, tcleanup, guard);
+ 
+       TARGET_EXPR_CLEANUP (*stmt_p) = tcleanup;
+     }
+ 
+   return NULL_TREE;
+ }
+ 
+ /* We're initializing a local variable which has a cleanup GUARD.  If there
+    are any temporaries used in the initializer INIT of this variable, we
+    need to wrap their cleanups with TRY_CATCH_EXPR (, GUARD) so that the
+    variable will be cleaned up properly if one of them throws.  */
+ 
+ static void
+ wrap_temporary_cleanups (tree init, tree guard)
+ {
+   cp_walk_tree_without_duplicates (&init, wrap_cleanups_r, (void *)guard);
+ }
+ 
  /* Generate code to initialize DECL (a local variable).  */
  
  static void
*************** initialize_local_var (tree decl, tree in
*** 5143,5148 ****
--- 5178,5184 ----
  {
    tree type = TREE_TYPE (decl);
    tree cleanup;
+   int already_used;
  
    gcc_assert (TREE_CODE (decl) == VAR_DECL
  	      || TREE_CODE (decl) == RESULT_DECL);
*************** initialize_local_var (tree decl, tree in
*** 5153,5198 ****
        /* If we used it already as memory, it must stay in memory.  */
        DECL_INITIAL (decl) = NULL_TREE;
        TREE_ADDRESSABLE (decl) = TREE_USED (decl);
      }
  
!   if (DECL_SIZE (decl) && type != error_mark_node)
!     {
!       int already_used;
  
!       /* Compute and store the initial value.  */
!       already_used = TREE_USED (decl) || TREE_USED (type);
  
!       /* Perform the initialization.  */
!       if (init)
! 	{
! 	  int saved_stmts_are_full_exprs_p;
  
! 	  gcc_assert (building_stmt_tree ());
! 	  saved_stmts_are_full_exprs_p = stmts_are_full_exprs_p ();
! 	  current_stmt_tree ()->stmts_are_full_exprs_p = 1;
! 	  finish_expr_stmt (init);
! 	  current_stmt_tree ()->stmts_are_full_exprs_p =
! 	    saved_stmts_are_full_exprs_p;
! 	}
! 
!       /* Set this to 0 so we can tell whether an aggregate which was
! 	 initialized was ever used.  Don't do this if it has a
! 	 destructor, so we don't complain about the 'resource
! 	 allocation is initialization' idiom.  Now set
! 	 attribute((unused)) on types so decls of that type will be
! 	 marked used. (see TREE_USED, above.)  */
!       if (TYPE_NEEDS_CONSTRUCTING (type)
! 	  && ! already_used
! 	  && TYPE_HAS_TRIVIAL_DESTRUCTOR (type)
! 	  && DECL_NAME (decl))
! 	TREE_USED (decl) = 0;
!       else if (already_used)
! 	TREE_USED (decl) = 1;
      }
  
!   /* Generate a cleanup, if necessary.  */
!   cleanup = cxx_maybe_build_cleanup (decl);
!   if (DECL_SIZE (decl) && cleanup)
      finish_decl_cleanup (decl, cleanup);
  }
  
--- 5189,5241 ----
        /* If we used it already as memory, it must stay in memory.  */
        DECL_INITIAL (decl) = NULL_TREE;
        TREE_ADDRESSABLE (decl) = TREE_USED (decl);
+       return;
      }
  
!   if (type == error_mark_node)
!     return;
  
!   /* Compute and store the initial value.  */
!   already_used = TREE_USED (decl) || TREE_USED (type);
  
!   /* Generate a cleanup, if necessary.  */
!   cleanup = cxx_maybe_build_cleanup (decl);
! 
!   /* Perform the initialization.  */
!   if (init)
!     {
!       int saved_stmts_are_full_exprs_p;
  
!       /* If we're only initializing a single object, guard the destructors
! 	 of any temporaries used in its initializer with its destructor.
! 	 This isn't right for arrays because each element initialization is
! 	 a full-expression.  */
!       if (cleanup && TREE_CODE (type) != ARRAY_TYPE)
! 	wrap_temporary_cleanups (init, cleanup);
! 
!       gcc_assert (building_stmt_tree ());
!       saved_stmts_are_full_exprs_p = stmts_are_full_exprs_p ();
!       current_stmt_tree ()->stmts_are_full_exprs_p = 1;
!       finish_expr_stmt (init);
!       current_stmt_tree ()->stmts_are_full_exprs_p =
! 	saved_stmts_are_full_exprs_p;
      }
  
!   /* Set this to 0 so we can tell whether an aggregate which was
!      initialized was ever used.  Don't do this if it has a
!      destructor, so we don't complain about the 'resource
!      allocation is initialization' idiom.  Now set
!      attribute((unused)) on types so decls of that type will be
!      marked used. (see TREE_USED, above.)  */
!   if (TYPE_NEEDS_CONSTRUCTING (type)
!       && ! already_used
!       && TYPE_HAS_TRIVIAL_DESTRUCTOR (type)
!       && DECL_NAME (decl))
!     TREE_USED (decl) = 0;
!   else if (already_used)
!     TREE_USED (decl) = 1;
! 
!   if (cleanup)
      finish_decl_cleanup (decl, cleanup);
  }
  
Index: tree-pass.h
===================================================================
*** tree-pass.h	(revision 128841)
--- tree-pass.h	(working copy)
*************** extern struct tree_opt_pass pass_mudflap
*** 246,251 ****
--- 246,252 ----
  extern struct tree_opt_pass pass_mudflap_2;
  extern struct tree_opt_pass pass_remove_useless_stmts;
  extern struct tree_opt_pass pass_lower_cf;
+ extern struct tree_opt_pass pass_refactor_eh;
  extern struct tree_opt_pass pass_lower_eh;
  extern struct tree_opt_pass pass_build_cfg;
  extern struct tree_opt_pass pass_tree_profile;
Index: tree-eh.c
===================================================================
*** tree-eh.c	(revision 128841)
--- tree-eh.c	(working copy)
*************** maybe_clean_or_replace_eh_stmt (tree old
*** 2093,2095 ****
--- 2093,2251 ----
  
    return false;
  }
+ 
+ /* Returns TRUE if oneh and twoh are exception handlers (op 1 of
+    TRY_CATCH_EXPR or TRY_FINALLY_EXPR that are similar enough to be
+    considered the same.  Currently this only handles handlers consisting of
+    a single call, as that's the important case for C++: a destructor call
+    for a particular object showing up in multiple handlers.  */
+ 
+ static bool
+ same_handler_p (tree oneh, tree twoh)
+ {
+   tree_stmt_iterator i;
+   tree ones, twos;
+   int ai;
+ 
+   i = tsi_start (oneh);
+   if (!tsi_one_before_end_p (i))
+     return false;
+   ones = tsi_stmt (i);
+ 
+   i = tsi_start (twoh);
+   if (!tsi_one_before_end_p (i))
+     return false;
+   twos = tsi_stmt (i);
+ 
+   if (TREE_CODE (ones) != CALL_EXPR
+       || TREE_CODE (twos) != CALL_EXPR
+       || !operand_equal_p (CALL_EXPR_FN (ones), CALL_EXPR_FN (twos), 0)
+       || call_expr_nargs (ones) != call_expr_nargs (twos))
+     return false;
+ 
+   for (ai = 0; ai < call_expr_nargs (ones); ++ai)
+     if (!operand_equal_p (CALL_EXPR_ARG (ones, ai),
+ 			  CALL_EXPR_ARG (twos, ai), 0))
+       return false;
+ 
+   return true;
+ }
+ 
+ /* Optimize
+     try { A() } finally { try { ~B() } catch { ~A() } }
+     try { ... } finally { ~A() }
+    into
+     try { A() } catch { ~B() }
+     try { ~B() ... } finally { ~A() }
+ 
+    This occurs frequently in C++, where A is a local variable and B is a
+    temporary used in the initializer for A.  */
+ 
+ static void
+ optimize_double_finally (tree one, tree two)
+ {
+   tree oneh;
+   tree_stmt_iterator i;
+ 
+   i = tsi_start (TREE_OPERAND (one, 1));
+   if (!tsi_one_before_end_p (i))
+     return;
+ 
+   oneh = tsi_stmt (i);
+   if (TREE_CODE (oneh) != TRY_CATCH_EXPR)
+     return;
+ 
+   if (same_handler_p (TREE_OPERAND (oneh, 1), TREE_OPERAND (two, 1)))
+     {
+       tree twoh;
+ 
+       tree b = TREE_OPERAND (oneh, 0);
+       TREE_OPERAND (one, 1) = b;
+       TREE_SET_CODE (one, TRY_CATCH_EXPR);
+ 
+       b = tsi_stmt (tsi_start (b));
+       twoh = TREE_OPERAND (two, 0);
+       /* same_handler_p only handles single-statement handlers,
+ 	 so there must only be one statement.  */
+       i = tsi_start (twoh);
+       tsi_link_before (&i, unshare_expr (b), TSI_SAME_STMT);
+     }
+ }
+ 
+ /* Perform EH refactoring optimizations that are simpler to do when code
+    flow has been lowered but EH structurs haven't.  */
+ 
+ static void
+ refactor_eh_r (tree t)
+ {
+  tailrecurse:
+   switch (TREE_CODE (t))
+     {
+     case TRY_FINALLY_EXPR:
+     case TRY_CATCH_EXPR:
+       refactor_eh_r (TREE_OPERAND (t, 0));
+       t = TREE_OPERAND (t, 1);
+       goto tailrecurse;
+ 
+     case CATCH_EXPR:
+       t = CATCH_BODY (t);
+       goto tailrecurse;
+ 
+     case EH_FILTER_EXPR:
+       t = EH_FILTER_FAILURE (t);
+       goto tailrecurse;
+ 
+     case STATEMENT_LIST:
+       {
+ 	tree_stmt_iterator i;
+ 	tree one = NULL_TREE, two = NULL_TREE;
+ 	/* Try to refactor double try/finally.  */
+ 	for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i))
+ 	  {
+ 	    one = two;
+ 	    two = tsi_stmt (i);
+ 	    if (one && two
+ 		&& TREE_CODE (one) == TRY_FINALLY_EXPR
+ 		&& TREE_CODE (two) == TRY_FINALLY_EXPR)
+ 	      optimize_double_finally (one, two);
+ 	    if (one)
+ 	      refactor_eh_r (one);
+ 	  }
+ 	if (two)
+ 	  {
+ 	    t = two;
+ 	    goto tailrecurse;
+ 	  }
+       }
+       break;
+ 
+     default:
+       /* A type, a decl, or some kind of statement that we're not
+ 	 interested in.  Don't walk them.  */
+       break;
+     }
+ }
+ 
+ static unsigned
+ refactor_eh (void)
+ {
+   refactor_eh_r (DECL_SAVED_TREE (current_function_decl));
+   return 0;
+ }
+ 
+ struct tree_opt_pass pass_refactor_eh =
+ {
+   "ehopt",				/* name */
+   NULL,					/* gate */
+   refactor_eh,				/* execute */
+   NULL,					/* sub */
+   NULL,					/* next */
+   0,					/* static_pass_number */
+   TV_TREE_EH,				/* tv_id */
+   PROP_gimple_lcf,			/* properties_required */
+   0,					/* properties_provided */
+   0,					/* properties_destroyed */
+   0,					/* todo_flags_start */
+   TODO_dump_func,			/* todo_flags_finish */
+   0					/* letter */
+ };
Index: passes.c
===================================================================
*** passes.c	(revision 128841)
--- passes.c	(working copy)
*************** init_optimization_passes (void)
*** 480,485 ****
--- 480,486 ----
    NEXT_PASS (pass_mudflap_1);
    NEXT_PASS (pass_lower_omp);
    NEXT_PASS (pass_lower_cf);
+   NEXT_PASS (pass_refactor_eh);
    NEXT_PASS (pass_lower_eh);
    NEXT_PASS (pass_build_cfg);
    NEXT_PASS (pass_lower_complex_O0);
Index: testsuite/g++.dg/eh/init-temp1.C
===================================================================
*** testsuite/g++.dg/eh/init-temp1.C	(revision 0)
--- testsuite/g++.dg/eh/init-temp1.C	(revision 0)
***************
*** 0 ****
--- 1,44 ----
+ // PR c++/15764
+ 
+ extern "C" void abort (); 
+  
+ int counter = 0; 
+ int thrown; 
+ struct a { 
+   ~a () { if (thrown++ == 0) throw 42; } 
+ }; 
+  
+ int f (a const&) { return 1; } 
+ int f (a const&, a const&) { return 1; } 
+  
+ struct b { 
+   b (...) { ++counter; } 
+   ~b ()   { --counter; } 
+ }; 
+ 
+ bool p;
+ void g()
+ {
+   if (p) throw 42;
+ }
+ 
+ int main () { 
+   thrown = 0;
+   try {
+     b tmp(f (a(), a()));
+ 
+     g();
+   }  
+   catch (...) {} 
+ 
+   thrown = 0;
+   try {
+     b tmp(f (a()));
+ 
+     g();
+   }  
+   catch (...) {} 
+  
+   if (counter != 0) 
+     abort (); 
+ } 

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]