Fix bbpart handling of EH pads (PR rtl-optimization/85393)

Message ID 20180413172019.GF8577@tucnak
State New
Headers show
Series
  • Fix bbpart handling of EH pads (PR rtl-optimization/85393)
Related show

Commit Message

Jakub Jelinek April 13, 2018, 5:20 p.m.
Hi!

As mentioned in the comments, the .gcc_except_table format doesn't allow the
throwing region and corresponding landing pad to be in different partitions,
because it uses landing_pad_symbol - the start of the partition in which the
throwing region is and we don't generally have relocations for arbitrary
symbol subtraction.  The fix_up_crossing_landing_pad caller checks if all
the EH incoming edges are from the same partition, in that case makes sure
the landing pad is also in that partition; in case they are from different
partitions, "duplicates" the landing pad into another bb, redirects EH edges
from one of the partitions to the new landing pad and makes sure we jump
from that landing pad to the single_succ of the old landing pad.

This works nicely if the landing pad bb hasn't been really modified much,
in particular it still needs to contain only the instructions emitted
by expand_dw2_landing_pad_for_region function call and then an unconditional
jump or at least single successor and nothing else in the bb.

On the following testcase, GCSE pre pass adds another instruction into the
landing pad bb and because fix_up_crossing_landing_pad doesn't really
duplicate the bb, but instead regenerates it from scratch based from what
the landing pad should contain, that pre inserted insn is lost from the new
landing pad and we segfault trying to dereference uninitialized register.
Similarly, if e.g. the landing pad bb is merged with some other bb, or has
more than one successor etc.

Instead of trying to duplicate the whole landing pad (which also wastes
space), this patch instead just emits into the new landing pad's bb a label
and immediately jumps to the other landing pad (that is a crossing jump of
course).  This way we don't need to duplicate anything.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-04-13  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/85393
	* bb-reorder.c (fix_up_crossing_landing_pad): In new_bb emit just
	a label and unconditional jump to old_bb, rather than
	expand_dw2_landing_pad_for_region insn(s) and jump to single_succ
	basic block.

	* g++.dg/opt/pr85393.C: New test.
	* g++.dg/opt/pr85393-aux.cc: New file.


	Jakub

Comments

Eric Botcazou April 13, 2018, 7:32 p.m. | #1
> This works nicely if the landing pad bb hasn't been really modified much,
> in particular it still needs to contain only the instructions emitted
> by expand_dw2_landing_pad_for_region function call and then an unconditional
> jump or at least single successor and nothing else in the bb.

How is that supposed to work for SJLJ though?

> 2018-04-13  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR rtl-optimization/85393
> 	* bb-reorder.c (fix_up_crossing_landing_pad): In new_bb emit just
> 	a label and unconditional jump to old_bb, rather than
> 	expand_dw2_landing_pad_for_region insn(s) and jump to single_succ
> 	basic block.

OK, but also make expand_dw2_landing_pad_for_region private so that the same 
mistake is not made again in the future.
Jakub Jelinek April 13, 2018, 7:58 p.m. | #2
On Fri, Apr 13, 2018 at 09:32:07PM +0200, Eric Botcazou wrote:
> > This works nicely if the landing pad bb hasn't been really modified much,
> > in particular it still needs to contain only the instructions emitted
> > by expand_dw2_landing_pad_for_region function call and then an unconditional
> > jump or at least single successor and nothing else in the bb.
> 
> How is that supposed to work for SJLJ though?

Dunno, does SJLJ emit .gcc_except_table section with this limitation?

> OK, but also make expand_dw2_landing_pad_for_region private so that the same 
> mistake is not made again in the future.

Done, thanks.

	Jakub

Patch

--- gcc/bb-reorder.c.jj	2018-01-12 19:20:32.412976124 +0100
+++ gcc/bb-reorder.c	2018-04-13 15:59:41.094859051 +0200
@@ -1409,14 +1409,14 @@  get_uncond_jump_length (void)
 }
 
 /* The landing pad OLD_LP, in block OLD_BB, has edges from both partitions.
-   Duplicate the landing pad and split the edges so that no EH edge
-   crosses partitions.  */
+   Add a new landing pad that will just jump to the old one and split the
+   edges so that no EH edge crosses partitions.  */
 
 static void
 fix_up_crossing_landing_pad (eh_landing_pad old_lp, basic_block old_bb)
 {
   eh_landing_pad new_lp;
-  basic_block new_bb, last_bb, post_bb;
+  basic_block new_bb, last_bb;
   rtx_insn *jump;
   unsigned new_partition;
   edge_iterator ei;
@@ -1431,24 +1431,20 @@  fix_up_crossing_landing_pad (eh_landing_
   /* Put appropriate instructions in new bb.  */
   rtx_code_label *new_label = emit_label (new_lp->landing_pad);
 
-  expand_dw2_landing_pad_for_region (old_lp->region);
-
-  post_bb = BLOCK_FOR_INSN (old_lp->landing_pad);
-  post_bb = single_succ (post_bb);
-  rtx_code_label *post_label = block_label (post_bb);
-  jump = emit_jump_insn (targetm.gen_jump (post_label));
-  JUMP_LABEL (jump) = post_label;
+  rtx_code_label *old_label = block_label (old_bb);
+  jump = emit_jump_insn (targetm.gen_jump (old_label));
+  JUMP_LABEL (jump) = old_label;
 
   /* Create new basic block to be dest for lp.  */
   last_bb = EXIT_BLOCK_PTR_FOR_FN (cfun)->prev_bb;
   new_bb = create_basic_block (new_label, jump, last_bb);
   new_bb->aux = last_bb->aux;
-  new_bb->count = post_bb->count;
+  new_bb->count = old_bb->count;
   last_bb->aux = new_bb;
 
   emit_barrier_after_bb (new_bb);
 
-  make_single_succ_edge (new_bb, post_bb, 0);
+  make_single_succ_edge (new_bb, old_bb, 0);
 
   /* Make sure new bb is in the other partition.  */
   new_partition = BB_PARTITION (old_bb);
@@ -1457,7 +1453,7 @@  fix_up_crossing_landing_pad (eh_landing_
 
   /* Fix up the edges.  */
   for (ei = ei_start (old_bb->preds); (e = ei_safe_edge (ei)) != NULL; )
-    if (BB_PARTITION (e->src) == new_partition)
+    if (e->src != new_bb && BB_PARTITION (e->src) == new_partition)
       {
 	rtx_insn *insn = BB_END (e->src);
 	rtx note = find_reg_note (insn, REG_EH_REGION, NULL_RTX);
--- gcc/testsuite/g++.dg/opt/pr85393.C.jj	2018-04-13 12:05:46.935137936 +0200
+++ gcc/testsuite/g++.dg/opt/pr85393.C	2018-04-13 12:10:03.473256722 +0200
@@ -0,0 +1,29 @@ 
+// PR rtl-optimization/85393
+// { dg-do run { target c++11 } }
+// { dg-options "-O2" }
+// { dg-additional-sources "pr85393-aux.cc" }
+
+#include <stdexcept>
+#include <vector>
+
+void foo (char const *s);
+struct S { ~S () noexcept (false) { throw std::runtime_error ("foo"); } };
+
+int
+main (int argc, char *argv[])
+{
+  std::vector <std::vector <char> > args;
+  try
+    {
+      {
+        S k;
+        foo ("A");
+      }
+
+      if (argv)
+        throw std::runtime_error ("foo");
+      args.push_back ({});
+    }
+  catch (std::runtime_error const& e)
+    {}
+}
--- gcc/testsuite/g++.dg/opt/pr85393-aux.cc.jj	2018-04-13 12:05:55.475141893 +0200
+++ gcc/testsuite/g++.dg/opt/pr85393-aux.cc	2018-04-13 12:07:49.128194517 +0200
@@ -0,0 +1,5 @@ 
+// PR rtl-optimization/85393
+// { dg-do compile }
+// { dg-options "" }
+
+void foo (char const *) {}