public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [RFC] Sanitize rtx_addr_can_trap_p_1
@ 2015-06-11  7:48 Bernd Edlinger
  2015-06-11  8:40 ` Jakub Jelinek
  2015-06-15  9:03 ` Bernd Edlinger
  0 siblings, 2 replies; 15+ messages in thread
From: Bernd Edlinger @ 2015-06-11  7:48 UTC (permalink / raw)
  To: gcc-patches; +Cc: Richard Biener, Jakub Jelinek, Jeff Law, Eric Botcazou

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

Hi,

it was exactly to the day one year ago, when I posted this patch the first time:
[PATCH] PR rtl-optimization/61047: https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00996.html

The PR was suspended, but the discussion did not stop.  And I personally still see a bug like this
as an in-acceptable security risk for embedded safety systems.

The problem is, that when rtx_addr_can_trap_p_1 returns 0, that means for the code generation
that this instruction can be moved out of any guarding if-block when that seems profitable.

This is a latent wrong code generation bug, that happens mostly in machine generated compiler-test code,
but the point is, we can never be sure, that this is impossible to happen in "real" code.

So I would like to fix this now, because there are quite a few duplicated reports of the same
bug already, and because refusing to fix a known wrong code bug is just not our style.


I have boot-strapped and regression tested the patch with all languages again on x86_64-linux-gnu.
The patch still works unmodified, and all I would have to change on the original patch files
will be the year: s/2014/2015/g ;)


Thanks
Bernd.
 		 	   		  

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

gcc/ChangeLog:
2014-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:
2014-06-11  Bernd Edlinger  <bernd.edlinger@hotmail.de>

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


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

Index: gcc/rtlanal.c
===================================================================
--- gcc/rtlanal.c	(revision 211341)
+++ gcc/rtlanal.c	(working copy)
@@ -224,6 +224,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 ();
+#ifndef 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 ();
+#ifndef 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
@@ -301,29 +440,117 @@ 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);
+
+#ifdef STACK_GROWS_DOWNWARD
+	      low_bound  = sp_offset - red_zone_size - stack_boundary;
+	      high_bound = ap_offset
+			   + FIRST_PARM_OFFSET (current_function_decl)
+#ifndef 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)
+#ifdef 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);
+
+#ifdef STACK_GROWS_DOWNWARD
+	      low_bound  = - red_zone_size - stack_boundary;
+	      high_bound = ap_offset
+			   + FIRST_PARM_OFFSET (current_function_decl)
+#ifndef 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)
+#ifdef 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.  */
+#ifdef 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;
+#if 0
+	  fprintf (stderr, "*** %s can trap: 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",
+		   offset, size, low_bound, high_bound);
+#endif
+	  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/20140611-1.c
===================================================================
--- gcc/testsuite/gcc.c-torture/execute/20140611-1.c	(revision 0)
+++ gcc/testsuite/gcc.c-torture/execute/20140611-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;
+}

^ permalink raw reply	[flat|nested] 15+ messages in thread
end of thread, other threads:[~2015-07-01 12:35 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-11  7:48 [RFC] Sanitize rtx_addr_can_trap_p_1 Bernd Edlinger
2015-06-11  8:40 ` Jakub Jelinek
2015-06-11 10:44   ` Bernd Edlinger
2015-06-11 10:57     ` Richard Biener
2015-06-11 16:04       ` Bernd Edlinger
2015-06-11 11:00     ` Jakub Jelinek
2015-06-11 14:38       ` Bernd Edlinger
2015-06-11 14:40         ` Richard Biener
2015-06-11 15:55           ` Bernd Edlinger
2015-06-11 11:29   ` Eric Botcazou
2015-06-11 11:57     ` Jakub Jelinek
2015-06-12  9:24     ` Bernd Edlinger
2015-06-15  9:03 ` Bernd Edlinger
2015-07-01 12:31   ` [PING] " Bernd Edlinger
2015-07-01 12:35     ` Jakub Jelinek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).