| Message ID | 20180117200749.GX2063@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | Add clobbers for callee copied argument temporaries (PR sanitizer/81715, PR testsuite/83882) | expand |
On 2018-01-17 3:07 PM, Jakub Jelinek wrote: > John, do you think you could test this on hppa without the callee copies > default change? > > Or should we not care anymore if there aren't any similar targets left? I'll test, probably starting build this evening. I only changed the linux target. The hpux and bsd targets are still callee copies. Dave
On Wed, 17 Jan 2018, Jakub Jelinek wrote: > Hi! > > PR83882 complains that PR81715 testcase fails on callee copies parameter > targets. The following patch ought to fix that, but I have only > bootstrapped/regtested it on x86_64-linux and i686-linux + on the testcase > with hppa. Looks reasonable. > John, do you think you could test this on hppa without the callee copies > default change? > > Or should we not care anymore if there aren't any similar targets left? How's that communicated to the middle-end anyways? Richard. > 2018-01-17 Jakub Jelinek <jakub@redhat.com> > > PR sanitizer/81715 > PR testsuite/83882 > * function.h (gimplify_parameters): Add gimple_seq * argument. > * function.c: Include gimple.h and options.h. > (gimplify_parameters): Add cleanup argument, add CLOBBER stmts > for the added local temporaries if needed. > * gimplify.c (gimplify_body): Adjust gimplify_parameters caller, > if there are any parameter cleanups, wrap whole body into a > try/finally with the cleanups. > > --- gcc/function.h.jj 2018-01-03 10:19:53.858533740 +0100 > +++ gcc/function.h 2018-01-16 14:31:21.409972177 +0100 > @@ -607,7 +607,7 @@ extern bool initial_value_entry (int i, > extern void instantiate_decl_rtl (rtx x); > extern int aggregate_value_p (const_tree, const_tree); > extern bool use_register_for_decl (const_tree); > -extern gimple_seq gimplify_parameters (void); > +extern gimple_seq gimplify_parameters (gimple_seq *); > extern void locate_and_pad_parm (machine_mode, tree, int, int, int, > tree, struct args_size *, > struct locate_and_pad_arg_data *); > --- gcc/function.c.jj 2018-01-12 11:35:48.901222595 +0100 > +++ gcc/function.c 2018-01-16 15:13:22.165665047 +0100 > @@ -79,6 +79,8 @@ along with GCC; see the file COPYING3. > #include "tree-ssa.h" > #include "stringpool.h" > #include "attribs.h" > +#include "gimple.h" > +#include "options.h" > > /* So we can assign to cfun in this file. */ > #undef cfun > @@ -3993,7 +3995,7 @@ gimplify_parm_type (tree *tp, int *walk_ > statements to add to the beginning of the function. */ > > gimple_seq > -gimplify_parameters (void) > +gimplify_parameters (gimple_seq *cleanup) > { > struct assign_parm_data_all all; > tree parm; > @@ -4058,6 +4060,16 @@ gimplify_parameters (void) > else if (TREE_CODE (type) == COMPLEX_TYPE > || TREE_CODE (type) == VECTOR_TYPE) > DECL_GIMPLE_REG_P (local) = 1; > + > + if (!is_gimple_reg (local) > + && flag_stack_reuse != SR_NONE) > + { > + tree clobber = build_constructor (type, NULL); > + gimple *clobber_stmt; > + TREE_THIS_VOLATILE (clobber) = 1; > + clobber_stmt = gimple_build_assign (local, clobber); > + gimple_seq_add_stmt (cleanup, clobber_stmt); > + } > } > else > { > --- gcc/gimplify.c.jj 2018-01-16 12:21:15.895859416 +0100 > +++ gcc/gimplify.c 2018-01-16 14:41:27.643872081 +0100 > @@ -12589,7 +12589,7 @@ gbind * > gimplify_body (tree fndecl, bool do_parms) > { > location_t saved_location = input_location; > - gimple_seq parm_stmts, seq; > + gimple_seq parm_stmts, parm_cleanup = NULL, seq; > gimple *outer_stmt; > gbind *outer_bind; > struct cgraph_node *cgn; > @@ -12628,7 +12628,7 @@ gimplify_body (tree fndecl, bool do_parm > > /* Resolve callee-copies. This has to be done before processing > the body so that DECL_VALUE_EXPR gets processed correctly. */ > - parm_stmts = do_parms ? gimplify_parameters () : NULL; > + parm_stmts = do_parms ? gimplify_parameters (&parm_cleanup) : NULL; > > /* Gimplify the function's body. */ > seq = NULL; > @@ -12657,6 +12657,13 @@ gimplify_body (tree fndecl, bool do_parm > tree parm; > > gimplify_seq_add_seq (&parm_stmts, gimple_bind_body (outer_bind)); > + if (parm_cleanup) > + { > + gtry *g = gimple_build_try (parm_stmts, parm_cleanup, > + GIMPLE_TRY_FINALLY); > + parm_stmts = NULL; > + gimple_seq_add_stmt (&parm_stmts, g); > + } > gimple_bind_set_body (outer_bind, parm_stmts); > > for (parm = DECL_ARGUMENTS (current_function_decl); > > Jakub > >
On Thu, Jan 18, 2018 at 09:10:03AM +0100, Richard Biener wrote: > On Wed, 17 Jan 2018, Jakub Jelinek wrote: > > > Hi! > > > > PR83882 complains that PR81715 testcase fails on callee copies parameter > > targets. The following patch ought to fix that, but I have only > > bootstrapped/regtested it on x86_64-linux and i686-linux + on the testcase > > with hppa. > > Looks reasonable. > > > John, do you think you could test this on hppa without the callee copies > > default change? > > > > Or should we not care anymore if there aren't any similar targets left? > > How's that communicated to the middle-end anyways? You mean the callee copies stuff? gimplify_parameters creates new temporaries, assigns the parameter to them, set DECL_VALUE_EXPR on the parameters to the temporary and at the end of function clears DECL_VALUE_EXPR and with this patch adds a CLOBBER in a cleanup as well. Jakub
On Thu, 18 Jan 2018, Jakub Jelinek wrote: > On Thu, Jan 18, 2018 at 09:10:03AM +0100, Richard Biener wrote: > > On Wed, 17 Jan 2018, Jakub Jelinek wrote: > > > > > Hi! > > > > > > PR83882 complains that PR81715 testcase fails on callee copies parameter > > > targets. The following patch ought to fix that, but I have only > > > bootstrapped/regtested it on x86_64-linux and i686-linux + on the testcase > > > with hppa. > > > > Looks reasonable. > > > > > John, do you think you could test this on hppa without the callee copies > > > default change? > > > > > > Or should we not care anymore if there aren't any similar targets left? > > > > How's that communicated to the middle-end anyways? > > You mean the callee copies stuff? gimplify_parameters creates new > temporaries, assigns the parameter to them, set DECL_VALUE_EXPR on the > parameters to the temporary and at the end of function clears > DECL_VALUE_EXPR and with this patch adds a CLOBBER in a cleanup as well. No, I meant whether the target wants this or not. Richard.
On Thu, Jan 18, 2018 at 09:18:21AM +0100, Richard Biener wrote: > On Thu, 18 Jan 2018, Jakub Jelinek wrote: > > > On Thu, Jan 18, 2018 at 09:10:03AM +0100, Richard Biener wrote: > > > On Wed, 17 Jan 2018, Jakub Jelinek wrote: > > > > > > > Hi! > > > > > > > > PR83882 complains that PR81715 testcase fails on callee copies parameter > > > > targets. The following patch ought to fix that, but I have only > > > > bootstrapped/regtested it on x86_64-linux and i686-linux + on the testcase > > > > with hppa. > > > > > > Looks reasonable. > > > > > > > John, do you think you could test this on hppa without the callee copies > > > > default change? > > > > > > > > Or should we not care anymore if there aren't any similar targets left? > > > > > > How's that communicated to the middle-end anyways? > > > > You mean the callee copies stuff? gimplify_parameters creates new > > temporaries, assigns the parameter to them, set DECL_VALUE_EXPR on the > > parameters to the temporary and at the end of function clears > > DECL_VALUE_EXPR and with this patch adds a CLOBBER in a cleanup as well. > > No, I meant whether the target wants this or not. Ah, that. Through targetm.calls.callee_copies hook. Jakub
On Thu, 18 Jan 2018, Jakub Jelinek wrote: > On Thu, Jan 18, 2018 at 09:18:21AM +0100, Richard Biener wrote: > > On Thu, 18 Jan 2018, Jakub Jelinek wrote: > > > > > On Thu, Jan 18, 2018 at 09:10:03AM +0100, Richard Biener wrote: > > > > On Wed, 17 Jan 2018, Jakub Jelinek wrote: > > > > > > > > > Hi! > > > > > > > > > > PR83882 complains that PR81715 testcase fails on callee copies parameter > > > > > targets. The following patch ought to fix that, but I have only > > > > > bootstrapped/regtested it on x86_64-linux and i686-linux + on the testcase > > > > > with hppa. > > > > > > > > Looks reasonable. > > > > > > > > > John, do you think you could test this on hppa without the callee copies > > > > > default change? > > > > > > > > > > Or should we not care anymore if there aren't any similar targets left? > > > > > > > > How's that communicated to the middle-end anyways? > > > > > > You mean the callee copies stuff? gimplify_parameters creates new > > > temporaries, assigns the parameter to them, set DECL_VALUE_EXPR on the > > > parameters to the temporary and at the end of function clears > > > DECL_VALUE_EXPR and with this patch adds a CLOBBER in a cleanup as well. > > > > No, I meant whether the target wants this or not. > > Ah, that. Through targetm.calls.callee_copies hook. So there are quite a few more callee copies targets then, notably unconditionally epiphany, mmix, mn10300 and v850 and conditionally a few more, for example mips. Richard.
On 2018-01-17 3:07 PM, Jakub Jelinek wrote: > PR83882 complains that PR81715 testcase fails on callee copies parameter > targets. The following patch ought to fix that, but I have only > bootstrapped/regtested it on x86_64-linux and i686-linux + on the testcase > with hppa. > > John, do you think you could test this on hppa without the callee copies > default change? On hppa2.0w-hp-hpux11.11, pr81715.C now passes. However, there are some new fails in the gcc and g++ suites: FAIL: g++.dg/torture/stackalign/throw-1.C -O0 execution test FAIL: g++.dg/torture/stackalign/throw-1.C -O0 -fpic execution test FAIL: g++.dg/torture/stackalign/throw-1.C -O1 -fpic execution test FAIL: g++.dg/torture/stackalign/throw-1.C -O2 -fpic execution test FAIL: g++.dg/torture/stackalign/throw-1.C -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions -fpic execution test FAIL: g++.dg/torture/stackalign/throw-1.C -O3 -g -fpic execution test FAIL: g++.dg/torture/stackalign/throw-1.C -Os -fpic execution test FAIL: g++.dg/torture/stackalign/throw-2.C -O0 execution test FAIL: g++.dg/torture/stackalign/throw-2.C -O0 -fpic execution test FAIL: g++.dg/torture/stackalign/throw-2.C -O1 -fpic execution test FAIL: g++.dg/torture/stackalign/throw-2.C -O2 -fpic execution test FAIL: g++.dg/torture/stackalign/throw-2.C -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions -fpic execution test FAIL: g++.dg/torture/stackalign/throw-2.C -O3 -g -fpic execution test FAIL: g++.dg/torture/stackalign/throw-2.C -Os -fpic execution test FAIL: g++.dg/torture/stackalign/throw-4.C -O0 execution test FAIL: g++.dg/torture/stackalign/throw-4.C -O0 -fpic execution test FAIL: g++.dg/torture/stackalign/throw-4.C -O1 -fpic execution test FAIL: g++.dg/torture/stackalign/throw-4.C -O2 -fpic execution test FAIL: g++.dg/torture/stackalign/throw-4.C -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions -fpic execution test FAIL: g++.dg/torture/stackalign/throw-4.C -O3 -g -fpic execution test FAIL: g++.dg/torture/stackalign/throw-4.C -Os -fpic execution test FAIL: gcc.dg/torture/pr52451.c -O0 execution test FAIL: gcc.dg/torture/pr52451.c -O1 execution test FAIL: gcc.dg/torture/pr52451.c -O2 execution test FAIL: gcc.dg/torture/pr52451.c -O3 -g execution test FAIL: gcc.dg/torture/pr52451.c -Os execution test FAIL: gcc.dg/compat/pr83487-1 c_compat_y_tst.o compile FAIL: gcc.dg/compat/pr83487-2 c_compat_y_tst.o compile Dave
On Thu, Jan 18, 2018 at 08:10:50AM -0500, John David Anglin wrote: > On 2018-01-17 3:07 PM, Jakub Jelinek wrote: > > PR83882 complains that PR81715 testcase fails on callee copies parameter > > targets. The following patch ought to fix that, but I have only > > bootstrapped/regtested it on x86_64-linux and i686-linux + on the testcase > > with hppa. > > > > John, do you think you could test this on hppa without the callee copies > > default change? > On hppa2.0w-hp-hpux11.11, pr81715.C now passes. > > However, there are some new fails in the gcc and g++ suites: That is really weird, at least in the cross-compiler on throw-1.C I don't see with -mno-caller-copies the new code triggering at all. Tried both hppa-linux and hppa-hpux crosses. Furthermore, throw-1.C has: /* { dg-skip-if "Stack alignment is too small" { hppa*-*-hpux* } } */ so I wonder how it comes you see them run on hpux at all. I don't have hpux or hppa-linux fenv.h, so can't check pr52451.c. What kind of error would be on pr83487-*? If any of those are really caused by my patch, I'd be interest to see e.g. *.gimple and *.optimized dumps with and without the patch (rather than reverting the whole patch, it should be enough to hand edit in function.c the && flag_stack_reuse != SR_NONE part and change it to && 0 && flag_stack_reuse != SR_NONE or similar. > FAIL: g++.dg/torture/stackalign/throw-1.C -O0 execution test > FAIL: g++.dg/torture/stackalign/throw-1.C -O0 -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-1.C -O1 -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-1.C -O2 -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-1.C -O3 -fomit-frame-pointer > -funroll-loops -fpeel-loops -ftracer -finline-functions -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-1.C -O3 -g -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-1.C -Os -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-2.C -O0 execution test > FAIL: g++.dg/torture/stackalign/throw-2.C -O0 -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-2.C -O1 -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-2.C -O2 -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-2.C -O3 -fomit-frame-pointer > -funroll-loops -fpeel-loops -ftracer -finline-functions -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-2.C -O3 -g -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-2.C -Os -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-4.C -O0 execution test > FAIL: g++.dg/torture/stackalign/throw-4.C -O0 -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-4.C -O1 -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-4.C -O2 -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-4.C -O3 -fomit-frame-pointer > -funroll-loops -fpeel-loops -ftracer -finline-functions -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-4.C -O3 -g -fpic execution test > FAIL: g++.dg/torture/stackalign/throw-4.C -Os -fpic execution test > > FAIL: gcc.dg/torture/pr52451.c -O0 execution test > FAIL: gcc.dg/torture/pr52451.c -O1 execution test > FAIL: gcc.dg/torture/pr52451.c -O2 execution test > FAIL: gcc.dg/torture/pr52451.c -O3 -g execution test > FAIL: gcc.dg/torture/pr52451.c -Os execution test > > FAIL: gcc.dg/compat/pr83487-1 c_compat_y_tst.o compile > FAIL: gcc.dg/compat/pr83487-2 c_compat_y_tst.o compile > > Dave > > -- > John David Anglin dave.anglin@bell.net Jakub
On 2018-01-18 12:51 PM, Jakub Jelinek wrote: > On Thu, Jan 18, 2018 at 08:10:50AM -0500, John David Anglin wrote: >> On 2018-01-17 3:07 PM, Jakub Jelinek wrote: >>> PR83882 complains that PR81715 testcase fails on callee copies parameter >>> targets. The following patch ought to fix that, but I have only >>> bootstrapped/regtested it on x86_64-linux and i686-linux + on the testcase >>> with hppa. >>> >>> John, do you think you could test this on hppa without the callee copies >>> default change? >> On hppa2.0w-hp-hpux11.11, pr81715.C now passes. >> >> However, there are some new fails in the gcc and g++ suites: > That is really weird, at least in the cross-compiler on throw-1.C I don't > see with -mno-caller-copies the new code triggering at all. > Tried both hppa-linux and hppa-hpux crosses. > Furthermore, throw-1.C has: > /* { dg-skip-if "Stack alignment is too small" { hppa*-*-hpux* } } */ Sorry, these fails are my fault. I tweaked the skip to just hppa*64*. Nominally, 32-bit hpux has 64 byte stack alignment but maybe alignment in signal frames is insufficient. > so I wonder how it comes you see them run on hpux at all. > I don't have hpux or hppa-linux fenv.h, so can't check pr52451.c. Will have to investigate. I see they predate your patch. > What kind of error would be on pr83487-*? Digging into log, I see: spawn /test/gnu/gcc/objdir/gcc/xgcc -B/test/gnu/gcc/objdir/gcc/ -fno-diagnostics-show-caret -fdiagnostics-color=never -DSKIP_DECIMAL_FLOAT -c -o c_compat_y_tst.o /test/gnu/gcc/gcc/gcc/testsuite/gcc.dg/compat//pr83487-1_y.c /test/gnu/gcc/gcc/gcc/testsuite/gcc.dg/compat//pr83487-1_y.c:27:1: warning: alignment (16) for a exceeds maximum alignment for global common data. Using 8 output is: /test/gnu/gcc/gcc/gcc/testsuite/gcc.dg/compat//pr83487-1_y.c:27:1: warning: alignment (16) for a exceeds maximum alignment for global common data. Using 8 FAIL: gcc.dg/compat/pr83487-1 c_compat_y_tst.o compile The typical fix is to add "-fno-common" but I'm not sure where it would go in this case. So, I don't see any regressions from you change. Thanks, Dave
On 2018-01-18 2:44 PM, John David Anglin wrote: >> I don't have hpux or hppa-linux fenv.h, so can't check pr52451.c. > Will have to investigate. I see they predate your patch. This is a test issue. The long double support on hpux is a software emulation and it's not supported the fenv.h routines. Is this change okay? Dave
On Thu, Jan 18, 2018 at 03:31:38PM -0500, John David Anglin wrote: > On 2018-01-18 2:44 PM, John David Anglin wrote: > > > I don't have hpux or hppa-linux fenv.h, so can't check pr52451.c. > > Will have to investigate. I see they predate your patch. > This is a test issue. The long double support on hpux is a software > emulation > and it's not supported the fenv.h routines. I think this explanation belongs into the testcase as a comment to explain why you aren't testing it on hppa-hpux. Ok with that change. > 2018-01-18 John David Anglin <danglin@gcc.gnu.org> > > * gcc.dg/torture/pr52451.c (main): Skip long double test on > hppa*-*-hpux*. > > Index: gcc.dg/torture/pr52451.c > =================================================================== > --- gcc.dg/torture/pr52451.c (revision 256744) > +++ gcc.dg/torture/pr52451.c (working copy) > @@ -49,7 +49,9 @@ > > TEST (float, f); > TEST (double, ); > +#if !defined(__hppa__) || !defined(__hpux__) > TEST (long double, l); > +#endif > > return 0; > } Jakub
--- gcc/function.h.jj 2018-01-03 10:19:53.858533740 +0100 +++ gcc/function.h 2018-01-16 14:31:21.409972177 +0100 @@ -607,7 +607,7 @@ extern bool initial_value_entry (int i, extern void instantiate_decl_rtl (rtx x); extern int aggregate_value_p (const_tree, const_tree); extern bool use_register_for_decl (const_tree); -extern gimple_seq gimplify_parameters (void); +extern gimple_seq gimplify_parameters (gimple_seq *); extern void locate_and_pad_parm (machine_mode, tree, int, int, int, tree, struct args_size *, struct locate_and_pad_arg_data *); --- gcc/function.c.jj 2018-01-12 11:35:48.901222595 +0100 +++ gcc/function.c 2018-01-16 15:13:22.165665047 +0100 @@ -79,6 +79,8 @@ along with GCC; see the file COPYING3. #include "tree-ssa.h" #include "stringpool.h" #include "attribs.h" +#include "gimple.h" +#include "options.h" /* So we can assign to cfun in this file. */ #undef cfun @@ -3993,7 +3995,7 @@ gimplify_parm_type (tree *tp, int *walk_ statements to add to the beginning of the function. */ gimple_seq -gimplify_parameters (void) +gimplify_parameters (gimple_seq *cleanup) { struct assign_parm_data_all all; tree parm; @@ -4058,6 +4060,16 @@ gimplify_parameters (void) else if (TREE_CODE (type) == COMPLEX_TYPE || TREE_CODE (type) == VECTOR_TYPE) DECL_GIMPLE_REG_P (local) = 1; + + if (!is_gimple_reg (local) + && flag_stack_reuse != SR_NONE) + { + tree clobber = build_constructor (type, NULL); + gimple *clobber_stmt; + TREE_THIS_VOLATILE (clobber) = 1; + clobber_stmt = gimple_build_assign (local, clobber); + gimple_seq_add_stmt (cleanup, clobber_stmt); + } } else { --- gcc/gimplify.c.jj 2018-01-16 12:21:15.895859416 +0100 +++ gcc/gimplify.c 2018-01-16 14:41:27.643872081 +0100 @@ -12589,7 +12589,7 @@ gbind * gimplify_body (tree fndecl, bool do_parms) { location_t saved_location = input_location; - gimple_seq parm_stmts, seq; + gimple_seq parm_stmts, parm_cleanup = NULL, seq; gimple *outer_stmt; gbind *outer_bind; struct cgraph_node *cgn; @@ -12628,7 +12628,7 @@ gimplify_body (tree fndecl, bool do_parm /* Resolve callee-copies. This has to be done before processing the body so that DECL_VALUE_EXPR gets processed correctly. */ - parm_stmts = do_parms ? gimplify_parameters () : NULL; + parm_stmts = do_parms ? gimplify_parameters (&parm_cleanup) : NULL; /* Gimplify the function's body. */ seq = NULL; @@ -12657,6 +12657,13 @@ gimplify_body (tree fndecl, bool do_parm tree parm; gimplify_seq_add_seq (&parm_stmts, gimple_bind_body (outer_bind)); + if (parm_cleanup) + { + gtry *g = gimple_build_try (parm_stmts, parm_cleanup, + GIMPLE_TRY_FINALLY); + parm_stmts = NULL; + gimple_seq_add_stmt (&parm_stmts, g); + } gimple_bind_set_body (outer_bind, parm_stmts); for (parm = DECL_ARGUMENTS (current_function_decl);