Message ID | 201008061941.55733.ebotcazou@adacore.com |
---|---|
State | New |
Headers | show |
> I think that this is mitigated by not adding the word for the new return > address slot; that's what I was trying to say in the comment. Here is a > corrected version, attached. I've also added a testcase for this, as well > as a generic one. > > > * config/i386/i386.c (ix86_expand_prologue): Set stack usage info. > testsuite/ > * lib/gcc-dg.exp (cleanup-stack-usage): New procedure. > * lib/scanasm.exp (scan-stack-usage): Likewise. > (scan-stack-usage-not): Likewise. > * gcc.dg/stack-usage-1.c: New test/ > * gcc.target/i386/stack-usage-realign.c: Likewise. Do you think this is good enough to be applied with the rest of the patch on the mainline?
On 08/30/2010 09:59 AM, Eric Botcazou wrote: >> I think that this is mitigated by not adding the word for the new return >> address slot; that's what I was trying to say in the comment. Here is a >> corrected version, attached. I've also added a testcase for this, as well >> as a generic one. >> >> >> * config/i386/i386.c (ix86_expand_prologue): Set stack usage info. >> testsuite/ >> * lib/gcc-dg.exp (cleanup-stack-usage): New procedure. >> * lib/scanasm.exp (scan-stack-usage): Likewise. >> (scan-stack-usage-not): Likewise. >> * gcc.dg/stack-usage-1.c: New test/ >> * gcc.target/i386/stack-usage-realign.c: Likewise. > > Do you think this is good enough to be applied with the rest of the patch on > the mainline? > Yes, this is fine. r~
On Fri, Aug 06, 2010 at 07:41:55PM +0200, Eric Botcazou wrote: > > Most of this looks fairly sane. > > Thanks. > > > This last line ought to add to the dynamic stack size instead. > > This makes sense indeed. > > > (And you're over-estimating by 1 minimal-stack-alignment-unit. ;-) > > I think that this is mitigated by not adding the word for the new return > address slot; that's what I was trying to say in the comment. Here is a > corrected version, attached. I've also added a testcase for this, as well > as a generic one. Eric, Was this patch tested on x86_64-apple-darwin10 at adacore? Since the patch was committed as r163660, we have been randomly failing three different testcases... FAIL: gcc.c-torture/execute/builtins/sprintf-chk.c compilation, -Os (internal compiler error) UNRESOLVED: gcc.c-torture/execute/builtins/sprintf-chk.c execution, -Os FAIL: gcc.dg/stack-usage-1.c scan-file foo\\t(256|264)\\tstatic FAIL: gcc.target/i386/stack-usage-realign.c scan-file main\\t48\\tdynamic,bounded which Richard Henderson says looks like random memory corruption. Jack > > > * config/i386/i386.c (ix86_expand_prologue): Set stack usage info. > testsuite/ > * lib/gcc-dg.exp (cleanup-stack-usage): New procedure. > * lib/scanasm.exp (scan-stack-usage): Likewise. > (scan-stack-usage-not): Likewise. > * gcc.dg/stack-usage-1.c: New test/ > * gcc.target/i386/stack-usage-realign.c: Likewise. > > > -- > Eric Botcazou > Index: config/i386/i386.c > =================================================================== > --- config/i386/i386.c (revision 162897) > +++ config/i386/i386.c (working copy) > @@ -9588,6 +9588,29 @@ ix86_expand_prologue (void) > } > allocate = frame.stack_pointer_offset - m->fs.sp_offset; > > + if (flag_stack_usage) > + { > + /* We start to count from ARG_POINTER. */ > + HOST_WIDE_INT stack_size = frame.stack_pointer_offset; > + > + /* If it was realigned, take into account the fake frame. */ > + if (stack_realign_drap) > + { > + if (ix86_static_chain_on_stack) > + stack_size += UNITS_PER_WORD; > + > + if (!call_used_regs[REGNO (crtl->drap_reg)]) > + stack_size += UNITS_PER_WORD; > + > + /* This over-estimates by 1 minimal-stack-alignment-unit but > + mitigates that by counting in the new return address slot. */ > + current_function_dynamic_stack_size > + += crtl->stack_alignment_needed / BITS_PER_UNIT; > + } > + > + current_function_static_stack_size = stack_size; > + } > + > /* The stack has already been decremented by the instruction calling us > so we need to probe unconditionally to preserve the protection area. */ > if (flag_stack_check == STATIC_BUILTIN_STACK_CHECK) > Index: testsuite/lib/gcc-dg.exp > =================================================================== > --- testsuite/lib/gcc-dg.exp (revision 162897) > +++ testsuite/lib/gcc-dg.exp (working copy) > @@ -1,5 +1,5 @@ > -# Copyright (C) 1997, 1999, 2000, 2003, 2004, 2005, 2006, 2007, 2008, 2009 > -# Free Software Foundation, Inc. > +# Copyright (C) 1997, 1999, 2000, 2003, 2004, 2005, 2006, 2007, 2008, 2009, > +# 2010 Free Software Foundation, Inc. > > # This program is free software; you can redistribute it and/or modify > # it under the terms of the GNU General Public License as published by > @@ -460,6 +460,11 @@ proc cleanup-ipa-dump { suffix } { > cleanup-dump "\[0-9\]\[0-9\]\[0-9\]i.$suffix" > } > > +# Remove a stack usage file for the current test. > +proc cleanup-stack-usage { args } { > + cleanup-dump "su" > +} > + > # Remove all dump files with the provided suffix. > proc cleanup-dump { suffix } { > # This assumes that we are three frames down from dg-test or some other > Index: testsuite/lib/scanasm.exp > =================================================================== > --- testsuite/lib/scanasm.exp (revision 162897) > +++ testsuite/lib/scanasm.exp (working copy) > @@ -1,4 +1,5 @@ > -# Copyright (C) 2000, 2002, 2003, 2007, 2008 Free Software Foundation, Inc. > +# Copyright (C) 2000, 2002, 2003, 2007, 2008, 2010 > +# Free Software Foundation, Inc. > > # This program is free software; you can redistribute it and/or modify > # it under the terms of the GNU General Public License as published by > @@ -154,6 +155,28 @@ proc scan-file-not { output_file args } > dg-scan "scan-file-not" 0 $testcase $output_file $args > } > > +# Look for a pattern in the .su file produced by the compiler. See > +# dg-scan for details. > + > +proc scan-stack-usage { args } { > + upvar 2 name testcase > + set testcase [lindex $testcase 0] > + set output_file "[file rootname [file tail $testcase]].su" > + > + dg-scan "scan-file" 1 $testcase $output_file $args > +} > + > +# Check that a pattern is not present in the .su file produced by the > +# compiler. See dg-scan for details. > + > +proc scan-stack-usage-not { args } { > + upvar 2 name testcase > + set testcase [lindex $testcase 0] > + set output_file "[file rootname [file tail $testcase]].su" > + > + dg-scan "scan-file-not" 0 $testcase $output_file $args > +} > + > # Call pass if pattern is present given number of times, otherwise fail. > proc scan-assembler-times { args } { > if { [llength $args] < 2 } { > /* { dg-do compile } */ > /* { dg-options "-fstack-usage" } */ > > /* This is aimed at testing basic support for -fstack-usage in the back-ends. > See the SPARC back-end for an example (grep flag_stack_usage in sparc.c). > Once it is implemented, adjust SIZE below so that the stack usage for the > function FOO is reported as 256 or 264 in the stack usage (.su) file. > Then check that this is the actual stack usage in the assembly file. */ > > #if defined(__i386__) > # define SIZE 248 > #elif defined(__x86_64__) > # define SIZE 356 > #elif defined (__sparc__) > # if defined (__arch64__) > # define SIZE 76 > # else > # define SIZE 160 > # endif > #elif defined(__hppa__) > # define SIZE 192 > #elif defined (__alpha__) > # define SIZE 240 > #elif defined (__ia64__) > # define SIZE 272 > #elif defined(__mips__) > # define SIZE 240 > #elif defined (__powerpc__) || defined (__PPC__) || defined (__ppc__) \ > || defined (__POWERPC__) || defined (PPC) || defined (_IBMR2) > # define SIZE 240 > #else > # define SIZE 256 > #endif > > int foo (void) > { > char arr[SIZE]; > arr[0] = 1; > return 0; > } > > /* { dg-final { scan-stack-usage "foo\t\(256|264\)\tstatic" } } */ > /* { dg-final { cleanup-stack-usage } } */ > /* { dg-do compile } */ > /* { dg-require-effective-target ilp32 } */ > /* { dg-options "-fstack-usage -msse2 -mforce-drap" } */ > > typedef int __attribute__((vector_size(16))) vec; > > vec foo (vec v) > { > return v; > } > > int main (void) > { > vec V; > V = foo (V); > return 0; > } > > /* { dg-final { scan-stack-usage "main\t48\tdynamic,bounded" } } */ > /* { dg-final { cleanup-stack-usage } } */
> Since the patch was committed as r163660, we have been randomly failing > three different testcases... > > FAIL: gcc.c-torture/execute/builtins/sprintf-chk.c compilation, -Os > (internal compiler error) > UNRESOLVED: gcc.c-torture/execute/builtins/sprintf-chk.c execution, -Os > FAIL: gcc.dg/stack-usage-1.c scan-file foo\\t(256|264)\\tstatic > FAIL: gcc.target/i386/stack-usage-realign.c scan-file > main\\t48\\tdynamic,bounded Do the last 2 really pass sometimes? They should _always_ fail on 32-bit Darwin as far as I can see...
On Wed, Sep 01, 2010 at 11:22:26PM +0200, Eric Botcazou wrote: > > Since the patch was committed as r163660, we have been randomly failing > > three different testcases... > > > > FAIL: gcc.c-torture/execute/builtins/sprintf-chk.c compilation, -Os > > (internal compiler error) > > UNRESOLVED: gcc.c-torture/execute/builtins/sprintf-chk.c execution, -Os > > FAIL: gcc.dg/stack-usage-1.c scan-file foo\\t(256|264)\\tstatic > > FAIL: gcc.target/i386/stack-usage-realign.c scan-file > > main\\t48\\tdynamic,bounded > > Do the last 2 really pass sometimes? They should _always_ fail on 32-bit > Darwin as far as I can see... Sorry, my mistake. The gcc.dg/stack-usage-1.c scan-file foo\\t(256|264)\\tstatic and gcc.target/i386/stack-usage-realign.c scan-file main\\t48\\tdynamic,bounded tests always compile and then fail on execution at -m32. Their code generation is always consistent. It is the gcc.c-torture/execute/builtins/sprintf-chk.c compilation which is exhibiting three behaviors... 1) consistent code 2) random differences in code 3) compiler ICE My main concern is if this bug will spill over into production code generation. Jack > > -- > Eric Botcazou
Index: config/i386/i386.c =================================================================== --- config/i386/i386.c (revision 162897) +++ config/i386/i386.c (working copy) @@ -9588,6 +9588,29 @@ ix86_expand_prologue (void) } allocate = frame.stack_pointer_offset - m->fs.sp_offset; + if (flag_stack_usage) + { + /* We start to count from ARG_POINTER. */ + HOST_WIDE_INT stack_size = frame.stack_pointer_offset; + + /* If it was realigned, take into account the fake frame. */ + if (stack_realign_drap) + { + if (ix86_static_chain_on_stack) + stack_size += UNITS_PER_WORD; + + if (!call_used_regs[REGNO (crtl->drap_reg)]) + stack_size += UNITS_PER_WORD; + + /* This over-estimates by 1 minimal-stack-alignment-unit but + mitigates that by counting in the new return address slot. */ + current_function_dynamic_stack_size + += crtl->stack_alignment_needed / BITS_PER_UNIT; + } + + current_function_static_stack_size = stack_size; + } + /* The stack has already been decremented by the instruction calling us so we need to probe unconditionally to preserve the protection area. */ if (flag_stack_check == STATIC_BUILTIN_STACK_CHECK) Index: testsuite/lib/gcc-dg.exp =================================================================== --- testsuite/lib/gcc-dg.exp (revision 162897) +++ testsuite/lib/gcc-dg.exp (working copy) @@ -1,5 +1,5 @@ -# Copyright (C) 1997, 1999, 2000, 2003, 2004, 2005, 2006, 2007, 2008, 2009 -# Free Software Foundation, Inc. +# Copyright (C) 1997, 1999, 2000, 2003, 2004, 2005, 2006, 2007, 2008, 2009, +# 2010 Free Software Foundation, Inc. # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -460,6 +460,11 @@ proc cleanup-ipa-dump { suffix } { cleanup-dump "\[0-9\]\[0-9\]\[0-9\]i.$suffix" } +# Remove a stack usage file for the current test. +proc cleanup-stack-usage { args } { + cleanup-dump "su" +} + # Remove all dump files with the provided suffix. proc cleanup-dump { suffix } { # This assumes that we are three frames down from dg-test or some other Index: testsuite/lib/scanasm.exp =================================================================== --- testsuite/lib/scanasm.exp (revision 162897) +++ testsuite/lib/scanasm.exp (working copy) @@ -1,4 +1,5 @@ -# Copyright (C) 2000, 2002, 2003, 2007, 2008 Free Software Foundation, Inc. +# Copyright (C) 2000, 2002, 2003, 2007, 2008, 2010 +# Free Software Foundation, Inc. # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -154,6 +155,28 @@ proc scan-file-not { output_file args } dg-scan "scan-file-not" 0 $testcase $output_file $args } +# Look for a pattern in the .su file produced by the compiler. See +# dg-scan for details. + +proc scan-stack-usage { args } { + upvar 2 name testcase + set testcase [lindex $testcase 0] + set output_file "[file rootname [file tail $testcase]].su" + + dg-scan "scan-file" 1 $testcase $output_file $args +} + +# Check that a pattern is not present in the .su file produced by the +# compiler. See dg-scan for details. + +proc scan-stack-usage-not { args } { + upvar 2 name testcase + set testcase [lindex $testcase 0] + set output_file "[file rootname [file tail $testcase]].su" + + dg-scan "scan-file-not" 0 $testcase $output_file $args +} + # Call pass if pattern is present given number of times, otherwise fail. proc scan-assembler-times { args } { if { [llength $args] < 2 } {