[RFC] Sanitize rtx_addr_can_trap_p_1

[RFC] Sanitize rtx_addr_can_trap_p_1

[Bernd Edlinger]

[-- 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;
+}

Re: [RFC] Sanitize rtx_addr_can_trap_p_1

[Jakub Jelinek]

On Thu, Jun 11, 2015 at 09:42:38AM +0200, Bernd Edlinger wrote:
> 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 ;)

IMHO the
#if 0
#endif
stuff doesn't belong to the patch.

Other than that, as I said already in the PR, I'm in favor of applying it to
the trunk (only, not release branches) and watching for performance and/or
wrong-code issues, but Eric is against it.  What do others think about it?

From John Regehr's talk at GCC Summit a few years ago I got the
impression that for people to be able to effectively report bugs in the
compiler through code generator it is important that discovered bugs in the
compiler are fixed timely, otherwise it makes life to the reporters much
harder, because then they'll run into the same still unfixed issue all the
time.

> 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.

	Jakub

RE: [RFC] Sanitize rtx_addr_can_trap_p_1

[Bernd Edlinger]

Hi,

On Thu, 11 Jun 2015 10:02:03, Jakub Jelinek wrote:
>
> IMHO the
> #if 0
> #endif
> stuff doesn't belong to the patch.
>

I just wanted to leave a hint, how I debugged this function, and how
to assess the performance of the decision that is taken here.

I mean, the boot-strap would certainly pass, if I always return 0 here,
but Eric would'nt like it.

I believe that, when the offset lies within the bounds that are implied by
the current function's stack frame, the access will always be safe.

But there are some very rare false positives, when this function returns 0
on "normal" code, like gcc source code itself, and they are interesting to debug.

Should I better change the #if 0 block into a comment?


> Other than that, as I said already in the PR, I'm in favor of applying it to
> the trunk (only, not release branches) and watching for performance and/or
> wrong-code issues, but Eric is against it. What do others think about it?
>
> From John Regehr's talk at GCC Summit a few years ago I got the
> impression that for people to be able to effectively report bugs in the
> compiler through code generator it is important that discovered bugs in the
> compiler are fixed timely, otherwise it makes life to the reporters much
> harder, because then they'll run into the same still unfixed issue all the
> time.
>

On that, I totally agree.


Thanks
Bernd.
 		 	   		  

Re: [RFC] Sanitize rtx_addr_can_trap_p_1

[Richard Biener]

On Thu, Jun 11, 2015 at 12:38 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
> On Thu, 11 Jun 2015 10:02:03, Jakub Jelinek wrote:
>>
>> IMHO the
>> #if 0
>> #endif
>> stuff doesn't belong to the patch.
>>
>
> I just wanted to leave a hint, how I debugged this function, and how
> to assess the performance of the decision that is taken here.
>
> I mean, the boot-strap would certainly pass, if I always return 0 here,
> but Eric would'nt like it.
>
> I believe that, when the offset lies within the bounds that are implied by
> the current function's stack frame, the access will always be safe.
>
> But there are some very rare false positives, when this function returns 0
> on "normal" code, like gcc source code itself, and they are interesting to debug.
>
> Should I better change the #if 0 block into a comment?

Yes.  If you have testcases for those rare false positives it would be nice to
reduce them and at least archieve them in bugzilla.

>
>> Other than that, as I said already in the PR, I'm in favor of applying it to
>> the trunk (only, not release branches) and watching for performance and/or
>> wrong-code issues, but Eric is against it. What do others think about it?
>>
>> From John Regehr's talk at GCC Summit a few years ago I got the
>> impression that for people to be able to effectively report bugs in the
>> compiler through code generator it is important that discovered bugs in the
>> compiler are fixed timely, otherwise it makes life to the reporters much
>> harder, because then they'll run into the same still unfixed issue all the
>> time.
>>
>
> On that, I totally agree.

I also think we need to be conservative.  I didn't look at the patch in detail
to check whether we are indeed conservative here (what about offsets
that are not visibly constant like for if (n > m) ... a[m];?).

Richard.

>
> Thanks
> Bernd.
>

Re: [RFC] Sanitize rtx_addr_can_trap_p_1

[Jakub Jelinek]

On Thu, Jun 11, 2015 at 12:38:40PM +0200, Bernd Edlinger wrote:
> On Thu, 11 Jun 2015 10:02:03, Jakub Jelinek wrote:
> >
> > IMHO the
> > #if 0
> > #endif
> > stuff doesn't belong to the patch.
> >
> 
> I just wanted to leave a hint, how I debugged this function, and how
> to assess the performance of the decision that is taken here.

What I usually do in these cases is something like:
FILE *f = fopen ("/tmp/mylogfile", "a");
fprintf (f, "%s %d ...\n", main_input_filename ? main_input_filename : "-", (int) BITS_PER_WORD, ...);
fclose (f);
and do full bootstrap/regtest (usually both x86_64-linux and i686-linux)
with it, then look at the log file.
But I keep those for myself, don't keep them even as comments.
In this case, you could post the hack as incremental patch for interested
folks to test on their architecture, but I'm not convinced we want to keep
it in the source, whether #if 0 or in a comment.

So, for a full bootstrap/regtest, how many log messages do you get, and are
they always resolved conservatively (i.e. if unsure the offset is ok, return
1)?

	Jakub

Re: [RFC] Sanitize rtx_addr_can_trap_p_1

[Eric Botcazou]

> Other than that, as I said already in the PR, I'm in favor of applying it to
> the trunk (only, not release branches) and watching for performance and/or
> wrong-code issues, but Eric is against it.  What do others think about it?

Yes, I'm against it, I think that the patch will introduce more issues, and on 
real-life software this time, than it fixes, but you just need to a second 
maintainer to overrule me.

> From John Regehr's talk at GCC Summit a few years ago I got the
> impression that for people to be able to effectively report bugs in the
> compiler through code generator it is important that discovered bugs in the
> compiler are fixed timely, otherwise it makes life to the reporters much
> harder, because then they'll run into the same still unfixed issue all the
> time.

This one is very peculiar though, you need a blatantly out-of-bounds array 
access that is unreachable, so you can avoid it if you avoid generating 
nonsensical code with your code generator.

-- 
Eric Botcazou

Re: [RFC] Sanitize rtx_addr_can_trap_p_1

[Jakub Jelinek]

On Thu, Jun 11, 2015 at 01:17:47PM +0200, Eric Botcazou wrote:
> > Other than that, as I said already in the PR, I'm in favor of applying it to
> > the trunk (only, not release branches) and watching for performance and/or
> > wrong-code issues, but Eric is against it.  What do others think about it?
> 
> Yes, I'm against it, I think that the patch will introduce more issues, and on 
> real-life software this time, than it fixes, but you just need to a second 
> maintainer to overrule me.
> 
> > From John Regehr's talk at GCC Summit a few years ago I got the
> > impression that for people to be able to effectively report bugs in the
> > compiler through code generator it is important that discovered bugs in the
> > compiler are fixed timely, otherwise it makes life to the reporters much
> > harder, because then they'll run into the same still unfixed issue all the
> > time.
> 
> This one is very peculiar though, you need a blatantly out-of-bounds array 
> access that is unreachable, so you can avoid it if you avoid generating 
> nonsensical code with your code generator.

But it doesn't have to be any out of bounds array access in the source actually.
It can be array access indexed by variable that just can be compared to a
very large value and compiler for some reason decides to jump-thread it.
Say an integral variable holding array indexes or a special value (think
0xdeadbeef) for some special handling.  It is enough for the GIMPLE
optimizers not to be able to figure out more complicated control flow
and suddenly you have a dead array[0xdeadbeef] access somewhere, which can
be if-converted to a crashing instruction if things just go wrong.  Just
look how many -Warray-bounds warning false-positives we used to emit before
that warning has been turn off in VRP2 - where the warning used to warn on
out of bounds accesses in dead code.

	Jakub

RE: [RFC] Sanitize rtx_addr_can_trap_p_1

[Bernd Edlinger]

Hi,

On Thu, 11 Jun 2015 12:57:34 +0200, Jakub Jelinek wrote:
>
> On Thu, Jun 11, 2015 at 12:38:40PM +0200, Bernd Edlinger wrote:
>> On Thu, 11 Jun 2015 10:02:03, Jakub Jelinek wrote:
>>>
>>> IMHO the
>>> #if 0
>>> #endif
>>> stuff doesn't belong to the patch.
>>>
>>
>> I just wanted to leave a hint, how I debugged this function, and how
>> to assess the performance of the decision that is taken here.
>
> What I usually do in these cases is something like:
> FILE *f = fopen ("/tmp/mylogfile", "a");
> fprintf (f, "%s %d ...\n", main_input_filename ? main_input_filename : "-", (int) BITS_PER_WORD, ...);
> fclose (f);
> and do full bootstrap/regtest (usually both x86_64-linux and i686-linux)
> with it, then look at the log file.
> But I keep those for myself, don't keep them even as comments.
> In this case, you could post the hack as incremental patch for interested
> folks to test on their architecture, but I'm not convinced we want to keep
> it in the source, whether #if 0 or in a comment.
>

I am not too sure about it either.

But I think, it is quite helpful data, however I am even tempted
to add the name of the current function, and the pass we are in at the moment,
but I have no idea how to grab that information...

> So, for a full bootstrap/regtest, how many log messages do you get, and are
> they always resolved conservatively (i.e. if unsure the offset is ok, return
> 1)?
>


In stage 2 of the build (with all languages) I get:

2930 messages of the form
*** frame can trap: offset=16, size=8, low_bound=-3152, high_bound=0

74 messages of the form
*** sp can trap: offset=112, size=4, low_bound=-144, high_bound=112

202 messages of the from
*** argp can trap: offset=16, size=8, low_bound=-56, high_bound=16

10 messages of the form
*** fp can trap: offset=40, size=4, low_bound=-264, high_bound=24


My patch does not change the handling of frame_pointer_rtx,
except that it avoids a possible integer overflow in "adj_offset + size - 1>= 0"
so these 2930 suppressed optimizations were already introduced by Eric's patch.

I think that is probably a new effect, that [FP+x] is now used more
often than before to access values at [ARGP+x].  I have not tried, but
maybe it would be possible to use the crtl->args.size, here too, to get more
optimistic upper bounds on the argument sizes.


So all in all my patch changed 286 times the return value of rtx_addr_can_trap_p_1
in the whole pass 2.

But OTOH there are millions of times, where the rtx_addr_can_trap_p_1
returns 0, which is rtx can not trap.


Bernd.
 		 	   		  

Re: [RFC] Sanitize rtx_addr_can_trap_p_1

[Richard Biener]

On Thu, Jun 11, 2015 at 4:34 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
> On Thu, 11 Jun 2015 12:57:34 +0200, Jakub Jelinek wrote:
>>
>> On Thu, Jun 11, 2015 at 12:38:40PM +0200, Bernd Edlinger wrote:
>>> On Thu, 11 Jun 2015 10:02:03, Jakub Jelinek wrote:
>>>>
>>>> IMHO the
>>>> #if 0
>>>> #endif
>>>> stuff doesn't belong to the patch.
>>>>
>>>
>>> I just wanted to leave a hint, how I debugged this function, and how
>>> to assess the performance of the decision that is taken here.
>>
>> What I usually do in these cases is something like:
>> FILE *f = fopen ("/tmp/mylogfile", "a");
>> fprintf (f, "%s %d ...\n", main_input_filename ? main_input_filename : "-", (int) BITS_PER_WORD, ...);
>> fclose (f);
>> and do full bootstrap/regtest (usually both x86_64-linux and i686-linux)
>> with it, then look at the log file.
>> But I keep those for myself, don't keep them even as comments.
>> In this case, you could post the hack as incremental patch for interested
>> folks to test on their architecture, but I'm not convinced we want to keep
>> it in the source, whether #if 0 or in a comment.
>>
>
> I am not too sure about it either.
>
> But I think, it is quite helpful data, however I am even tempted
> to add the name of the current function, and the pass we are in at the moment,
> but I have no idea how to grab that information...
>
>> So, for a full bootstrap/regtest, how many log messages do you get, and are
>> they always resolved conservatively (i.e. if unsure the offset is ok, return
>> 1)?
>>
>
>
> In stage 2 of the build (with all languages) I get:
>
> 2930 messages of the form
> *** frame can trap: offset=16, size=8, low_bound=-3152, high_bound=0
>
> 74 messages of the form
> *** sp can trap: offset=112, size=4, low_bound=-144, high_bound=112
>
> 202 messages of the from
> *** argp can trap: offset=16, size=8, low_bound=-56, high_bound=16
>
> 10 messages of the form
> *** fp can trap: offset=40, size=4, low_bound=-264, high_bound=24
>
>
> My patch does not change the handling of frame_pointer_rtx,
> except that it avoids a possible integer overflow in "adj_offset + size - 1>= 0"
> so these 2930 suppressed optimizations were already introduced by Eric's patch.
>
> I think that is probably a new effect, that [FP+x] is now used more
> often than before to access values at [ARGP+x].  I have not tried, but
> maybe it would be possible to use the crtl->args.size, here too, to get more
> optimistic upper bounds on the argument sizes.
>
>
> So all in all my patch changed 286 times the return value of rtx_addr_can_trap_p_1
> in the whole pass 2.
>
> But OTOH there are millions of times, where the rtx_addr_can_trap_p_1
> returns 0, which is rtx can not trap.

Sounds like a red-zone is not accounted for?

Richard.

>
> Bernd.
>

RE: [RFC] Sanitize rtx_addr_can_trap_p_1

[Bernd Edlinger]

Hi,

On Thu, 11 Jun 2015 16:38:23, Richard Biener wrote:
>
> Sounds like a red-zone is not accounted for?
>

No, I know about the red-zone:

+#ifdef RED_ZONE_SIZE
+         HOST_WIDE_INT red_zone_size = RED_ZONE_SIZE;
+#else
+         HOST_WIDE_INT red_zone_size = 0;
+#endif


otherwise there would be many more drop outs with [SP-x].


Bernd.
 		 	   		  

RE: [RFC] Sanitize rtx_addr_can_trap_p_1

[Bernd Edlinger]

Hi,

On Thu, 11 Jun 2015 12:56:50, Richard Biener wrote:
>
> I also think we need to be conservative. I didn't look at the patch in detail
> to check whether we are indeed conservative here (what about offsets
> that are not visibly constant like for if (n> m) ... a[m];?).
>

we only handle constant values for m, if m does not look like a constant we
use pointer arithmetic, and that can always trap.

Bernd.
 		 	   		  

RE: [RFC] Sanitize rtx_addr_can_trap_p_1

[Bernd Edlinger]

Hi,

On Thu, 11 Jun 2015 13:17:47 +0200, Eric Botcazou wrote:
>
>> Other than that, as I said already in the PR, I'm in favor of applying it to
>> the trunk (only, not release branches) and watching for performance and/or
>> wrong-code issues, but Eric is against it. What do others think about it?
>
> Yes, I'm against it, I think that the patch will introduce more issues, and on
> real-life software this time, than it fixes, but you just need to a second
> maintainer to overrule me.
>


I think I can say, my patch does not change the return value on frame_pointer_rtx.
So that will stay as before (identical as with your patch).

BTW: meanwhile I found out, that all these 2930 accesses to frame_pointer_rtx with positive
offset seem to happen in the reload pass.  That's funny, because I think that did not happen
when I looked at it the last time, and I do not yet understand why this happens now.

The worst thing that can happen on the other base registers, would be a few disabled
if conversions, on offsets that exceed the safety margins...

Of course, it would be good if some one with deeper insight than myself would look at
the patch in detail if the assumptions are actually safe.  I believe they are.

But one thing is sure, really nonsense values like 0xDEADBEEF will be outside the margins,
and that is not the case right now.


Thanks
Bernd.
 		 	   		  

RE: [RFC] Sanitize rtx_addr_can_trap_p_1

[Bernd Edlinger]

[-- 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.  */

RE: [PING] [RFC] Sanitize rtx_addr_can_trap_p_1

[Bernd Edlinger]

Hi,


the bogus offsets-issue is now entered in bugzilla, see PR66614.

It is however only a very minor issue, and does probably have
no impact on the generated code at all.


How should I continue with the rtx_addr_can_trap-patch?
Is it OK to commit?


Thanks
Bernd.


----------------------------------------
> From: bernd.edlinger@hotmail.de
> To: gcc-patches@gcc.gnu.org
> CC: richard.guenther@gmail.com; jakub@redhat.com; law@redhat.com; ebotcazou@adacore.com
> Subject: RE: [RFC] Sanitize rtx_addr_can_trap_p_1
> Date: Mon, 15 Jun 2015 05:50:46 +0200
>
> 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.
>
 		 	   		  

Re: [PING] [RFC] Sanitize rtx_addr_can_trap_p_1

[Jakub Jelinek]

On Wed, Jul 01, 2015 at 02:31:41PM +0200, Bernd Edlinger wrote:
> the bogus offsets-issue is now entered in bugzilla, see PR66614.
> 
> It is however only a very minor issue, and does probably have
> no impact on the generated code at all.
> 
> 
> How should I continue with the rtx_addr_can_trap-patch?
> Is it OK to commit?

Please commit it (the non-debug patch only of course), but watch
for fallout.  Thanks.

	Jakub