diff mbox

[rtl] : Teach _.barriers and _.eh_range passes to not split a call and its corresponding CALL_ARG_LOCATION note.

Message ID CAFULd4Z23Lo+H16d+3C3RP+qHQrwCM=RH3QFbRigpxznM+rE_Q@mail.gmail.com
State New
Headers show

Commit Message

Uros Bizjak June 26, 2014, 9:15 p.m. UTC 
Hello!

Attached patch is needed to fix PR 56858 [1], where alpha used
NOTE_INSN_EH_REGION_BEG and NOTE_INSN_EH_REGION_END notes to handle
its FP traps. After the last RTL exceptions rewrite, those notes are
generated after mach_reorg pass, effectively rendering existing
approach unusable.

In PR 56858, a new target-dependent pass was introduced that moved the
traps generation just after eh_ranges pass. trapb insns were emitted
at the point of NOTE_INSN_EH_REGION_END notes, and this insertion
broke compilation due to ICE in dwarf2out_var_location, at
dwarf2out.c.

The problem was that trapb insn split a call and its corresponding
CALL_ARG_LOCATION note.

The patch fixes the splitting problem by teaching _.barriers and
_.eh_range passes to not split a call and its corresponding
CALL_ARG_LOCATION note. Apparently, a scary comment in jump.c:

/* Some old code expects exactly one BARRIER as the NEXT_INSN of a
   non-fallthru insn.  This is not generally true, as multiple barriers
   may have crept in, or the BARRIER may be separated from the last
   real insn by one or more NOTEs.

   This simple pass moves barriers and removes duplicates so that the
   old code is happy.
 */

does not apply anymore, and the "old code" tolerates additional note
after the call just fine.

The problematic test (g++.dg/torture/stackalign/eh-vararg-1.C) now
compiles on x86_64-pc-linux-gnu to following _.final dump:

...
(insn:TI 115 114 116 11 (set (reg:DI 5 di)
        (reg/f:DI 0 ax [126])) eh-vararg-1.C:62 89 {*movdi_internal}
     (expr_list:REG_DEAD (reg/f:DI 0 ax [126])
        (nil)))
(call_insn:TI 116 115 203 11 (call (mem:QI (symbol_ref:DI
("__cxa_throw") [flags 0x41]  <function_decl 0x7fcf18d9d900
__cxa_throw>) [0 __cxa_throw S1 A8])
        (const_int 0 [0])) eh-vararg-1.C:62 642 {*call}
     (expr_list:REG_DEAD (reg:DI 5 di)
        (expr_list:REG_DEAD (reg:DI 4 si)
            (expr_list:REG_DEAD (reg:DI 1 dx)
                (expr_list:REG_EH_REGION (const_int 1 [0x1])
                    (expr_list:REG_CALL_DECL (symbol_ref:DI
("__cxa_throw") [flags 0x41]  <function_decl 0x7fcf18d9d900
__cxa_throw>)
                        (expr_list:REG_ARGS_SIZE (const_int 0 [0])
                            (expr_list:REG_NORETURN (const_int 0 [0])
                                (nil))))))))
    (expr_list:DI (use (reg:DI 5 di))
        (expr_list:DI (use (reg:DI 4 si))
            (expr_list:DI (use (reg:DI 1 dx))
                (nil)))))
(note 203 116 216 (expr_list:REG_DEP_TRUE (concat:DI (reg:DI 4 si)
        (symbol_ref/i:DI ("_ZTI1A")  <var_decl 0x7fcf18d9b360 _ZTI1A>))
    (expr_list:REG_DEP_TRUE (concat:DI (reg:DI 1 dx)
            (const_int 0 [0]))
        (nil))) NOTE_INSN_CALL_ARG_LOCATION)
(note 216 203 117 0 NOTE_INSN_EH_REGION_END)
(barrier 117 216 202)
...

Previously, both the barrier and the EH_REGION_END note were emitted
above CALL_ARG_LOCATION note.

2014-06-26  Uros Bizjak  <ubizjak@gmail.com>

    * except.c (emit_note_eh_region_end): New helper function.
    (convert_to_eh_region_ranges): Use emit_note_eh_region_end to
    emit EH_REGION_END note.
    * jump.c (cleanup_barriers): Do not split a call and its
    corresponding CALL_ARG_LOCATION note.

The patch was successfully bootstrapped and regression tested on
x86_64-pc-linux-gnu {,-m32} for all default languages, obj-c++ and go.
Additionally, the patch was bootstrapped and regression tested on
alpha-linux-gnu (configured with --host=alpha-linux-gnu
--build=alpha-linux-gnu --target=alpha-linux-gnu to make the trapb
insertion pass effective) together with an updated trapb insertion
patch [2], also for all default languages, obj-c++ and go.

OK for mainline?

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56858
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56858#c15

Uros.

Comments

Richard Henderson June 27, 2014, 7:11 p.m. UTC | #1 
On 06/26/2014 02:15 PM, Uros Bizjak wrote:
>     * except.c (emit_note_eh_region_end): New helper function.
>     (convert_to_eh_region_ranges): Use emit_note_eh_region_end to
>     emit EH_REGION_END note.

This bit looks good.


>    rtx insn, next, prev;
> -  for (insn = get_insns (); insn; insn = next)
> +  for (insn = get_insns (); insn; insn = NEXT_INSN (insn))
>      {
> -      next = NEXT_INSN (insn);
>        if (BARRIER_P (insn))
>  	{
>  	  prev = prev_nonnote_insn (insn);
>  	  if (!prev)
>  	    continue;
> +
> +	  /* Make sure we do not split a call and its corresponding
> +	     CALL_ARG_LOCATION note.  */
> +	  next = NEXT_INSN (prev);
> +	  if (next == NULL)
> +	    continue;
> +	  if (NOTE_P (next)
> +	      && NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION)
> +	    prev = next;
> +

This bit looks more complicated than it needs to be.
It would also be helpful for legibility to move the
declarations from the top to the first uses.

The next == NULL test can never be true, since we've
already searched backward from insn.


r~
diff mbox

Patch

Index: except.c
===================================================================
--- except.c	(revision 212052)
+++ except.c	(working copy)
@@ -2466,6 +2466,20 @@  add_call_site (rtx landing_pad, int action, int se
   return call_site_base + crtl->eh.call_site_record_v[section]->length () - 1;
 }
 
+static rtx
+emit_note_eh_region_end (rtx insn)
+{
+  rtx next = NEXT_INSN (insn);
+
+  /* Make sure we do not split a call and its corresponding
+     CALL_ARG_LOCATION note.  */
+  if (next && NOTE_P (next)
+      && NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION)
+    insn = next;
+
+  return emit_note_after (NOTE_INSN_EH_REGION_END, insn);
+}
+
 /* Turn REG_EH_REGION notes back into NOTE_INSN_EH_REGION notes.
    The new note numbers will not refer to region numbers, but
    instead to call site entries.  */
@@ -2544,8 +2558,8 @@  convert_to_eh_region_ranges (void)
 		note = emit_note_before (NOTE_INSN_EH_REGION_BEG,
 					 first_no_action_insn_before_switch);
 		NOTE_EH_HANDLER (note) = call_site;
-		note = emit_note_after (NOTE_INSN_EH_REGION_END,
-					last_no_action_insn_before_switch);
+		note
+		  = emit_note_eh_region_end (last_no_action_insn_before_switch);
 		NOTE_EH_HANDLER (note) = call_site;
 		gcc_assert (last_action != -3
 			    || (last_action_insn
@@ -2569,8 +2583,7 @@  convert_to_eh_region_ranges (void)
 		    first_no_action_insn = NULL_RTX;
 		  }
 
-		note = emit_note_after (NOTE_INSN_EH_REGION_END,
-					last_action_insn);
+		note = emit_note_eh_region_end (last_action_insn);
 		NOTE_EH_HANDLER (note) = call_site;
 	      }
 
@@ -2617,7 +2630,7 @@  convert_to_eh_region_ranges (void)
 
   if (last_action >= -1 && ! first_no_action_insn)
     {
-      note = emit_note_after (NOTE_INSN_EH_REGION_END, last_action_insn);
+      note = emit_note_eh_region_end (last_action_insn);
       NOTE_EH_HANDLER (note) = call_site;
     }
 
Index: jump.c
===================================================================
--- jump.c	(revision 212052)
+++ jump.c	(working copy)
@@ -122,14 +122,23 @@  static unsigned int
 cleanup_barriers (void)
 {
   rtx insn, next, prev;
-  for (insn = get_insns (); insn; insn = next)
+  for (insn = get_insns (); insn; insn = NEXT_INSN (insn))
     {
-      next = NEXT_INSN (insn);
       if (BARRIER_P (insn))
 	{
 	  prev = prev_nonnote_insn (insn);
 	  if (!prev)
 	    continue;
+
+	  /* Make sure we do not split a call and its corresponding
+	     CALL_ARG_LOCATION note.  */
+	  next = NEXT_INSN (prev);
+	  if (next == NULL)
+	    continue;
+	  if (NOTE_P (next)
+	      && NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION)
+	    prev = next;
+
 	  if (BARRIER_P (prev))
 	    delete_insn (insn);
 	  else if (prev != PREV_INSN (insn))