Re: Reinstate generic stack checking warning with LRA

[Eric Botcazou]

> This works though, ok for trunk?
> 
> 2016-03-03  Jakub Jelinek  
> 
>   PR ada/70017
>   * gcc.dg/pr70017.c (foo): Store 0 to first element of each array.

Sure, thanks.

-- 
Eric Botcazou

Re: Reinstate generic stack checking warning with LRA

[Jakub Jelinek]

On Thu, Mar 03, 2016 at 12:31:52PM +0100, Eric Botcazou wrote:
> > Anyway, looking at pro_and_epilogue dumps, with additional
> > -fstack-protector-strong we decrease sp only by 4176, while without it by
> > 8224 (on x86_64; the testcase fails on all targets I've tried so far
> > ({x86_64,i686,powerpc64{,le},s390{,x},aarch64,armv7hl}-linux).
> 
> Yeah, the threshold is around 4KB, feel free to add a few more HUNDREDs.

I've mislooked, with -fstack-protector-strong it just has 48 bytes, and
adding many more HUNDREDs doesn't change anything.

This works though, ok for trunk?

2016-03-03  Jakub Jelinek  

PR ada/70017
* gcc.dg/pr70017.c (foo): Store 0 to first element of each array.

--- gcc/testsuite/gcc.dg/pr70017.c.jj   2016-03-02 07:39:10.0 +0100
+++ gcc/testsuite/gcc.dg/pr70017.c  2016-03-03 19:22:02.098801850 +0100
@@ -13,4 +13,8 @@ void foo(void)
 {
   HUNDRED(a)
   HUNDRED(b)
+#undef ONE
+#define ONE(s) a##s[0] = 0;
+  HUNDRED(a)
+  HUNDRED(b)
 } /* { dg-warning "frame size too large for reliable stack checking" } */


Jakub

Re: Reinstate generic stack checking warning with LRA

[Eric Botcazou]

> Anyway, looking at pro_and_epilogue dumps, with additional
> -fstack-protector-strong we decrease sp only by 4176, while without it by
> 8224 (on x86_64; the testcase fails on all targets I've tried so far
> ({x86_64,i686,powerpc64{,le},s390{,x},aarch64,armv7hl}-linux).

Yeah, the threshold is around 4KB, feel free to add a few more HUNDREDs.

-- 
Eric Botcazou

Re: Reinstate generic stack checking warning with LRA

[Jakub Jelinek]

On Thu, Mar 03, 2016 at 07:46:49AM +0100, Jakub Jelinek wrote:
> On Tue, Mar 01, 2016 at 09:04:54PM +0100, Eric Botcazou wrote:
> > When stack checking is entirely done by the middle-end (default if no 
> > specific 
> > back-end support or forced by -fstack-check=generic), the checking for the 
> > prologue is actually done in the caller with a default range and the hope 
> > is 
> > that the callee's static frame is not too large...  There are a few tricks 
> > to 
> > make it sort of work, but it's obviously not bullet proof so a warning will 
> > be 
> > issued in the problematic cases (lot of small variables).
> > 
> > But it is issued at the end of reload and LRA didn't replicate it, so it 
> > will 
> > no longer be issued e.g. for s390, as seen under PR ada/70017.  Therefore 
> > the 
> > attached patch moves it to the end of do_reload and also streamlines it a 
> > bit.
> 
> Note that the testcase fails with -fstack-protector-strong, I'm testing
> in the rpm builds with --target_board=unix\{,-fstack-protector-strong\}
> because the latter option is what we build packages with, but with
> -fstack-protector-strong there is no warning from the testcase.
> Is that a -fstack-check=generic bug, or is -fstack-check= being known
> incompatible with -fstack-protector*, or testcase bug?

None of the vars in the stack frame are actually used, so I wonder why the
expansion actually allocates anything in the stack frame for them (there is
just a CLOBBER for them).
Anyway, looking at pro_and_epilogue dumps, with additional
-fstack-protector-strong we decrease sp only by 4176, while without it by
8224 (on x86_64; the testcase fails on all targets I've tried so far
({x86_64,i686,powerpc64{,le},s390{,x},aarch64,armv7hl}-linux).

Jakub

Re: Reinstate generic stack checking warning with LRA

[Jakub Jelinek]

On Tue, Mar 01, 2016 at 09:04:54PM +0100, Eric Botcazou wrote:
> When stack checking is entirely done by the middle-end (default if no 
> specific 
> back-end support or forced by -fstack-check=generic), the checking for the 
> prologue is actually done in the caller with a default range and the hope is 
> that the callee's static frame is not too large...  There are a few tricks to 
> make it sort of work, but it's obviously not bullet proof so a warning will 
> be 
> issued in the problematic cases (lot of small variables).
> 
> But it is issued at the end of reload and LRA didn't replicate it, so it will 
> no longer be issued e.g. for s390, as seen under PR ada/70017.  Therefore the 
> attached patch moves it to the end of do_reload and also streamlines it a bit.

Note that the testcase fails with -fstack-protector-strong, I'm testing
in the rpm builds with --target_board=unix\{,-fstack-protector-strong\}
because the latter option is what we build packages with, but with
-fstack-protector-strong there is no warning from the testcase.
Is that a -fstack-check=generic bug, or is -fstack-check= being known
incompatible with -fstack-protector*, or testcase bug?

> 2016-03-01  Eric Botcazou  
> 
>   * gcc.dg/pr70017.c: New test.

Jakub

Reinstate generic stack checking warning with LRA

[Eric Botcazou]

When stack checking is entirely done by the middle-end (default if no specific 
back-end support or forced by -fstack-check=generic), the checking for the 
prologue is actually done in the caller with a default range and the hope is 
that the callee's static frame is not too large...  There are a few tricks to 
make it sort of work, but it's obviously not bullet proof so a warning will be 
issued in the problematic cases (lot of small variables).

But it is issued at the end of reload and LRA didn't replicate it, so it will 
no longer be issued e.g. for s390, as seen under PR ada/70017.  Therefore the 
attached patch moves it to the end of do_reload and also streamlines it a bit.

Tested on x86_64-suse-linux, applied on the mainline as obvious.


2016-03-01  Eric Botcazou  

PR ada/70017
* ira.c (do_reload): Issue warning for generic stack checking here...
* reload1.c (reload): ...instead of here and streamline it.


2016-03-01  Eric Botcazou  

* gcc.dg/pr70017.c: New test.

-- 
Eric BotcazouIndex: ira.c
===
--- ira.c	(revision 233840)
+++ ira.c	(working copy)
@@ -5404,9 +5404,8 @@ do_reload (void)
 {
   df_set_flags (DF_NO_INSN_RESCAN);
   build_insn_chain ();
-  
-  need_dce = reload (get_insns (), ira_conflicts_p);
 
+  need_dce = reload (get_insns (), ira_conflicts_p);
 }
 
   timevar_pop (TV_RELOAD);
@@ -5484,6 +5483,20 @@ do_reload (void)
   inform (DECL_SOURCE_LOCATION (decl), "for %qD", decl);
 }
 
+  /* If we are doing generic stack checking, give a warning if this
+ function's frame size is larger than we expect.  */
+  if (flag_stack_check == GENERIC_STACK_CHECK)
+{
+  HOST_WIDE_INT size = get_frame_size () + STACK_CHECK_FIXED_FRAME_SIZE;
+
+  for (int i = 0; i < FIRST_PSEUDO_REGISTER; i++)
+	if (df_regs_ever_live_p (i) && !fixed_regs[i] && call_used_regs[i])
+	  size += UNITS_PER_WORD;
+
+  if (size > STACK_CHECK_MAX_FRAME_SIZE)
+	warning (0, "frame size too large for reliable stack checking");
+}
+
   if (pic_offset_table_regno != INVALID_REGNUM)
 pic_offset_table_rtx = gen_rtx_REG (Pmode, pic_offset_table_regno);
 
Index: reload1.c
===
--- reload1.c	(revision 233840)
+++ reload1.c	(working copy)
@@ -1258,28 +1258,6 @@ reload (rtx_insn *first, int global)
 	  }
   }
 
-  /* If we are doing generic stack checking, give a warning if this
- function's frame size is larger than we expect.  */
-  if (flag_stack_check == GENERIC_STACK_CHECK)
-{
-  HOST_WIDE_INT size = get_frame_size () + STACK_CHECK_FIXED_FRAME_SIZE;
-  static int verbose_warned = 0;
-
-  for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
-	if (df_regs_ever_live_p (i) && ! fixed_regs[i] && call_used_regs[i])
-	  size += UNITS_PER_WORD;
-
-  if (size > STACK_CHECK_MAX_FRAME_SIZE)
-	{
-	  warning (0, "frame size too large for reliable stack checking");
-	  if (! verbose_warned)
-	{
-	  warning (0, "try reducing the number of local variables");
-	  verbose_warned = 1;
-	}
-	}
-}
-
   free (temp_pseudo_reg_arr);
 
   /* Indicate that we no longer have known memory locations or constants.  */
/* { dg-do compile } */
/* { dg-options "-fstack-check=generic" } */

/* Check that the expected warning is issued for large frames.  */

#define ONE(s) char a##s[32];
#define TEN(s) ONE(s##0) ONE(s##1) ONE(s##2) ONE(s##3) ONE(s##4) \
   ONE(s##5) ONE(s##6) ONE(s##7) ONE(s##8) ONE(s##9)
#define HUNDRED(s) TEN(s##0) TEN(s##1) TEN(s##2) TEN(s##3) TEN(s##4) \
   TEN(s##5) TEN(s##6) TEN(s##7) TEN(s##8) TEN(s##9)

void foo(void)
{
  HUNDRED(a)
  HUNDRED(b)
} /* { dg-warning "frame size too large for reliable stack checking" } */