diff mbox

Fix combiner from accessing or writing out of bounds SET_N_REGS (PR other/63504)

Message ID 20150202185446.GR1746@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Feb. 2, 2015, 6:54 p.m. UTC 
Hi!

During combine we sometimes split instructions and on some target that can
create new pseudos.

combine_split_insns in that case grows the reg_stat vector, but REG_N_SETS
lives in an array allocated by regstat.c once and it isn't able right now to
grow it.  Even adding the ability to grow that (by making it a vector)
wouldn't help much though, because the question is what to initialize it
with, DF isn't updated when we split and it isn't sure if we actually keep
the split insns.  From what I can see in the combiner, using
DF_REG_DEF_COUNT directly would probably not work, because combiner wants to
adjust its counts during try_combine, rather than only when it is done with
try_combine and DF is updated.

So, this patch instead just stops touching (reading and/or writing) REG_N_SETS
of pseudos created during combine.  Combiner only cares about REG_N_SETS
(...) == 1, so the patch is conservative, but something that previously
resulted in pretty much random behavior.

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

2015-02-02  Jakub Jelinek  <jakub@redhat.com>

	PR other/63504
	* combine.c (reg_n_sets_max): New variable.
	(can_change_dest_mode, reg_nonzero_bits_for_combine,
	reg_num_sign_bit_copies_for_combine, get_last_value_validate,
	get_last_value): Use REG_N_SETS only on pseudos < reg_n_sets_max.
	(try_combine): Use INC_REG_N_SETS only on pseudos < reg_n_sets_max.
	(rest_of_handle_combine): Initialize reg_n_sets_max.


	Jakub

Comments

Segher Boessenkool Feb. 2, 2015, 11:26 p.m. UTC | #1 
On Mon, Feb 02, 2015 at 07:54:46PM +0100, Jakub Jelinek wrote:
> +/* Highest pseudo for which we track REG_N_SETS.  */
> +static unsigned int reg_n_sets_max;

One more than the highest reg num, actually.

Looks fine otherwise :-)


Segher
Jakub Jelinek Feb. 3, 2015, 7:20 a.m. UTC | #2 
On Mon, Feb 02, 2015 at 05:26:23PM -0600, Segher Boessenkool wrote:
> On Mon, Feb 02, 2015 at 07:54:46PM +0100, Jakub Jelinek wrote:
> > +/* Highest pseudo for which we track REG_N_SETS.  */
> > +static unsigned int reg_n_sets_max;
> 
> One more than the highest reg num, actually.

Changed in my copy to
/* One plus the highest pseudo for which we track REG_N_SETS.  */

Ok with that change?

	Jakub
Eric Botcazou Feb. 3, 2015, 9:23 a.m. UTC | #3 
> Changed in my copy to
> /* One plus the highest pseudo for which we track REG_N_SETS.  */
> 
> Ok with that change?

OK if you also add (part of) the explanation you wrote down in the opening 
message to the above comment.
H.J. Lu Feb. 3, 2015, 3:43 p.m. UTC | #4 
On Mon, Feb 2, 2015 at 11:20 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Feb 02, 2015 at 05:26:23PM -0600, Segher Boessenkool wrote:
>> On Mon, Feb 02, 2015 at 07:54:46PM +0100, Jakub Jelinek wrote:
>> > +/* Highest pseudo for which we track REG_N_SETS.  */
>> > +static unsigned int reg_n_sets_max;
>>
>> One more than the highest reg num, actually.
>
> Changed in my copy to
> /* One plus the highest pseudo for which we track REG_N_SETS.  */
>
> Ok with that change?
>
>         Jakub

This may have caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64921
diff mbox

Patch

--- gcc/combine.c.jj	2015-01-31 10:07:45.000000000 +0100
+++ gcc/combine.c	2015-02-02 13:33:06.190821688 +0100
@@ -284,6 +284,9 @@  typedef struct reg_stat_struct {
 
 static vec<reg_stat_type> reg_stat;
 
+/* Highest pseudo for which we track REG_N_SETS.  */
+static unsigned int reg_n_sets_max;
+
 /* Record the luid of the last insn that invalidated memory
    (anything that writes memory, and subroutine calls, but not pushes).  */
 
@@ -2420,7 +2423,9 @@  can_change_dest_mode (rtx x, int added_s
 		>= hard_regno_nregs[regno][mode]));
 
   /* Or a pseudo that is only used once.  */
-  return (REG_N_SETS (regno) == 1 && !added_sets
+  return (regno < reg_n_sets_max
+	  && REG_N_SETS (regno) == 1
+	  && !added_sets
 	  && !REG_USERVAR_P (x));
 }
 
@@ -3630,7 +3635,8 @@  try_combine (rtx_insn *i3, rtx_insn *i2,
 
 	      if (REG_P (new_i3_dest)
 		  && REG_P (new_i2_dest)
-		  && REGNO (new_i3_dest) == REGNO (new_i2_dest))
+		  && REGNO (new_i3_dest) == REGNO (new_i2_dest)
+		  && REGNO (new_i2_dest) < reg_n_sets_max)
 		INC_REG_N_SETS (REGNO (new_i2_dest), 1);
 	    }
 	}
@@ -4480,7 +4486,8 @@  try_combine (rtx_insn *i3, rtx_insn *i2,
 	   zero its use count so it won't make `reload' do any work.  */
 	if (! added_sets_2
 	    && (newi2pat == 0 || ! reg_mentioned_p (i2dest, newi2pat))
-	    && ! i2dest_in_i2src)
+	    && ! i2dest_in_i2src
+	    && REGNO (i2dest) < reg_n_sets_max)
 	  INC_REG_N_SETS (REGNO (i2dest), -1);
       }
 
@@ -4497,7 +4504,9 @@  try_combine (rtx_insn *i3, rtx_insn *i2,
 
 	record_value_for_reg (i1dest, i1_insn, i1_val);
 
-	if (! added_sets_1 && ! i1dest_in_i1src)
+	if (! added_sets_1
+	    && ! i1dest_in_i1src
+	    && REGNO (i1dest) < reg_n_sets_max)
 	  INC_REG_N_SETS (REGNO (i1dest), -1);
       }
 
@@ -4514,7 +4523,9 @@  try_combine (rtx_insn *i3, rtx_insn *i2,
 
 	record_value_for_reg (i0dest, i0_insn, i0_val);
 
-	if (! added_sets_0 && ! i0dest_in_i0src)
+	if (! added_sets_0
+	    && ! i0dest_in_i0src
+	    && REGNO (i0dest) < reg_n_sets_max)
 	  INC_REG_N_SETS (REGNO (i0dest), -1);
       }
 
@@ -9750,6 +9761,7 @@  reg_nonzero_bits_for_combine (const_rtx
 	  || (rsp->last_set_label == label_tick
               && DF_INSN_LUID (rsp->last_set) < subst_low_luid)
 	  || (REGNO (x) >= FIRST_PSEUDO_REGISTER
+	      && REGNO (x) < reg_n_sets_max
 	      && REG_N_SETS (REGNO (x)) == 1
 	      && !REGNO_REG_SET_P
 		  (DF_LR_IN (ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb),
@@ -9825,6 +9837,7 @@  reg_num_sign_bit_copies_for_combine (con
 	  || (rsp->last_set_label == label_tick
               && DF_INSN_LUID (rsp->last_set) < subst_low_luid)
 	  || (REGNO (x) >= FIRST_PSEUDO_REGISTER
+	      && REGNO (x) < reg_n_sets_max
 	      && REG_N_SETS (REGNO (x)) == 1
 	      && !REGNO_REG_SET_P
 		  (DF_LR_IN (ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb),
@@ -12863,6 +12876,7 @@  get_last_value_validate (rtx *loc, rtx_i
 	      /* If this is a pseudo-register that was only set once and not
 		 live at the beginning of the function, it is always valid.  */
 	      || (! (regno >= FIRST_PSEUDO_REGISTER
+		     && regno < reg_n_sets_max
 		     && REG_N_SETS (regno) == 1
 		     && (!REGNO_REG_SET_P
 			 (DF_LR_IN (ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb),
@@ -12979,6 +12993,7 @@  get_last_value (const_rtx x)
   if (value == 0
       || (rsp->last_set_label < label_tick_ebb_start
 	  && (regno < FIRST_PSEUDO_REGISTER
+	      || regno >= reg_n_sets_max
 	      || REG_N_SETS (regno) != 1
 	      || REGNO_REG_SET_P
 		 (DF_LR_IN (ENTRY_BLOCK_PTR_FOR_FN (cfun)->next_bb), regno))))
@@ -14166,6 +14181,7 @@  rest_of_handle_combine (void)
   df_analyze ();
 
   regstat_init_n_sets_and_refs ();
+  reg_n_sets_max = max_reg_num ();
 
   rebuild_jump_labels_after_combine
     = combine_instructions (get_insns (), max_reg_num ());