RE: [RFC] Sanitize rtx_addr_can_trap_p_1

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

[-- Attachment #1: Type: text/plain, Size: 4200 bytes --]

Hi,

I have here an updated patch, which improves two things:

First it moves debug code out to an extra patch, as suggested by Jakub.

Secondly, it fixes the checks on STACK_GROWS_DOWNWARD and
ARGS_GROW_DOWNWARD.  Previously these used to be conditionally defined
symbols, but recently they were changed to be always defined, but with 0 or 1.

That made all #ifndef checks on those two flags work the wrong way, and it caused
most of the false positives in the previous version.

Now the number of false positives in the stage2 drops significantly to only 4 new ones:

*** argp can trap: function=uw_init_context_1, pass=reload, offset=236, size=4, low_bound=-16, high_bound=16
*** argp can trap: function=uw_init_context_1, pass=reload, offset=236, size=4, low_bound=-16, high_bound=16
*** argp can trap: function=uw_init_context_1, pass=reload, offset=440, size=8, low_bound=-16, high_bound=16
*** argp can trap: function=uw_init_context_1, pass=reload, offset=440, size=8, low_bound=-16, high_bound=16

These came from libgcc/unwind-dw2.c

They seem to have the same reason as the 2930 "frame can trap" warnings that were already
there before the patch.


All these seem to happen due to some hidden bug.  In the debugger the call stack looks always like this:

#0  rtx_addr_can_trap_p_1 (x=0x7ffff6ecb378, offset=440, size=8, mode=DImode, unaligned_mems=false) at ../../gcc-trunk/gcc/rtlanal.c:671
#1  0x0000000000d90f4a in rtx_addr_can_trap_p_1 (x=0x7ffff67ac7f8, offset=0, size=8, mode=DImode, unaligned_mems=false) at ../../gcc-trunk/gcc/rtlanal.c:699
#2  0x0000000000d953c4 in may_trap_p_1 (x=0x7ffff67ac810, flags=0) at ../../gcc-trunk/gcc/rtlanal.c:2760
#3  0x0000000000d95619 in may_trap_p_1 (x=0x7ffff671ac90, flags=0) at ../../gcc-trunk/gcc/rtlanal.c:2838
#4  0x0000000000d956cc in may_trap_p (x=0x7ffff671ac90) at ../../gcc-trunk/gcc/rtlanal.c:2857
#5  0x0000000000c6b2ab in process_bb_lives (bb=0x7ffff6c3a958, curr_point=@0x7fffffffd8e4: 23, dead_insn_p=true) at ../../gcc-trunk/gcc/lra-lives.c:698
#6  0x0000000000c6d19a in lra_create_live_ranges_1 (all_p=true, dead_insn_p=true) at ../../gcc-trunk/gcc/lra-lives.c:1262
#7  0x0000000000c6d47c in lra_create_live_ranges (all_p=true, dead_insn_p=true) at ../../gcc-trunk/gcc/lra-lives.c:1327
#8  0x0000000000c4a253 in lra (f=0x24f7940) at ../../gcc-trunk/gcc/lra.c:2309
#9  0x0000000000bf3d25 in do_reload () at ../../gcc-trunk/gcc/ira.c:5401
#10 0x0000000000bf40d3 in (anonymous namespace)::pass_reload::execute (this=0x23fce20) at ../../gcc-trunk/gcc/ira.c:5572
#11 0x0000000000d08e37 in execute_one_pass (pass=0x23fce20) at ../../gcc-trunk/gcc/passes.c:2359
#12 0x0000000000d09081 in execute_pass_list_1 (pass=0x23fce20) at ../../gcc-trunk/gcc/passes.c:2412
#13 0x0000000000d090b2 in execute_pass_list_1 (pass=0x23fbda0) at ../../gcc-trunk/gcc/passes.c:2413
#14 0x0000000000d090ef in execute_pass_list (fn=0x7ffff7044c78, pass=0x23f8bc0) at ../../gcc-trunk/gcc/passes.c:2423
#15 0x00000000008de80c in cgraph_node::expand (this=0x7ffff6c09498) at ../../gcc-trunk/gcc/cgraphunit.c:1937
#16 0x00000000008dee3d in expand_all_functions () at ../../gcc-trunk/gcc/cgraphunit.c:2073
#17 0x00000000008df954 in symbol_table::compile (this=0x7ffff6ecf000) at ../../gcc-trunk/gcc/cgraphunit.c:2426
#18 0x00000000008dfb68 in symbol_table::finalize_compilation_unit (this=0x7ffff6ecf000) at ../../gcc-trunk/gcc/cgraphunit.c:2513
#19 0x0000000000e094ba in compile_file () at ../../gcc-trunk/gcc/toplev.c:580
#20 0x0000000000e0b9fa in do_compile () at ../../gcc-trunk/gcc/toplev.c:2070
#21 0x0000000000e0bc46 in toplev::main (this=0x7fffffffdc50, argc=35, argv=0x7fffffffdd58) at ../../gcc-trunk/gcc/toplev.c:2171
#22 0x00000000016b656d in main (argc=35, argv=0x7fffffffdd58) at ../../gcc-trunk/gcc/main.c:39

I cannot find any instruction in the rtl dumps that corresponds to this large argp offset.  So I think there must be
something wrong along the call stack above, which looks identically even on the bogus frame pointer references.


Is this patch OK for trunk now?

At least Jakub and I are in favour of it, Eric is against it.
That makes 2:1 ...


Thanks
Bernd.
 		 	   		  

[-- Attachment #2: changelog-pr60147.txt --]
[-- Type: text/plain, Size: 378 bytes --]

gcc/ChangeLog:
2015-06-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR rtl-optimization/61047
	* rtlanal.c (get_initial_register_offset): New function.
	(rtx_addr_can_trap_p_1): Check offsets of stack references.

testsuite/ChangeLog:
2015-06-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR rtl-optimization/61047
	* gcc.c-torture/execute/20150611-1.c: New testcase.


[-- Attachment #3: patch-pr61047.diff --]
[-- Type: application/octet-stream, Size: 8551 bytes --]

Index: gcc/rtlanal.c
===================================================================
--- gcc/rtlanal.c	(revision 224467)
+++ gcc/rtlanal.c	(working copy)
@@ -346,6 +346,145 @@ rtx_varies_p (const_rtx x, bool for_alias)
   return 0;
 }
 
+/* Compute an approximation for the offset between the register
+   FROM and TO for the current function, as it was at the start
+   of the routine.  */
+
+static HOST_WIDE_INT
+get_initial_register_offset (int from, int to)
+{
+#ifdef ELIMINABLE_REGS
+  static const struct elim_table_t
+  {
+    const int from;
+    const int to;
+  } table[] = ELIMINABLE_REGS;
+  HOST_WIDE_INT offset1, offset2;
+  unsigned int i, j;
+
+  if (to == from)
+    return 0;
+
+  /* It is not safe to call INITIAL_ELIMINATION_OFFSET
+     before the reload pass.  We need to give at least
+     an estimation for the resulting frame size.  */
+  if (! reload_completed)
+    {
+      offset1 = crtl->outgoing_args_size + get_frame_size ();
+#if !STACK_GROWS_DOWNWARD
+      offset1 = - offset1;
+#endif
+      if (to == STACK_POINTER_REGNUM)
+	return offset1;
+      else if (from == STACK_POINTER_REGNUM)
+	return - offset1;
+      else
+	return 0;
+     }
+
+  for (i = 0; i < ARRAY_SIZE (table); i++)
+      if (table[i].from == from)
+	{
+	  if (table[i].to == to)
+	    {
+	      INITIAL_ELIMINATION_OFFSET (table[i].from, table[i].to,
+					  offset1);
+	      return offset1;
+	    }
+	  for (j = 0; j < ARRAY_SIZE (table); j++)
+	    {
+	      if (table[j].to == to
+		  && table[j].from == table[i].to)
+		{
+		  INITIAL_ELIMINATION_OFFSET (table[i].from, table[i].to,
+					      offset1);
+		  INITIAL_ELIMINATION_OFFSET (table[j].from, table[j].to,
+					      offset2);
+		  return offset1 + offset2;
+		}
+	      if (table[j].from == to
+		  && table[j].to == table[i].to)
+		{
+		  INITIAL_ELIMINATION_OFFSET (table[i].from, table[i].to,
+					      offset1);
+		  INITIAL_ELIMINATION_OFFSET (table[j].from, table[j].to,
+					      offset2);
+		  return offset1 - offset2;
+		}
+	    }
+	}
+      else if (table[i].to == from)
+	{
+	  if (table[i].from == to)
+	    {
+	      INITIAL_ELIMINATION_OFFSET (table[i].from, table[i].to,
+					  offset1);
+	      return - offset1;
+	    }
+	  for (j = 0; j < ARRAY_SIZE (table); j++)
+	    {
+	      if (table[j].to == to
+		  && table[j].from == table[i].from)
+		{
+		  INITIAL_ELIMINATION_OFFSET (table[i].from, table[i].to,
+					      offset1);
+		  INITIAL_ELIMINATION_OFFSET (table[j].from, table[j].to,
+					      offset2);
+		  return - offset1 + offset2;
+		}
+	      if (table[j].from == to
+		  && table[j].to == table[i].from)
+		{
+		  INITIAL_ELIMINATION_OFFSET (table[i].from, table[i].to,
+					      offset1);
+		  INITIAL_ELIMINATION_OFFSET (table[j].from, table[j].to,
+					      offset2);
+		  return - offset1 - offset2;
+		}
+	    }
+	}
+
+  /* If the requested register combination was not found,
+     try a different more simple combination.  */
+  if (from == ARG_POINTER_REGNUM)
+    return get_initial_register_offset (HARD_FRAME_POINTER_REGNUM, to);
+  else if (to == ARG_POINTER_REGNUM)
+    return get_initial_register_offset (from, HARD_FRAME_POINTER_REGNUM);
+  else if (from == HARD_FRAME_POINTER_REGNUM)
+    return get_initial_register_offset (FRAME_POINTER_REGNUM, to);
+  else if (to == HARD_FRAME_POINTER_REGNUM)
+    return get_initial_register_offset (from, FRAME_POINTER_REGNUM);
+  else
+    return 0;
+
+#else
+  HOST_WIDE_INT offset;
+
+  if (to == from)
+    return 0;
+
+  if (reload_completed)
+    {
+      INITIAL_FRAME_POINTER_OFFSET (offset);
+    }
+  else
+    {
+      offset = crtl->outgoing_args_size + get_frame_size ();
+#if !STACK_GROWS_DOWNWARD
+      offset = - offset;
+#endif
+    }
+
+  if (to == STACK_POINTER_REGNUM)
+    return offset;
+  else if (from == STACK_POINTER_REGNUM)
+    return - offset;
+  else
+    return 0;
+
+#endif
+}
+
 /* Return nonzero if the use of X+OFFSET as an address in a MEM with SIZE
    bytes can cause a trap.  MODE is the mode of the MEM (not that of X) and
    UNALIGNED_MEMS controls whether nonzero is returned for unaligned memory
@@ -423,29 +562,109 @@ rtx_addr_can_trap_p_1 (const_rtx x, HOST_WIDE_INT
     case REG:
       /* Stack references are assumed not to trap, but we need to deal with
 	 nonsensical offsets.  */
-      if (x == frame_pointer_rtx)
+      if (x == frame_pointer_rtx || x == hard_frame_pointer_rtx
+	 || x == stack_pointer_rtx
+	 /* The arg pointer varies if it is not a fixed register.  */
+	 || (x == arg_pointer_rtx && fixed_regs[ARG_POINTER_REGNUM]))
 	{
-	  HOST_WIDE_INT adj_offset = offset - STARTING_FRAME_OFFSET;
+#ifdef RED_ZONE_SIZE
+	  HOST_WIDE_INT red_zone_size = RED_ZONE_SIZE;
+#else
+	  HOST_WIDE_INT red_zone_size = 0;
+#endif
+	  HOST_WIDE_INT stack_boundary = PREFERRED_STACK_BOUNDARY
+					 / BITS_PER_UNIT;
+	  HOST_WIDE_INT low_bound, high_bound;
+
 	  if (size == 0)
 	    size = GET_MODE_SIZE (mode);
-	  if (FRAME_GROWS_DOWNWARD)
+
+	  if (x == frame_pointer_rtx)
 	    {
-	      if (adj_offset < frame_offset || adj_offset + size - 1 >= 0)
-		return 1;
+	      if (FRAME_GROWS_DOWNWARD)
+		{
+		  high_bound = STARTING_FRAME_OFFSET;
+		  low_bound  = high_bound - get_frame_size ();
+		}
+	      else
+		{
+		  low_bound  = STARTING_FRAME_OFFSET;
+		  high_bound = low_bound + get_frame_size ();
+		}
 	    }
+	  else if (x == hard_frame_pointer_rtx)
+	    {
+	      HOST_WIDE_INT sp_offset
+		= get_initial_register_offset (STACK_POINTER_REGNUM,
+					       HARD_FRAME_POINTER_REGNUM);
+	      HOST_WIDE_INT ap_offset
+		= get_initial_register_offset (ARG_POINTER_REGNUM,
+					       HARD_FRAME_POINTER_REGNUM);
+
+#if STACK_GROWS_DOWNWARD
+	      low_bound  = sp_offset - red_zone_size - stack_boundary;
+	      high_bound = ap_offset
+			   + FIRST_PARM_OFFSET (current_function_decl)
+#if !ARGS_GROW_DOWNWARD
+			   + crtl->args.size
+#endif
+			   + stack_boundary;
+#else
+	      high_bound = sp_offset + red_zone_size + stack_boundary;
+	      low_bound  = ap_offset
+			   + FIRST_PARM_OFFSET (current_function_decl)
+#if ARGS_GROW_DOWNWARD
+			   - crtl->args.size
+#endif
+			   - stack_boundary;
+#endif
+	    }
+	  else if (x == stack_pointer_rtx)
+	    {
+	      HOST_WIDE_INT ap_offset
+		= get_initial_register_offset (ARG_POINTER_REGNUM,
+					       STACK_POINTER_REGNUM);
+
+#if STACK_GROWS_DOWNWARD
+	      low_bound  = - red_zone_size - stack_boundary;
+	      high_bound = ap_offset
+			   + FIRST_PARM_OFFSET (current_function_decl)
+#if !ARGS_GROW_DOWNWARD
+			   + crtl->args.size
+#endif
+			   + stack_boundary;
+#else
+	      high_bound = red_zone_size + stack_boundary;
+	      low_bound  = ap_offset
+			   + FIRST_PARM_OFFSET (current_function_decl)
+#if ARGS_GROW_DOWNWARD
+			   - crtl->args.size
+#endif
+			   - stack_boundary;
+#endif
+	    }
 	  else
 	    {
-	      if (adj_offset < 0 || adj_offset + size - 1 >= frame_offset)
-		return 1;
+	      /* We assume that accesses are safe to at least the
+		 next stack boundary.
+		 Examples are varargs and __builtin_return_address.  */
+#if ARGS_GROW_DOWNWARD
+	      high_bound = FIRST_PARM_OFFSET (current_function_decl)
+			   + stack_boundary;
+	      low_bound  = FIRST_PARM_OFFSET (current_function_decl)
+			   - crtl->args.size - stack_boundary;
+#else
+	      low_bound  = FIRST_PARM_OFFSET (current_function_decl)
+			   - stack_boundary;
+	      high_bound = FIRST_PARM_OFFSET (current_function_decl)
+			   + crtl->args.size + stack_boundary;
+#endif
 	    }
-	  return 0;
+
+	  if (offset >= low_bound && offset <= high_bound - size)
+	    return 0;
+	  return 1;
 	}
-      /* ??? Need to add a similar guard for nonsensical offsets.  */
-      if (x == hard_frame_pointer_rtx
-	  || x == stack_pointer_rtx
-	  /* The arg pointer varies if it is not a fixed register.  */
-	  || (x == arg_pointer_rtx && fixed_regs[ARG_POINTER_REGNUM]))
-	return 0;
       /* All of the virtual frame registers are stack references.  */
       if (REGNO (x) >= FIRST_VIRTUAL_REGISTER
 	  && REGNO (x) <= LAST_VIRTUAL_REGISTER)
Index: gcc/testsuite/gcc.c-torture/execute/20150611-1.c
===================================================================
--- gcc/testsuite/gcc.c-torture/execute/20150611-1.c	(revision 0)
+++ gcc/testsuite/gcc.c-torture/execute/20150611-1.c	(working copy)
@@ -0,0 +1,18 @@
+int a, c, d;
+short b;
+
+int
+main ()
+{
+  int e[1];
+  for (; b < 2; b++)
+    {
+      a = 0;
+      if (b == 28378)
+        a = e[b];
+      if (!(d || b))
+        for (; c;)
+          ;
+    }
+  return 0;
+}

[-- Attachment #4: patch-pr61047-debug.diff --]
[-- Type: application/octet-stream, Size: 1053 bytes --]

--- gcc/rtlanal.c.jj	2015-06-15 04:55:41.027812458 +0200
+++ gcc/rtlanal.c	2015-06-15 05:11:58.921821778 +0200
@@ -42,6 +42,7 @@ along with GCC; see the file COPYING3.
 #include "emit-rtl.h"  /* FIXME: Can go away once crtl is moved to rtl.h.  */
 #include "addresses.h"
 #include "rtl-iter.h"
+#include "tree-pass.h"
 
 /* Forward declarations */
 static void set_of_1 (rtx, const_rtx, void *);
@@ -663,6 +664,15 @@ rtx_addr_can_trap_p_1 (const_rtx x, HOST
 
 	  if (offset >= low_bound && offset <= high_bound - size)
 	    return 0;
+	  fprintf (stderr, "*** %s can trap: function=%s, pass=%s"
+			   ", offset=%" PRId64 ", size=%" PRId64
+			   ", low_bound=%" PRId64 ", high_bound=%" PRId64 "\n",
+		   x == frame_pointer_rtx ? "frame"
+		   : x == hard_frame_pointer_rtx ? "fp"
+		   : x == stack_pointer_rtx ? "sp" : "argp",
+		   IDENTIFIER_POINTER (DECL_NAME (current_function_decl)),
+		   current_pass->name,
+		   offset, size, low_bound, high_bound);
 	  return 1;
 	}
       /* All of the virtual frame registers are stack references.  */