| Message ID | 20120123182918.GL18768@tyan-ft48-01.lab.bos.redhat.com |
|---|---|
| State | New |
| Headers | show |
OK. Jason
On Mon, Jan 23, 2012 at 1:29 PM, Jakub Jelinek <jakub@redhat.com> wrote: > profiledbootstrap currently fails on powerpc64-linux (64-bit). The problem > is that the linker errors on .toc section references from within > non-allocated sections. Alan changed the linker some months ago to > not die with assertion failure, but error out on these, and the attached > testcase reduced from cp/parser.c during bootstrap shows that we still > can hit it in some cases. The problem here is that var-tracking.c > (adjust_insn) isn't able to delegitimize it, because the UNSPEC_TOCREL > isn't added there to r2 register, but to a debug_expr (which only afterwards > is found to contain r2 value). So the UNSPEC_TOCREL makes it through to > mem_loc_descriptor, where it is already properly delegitimized into a > (mem (symbol_ref (".LC0")) where .LC0 is a constant pool (.toc) symbol. > We in some places try to prefer avoid_constant_pool_reference values over > the mems, in this case avoid_constant_pool returns (symbol_ref "w"), which > is a non-local symbol which isn't const_ok_for_output though. So > mem_loc_descriptor on the avoid_constant_pool_reference value fails and we > try the original (mem (symbol_ref (".LC0")) - on other targets this is still > useful, if we know the right value is in say some .got entry etc. and we > can reference that location from within the .debug_info, we win. > Unfortunately the ppc64 linker doesn't like this, so we need to reject > those. The following patch adds a target hook and rejects there from > emitting into .debug_info/.debug_loc any SYMBOL_REFs that are in the > .toc section. > > Bootstrapped/regtested on x86_64-linux and i686-linux, profiledbootstrapped > on powerpc64-linux (both --with-cpu=default32 and defaulting to 64-bit, > the last one failed without this patch), powerpc64-linux regtest is in > progress (but the testcase in the patch has been tested already to > fail without the patch and succeed with the patch). > Ok for trunk? > > 2012-01-23 Jakub Jelinek <jakub@redhat.com> > > PR target/51957 > * target.def (const_not_ok_for_debug_p): New hook. > * doc/tm.texi.in (TARGET_CONST_NOT_OK_FOR_DEBUG_P): New hook > documentation. > * doc/tm.texi: Regenerated. > * dwarf2out.c (const_ok_for_output_1): If > targetm.const_not_ok_for_debug_p returns true, fail. > * config/rs6000/rs6000.c (rs6000_const_not_ok_for_debug_p): New > function. > (TARGET_CONST_NOT_OK_FOR_DEBUG_P): Redefine. > > * gcc.dg/pr51957-1.c: New test. > * gcc.dg/pr51957-1.h: New file. > * gcc.dg/pr51957-2.c: New test. Okay. Thanks, David
On Mon, Jan 23, 2012 at 07:29:18PM +0100, Jakub Jelinek wrote: > can hit it in some cases. The problem here is that var-tracking.c > (adjust_insn) isn't able to delegitimize it, because the UNSPEC_TOCREL > isn't added there to r2 register, but to a debug_expr (which only afterwards > is found to contain r2 value). Why don't we relax operand checks in rs6000_delegitimize_address? http://gcc.gnu.org/ml/gcc-patches/2011-09/msg00304.html did that as part of changing toc references. For mainline you'd just get rid of - && ((GET_CODE (XEXP (x, 0)) == REG - && (REGNO (XEXP (x, 0)) == TOC_REGISTER - || TARGET_MINIMAL_TOC - || TARGET_CMODEL != CMODEL_SMALL)) - || (TARGET_CMODEL != CMODEL_SMALL - && GET_CODE (XEXP (x, 0)) == CONST - && GET_CODE (XEXP (XEXP (x, 0), 0)) == PLUS - && GET_CODE (XEXP (XEXP (XEXP (x, 0), 0), 0)) == REG - && REGNO (XEXP (XEXP (XEXP (x, 0), 0), 0)) == TOC_REGISTER - && GET_CODE (XEXP (XEXP (XEXP (x, 0), 0), 1)) == HIGH - && rtx_equal_p (XEXP (x, 1), - XEXP (XEXP (XEXP (XEXP (x, 0), 0), 1), 0)))))
On Tue, Jan 24, 2012 at 01:45:40PM +1030, Alan Modra wrote: > On Mon, Jan 23, 2012 at 07:29:18PM +0100, Jakub Jelinek wrote: > > can hit it in some cases. The problem here is that var-tracking.c > > (adjust_insn) isn't able to delegitimize it, because the UNSPEC_TOCREL > > isn't added there to r2 register, but to a debug_expr (which only afterwards > > is found to contain r2 value). > > Why don't we relax operand checks in rs6000_delegitimize_address? > http://gcc.gnu.org/ml/gcc-patches/2011-09/msg00304.html did that as > part of changing toc references. For mainline you'd just get rid of > > - && ((GET_CODE (XEXP (x, 0)) == REG > - && (REGNO (XEXP (x, 0)) == TOC_REGISTER > - || TARGET_MINIMAL_TOC > - || TARGET_CMODEL != CMODEL_SMALL)) > - || (TARGET_CMODEL != CMODEL_SMALL > - && GET_CODE (XEXP (x, 0)) == CONST > - && GET_CODE (XEXP (XEXP (x, 0), 0)) == PLUS > - && GET_CODE (XEXP (XEXP (XEXP (x, 0), 0), 0)) == REG > - && REGNO (XEXP (XEXP (XEXP (x, 0), 0), 0)) == TOC_REGISTER > - && GET_CODE (XEXP (XEXP (XEXP (x, 0), 0), 1)) == HIGH > - && rtx_equal_p (XEXP (x, 1), > - XEXP (XEXP (XEXP (XEXP (x, 0), 0), 1), 0))))) If you relax the checking too much, don't you risk that something other than the TOC register value is being added to the UNSPEC_TOCREL? Say, if you have an 8byte object in .toc, and some loop body uses ((char *) &object)[1 + iv]; to access it, can't you end up with (r2 + 1), or (r2 + 1 + iv) being added to UNSPEC_TOCREL? Certainly on i686 something like that happens and that is why the delegitimization needs to be strict about the pic register e.g. for UNSPEC_GOTOFF or UNSPEC_GOT. I've checked in my patch already, I think it is desirable even if delegitimization is somehow changed, it really models the linker requirements that debug sections don't reference .toc section symbols and this problem could pop up any time again with some other code. And allows other targets to express their requirements too. Jakub
On Tue, Jan 24, 2012 at 08:36:11AM +0100, Jakub Jelinek wrote: > If you relax the checking too much, don't you risk that something other than > the TOC register value is being added to the UNSPEC_TOCREL? Say, if you > have an 8byte object in .toc, and some loop body uses > ((char *) &object)[1 + iv]; > to access it, can't you end up with (r2 + 1), or (r2 + 1 + iv) being added > to UNSPEC_TOCREL? Certainly on i686 something like that happens and that > is why the delegitimization needs to be strict about the pic register I don't think so. We only put addresses, int constants smaller than Pmode, and FP constants in .toc. Hmm, however with -mcmodel=medium we do have UNSPEC_TOCREL refering to other local objects so you may be correct. > I've checked in my patch already OK. I was worried that we might be losing debug info, but if you take a look you'll see that location list is quite bogus anyway. At least it looks that way to me..
--- gcc/target.def.jj 2012-01-20 12:35:16.000000000 +0100 +++ gcc/target.def 2012-01-23 13:14:52.601638343 +0100 @@ -1382,6 +1382,14 @@ DEFHOOK rtx, (rtx x), delegitimize_mem_from_attrs) +/* Given an RTX, return true if it is not ok to emit it into debug info + section. */ +DEFHOOK +(const_not_ok_for_debug_p, + "", + bool, (rtx x), + hook_bool_rtx_false) + /* Given an address RTX, say whether it is valid. */ DEFHOOK (legitimate_address_p, --- gcc/doc/tm.texi.in.jj 2012-01-20 12:35:10.000000000 +0100 +++ gcc/doc/tm.texi.in 2012-01-23 13:27:19.544241676 +0100 @@ -5567,6 +5567,11 @@ the semantics of these opaque @code{UNSP into their original form. @end deftypefn +@hook TARGET_CONST_NOT_OK_FOR_DEBUG_P +This hook should return true if @var{x} should not be emitted into +debug sections. +@end deftypefn + @hook TARGET_CANNOT_FORCE_CONST_MEM This hook should return true if @var{x} is of a form that cannot (or should not) be spilled to the constant pool. @var{mode} is the mode --- gcc/doc/tm.texi.jj 2012-01-20 12:35:10.000000000 +0100 +++ gcc/doc/tm.texi 2012-01-23 13:27:33.000000000 +0100 @@ -5631,6 +5631,11 @@ the semantics of these opaque @code{UNSP into their original form. @end deftypefn +@deftypefn {Target Hook} bool TARGET_CONST_NOT_OK_FOR_DEBUG_P (rtx @var{x}) +This hook should return true if @var{x} should not be emitted into +debug sections. +@end deftypefn + @deftypefn {Target Hook} bool TARGET_CANNOT_FORCE_CONST_MEM (enum machine_mode @var{mode}, rtx @var{x}) This hook should return true if @var{x} is of a form that cannot (or should not) be spilled to the constant pool. @var{mode} is the mode --- gcc/dwarf2out.c.jj 2012-01-22 16:02:10.000000000 +0100 +++ gcc/dwarf2out.c 2012-01-23 13:58:09.887380014 +0100 @@ -10683,6 +10683,13 @@ const_ok_for_output_1 (rtx *rtlp, void * return 1; } + if (targetm.const_not_ok_for_debug_p (rtl)) + { + expansion_failed (NULL_TREE, rtl, + "Expression rejected for debug by the backend.\n"); + return 1; + } + if (GET_CODE (rtl) != SYMBOL_REF) return 0; --- gcc/config/rs6000/rs6000.c.jj 2012-01-22 16:02:10.000000000 +0100 +++ gcc/config/rs6000/rs6000.c 2012-01-23 13:25:05.092031129 +0100 @@ -1106,6 +1106,7 @@ static rtx rs6000_debug_legitimize_addre static rtx rs6000_legitimize_tls_address (rtx, enum tls_model); static void rs6000_output_dwarf_dtprel (FILE *, int, rtx) ATTRIBUTE_UNUSED; static rtx rs6000_delegitimize_address (rtx); +static bool rs6000_const_not_ok_for_debug_p (rtx); static rtx rs6000_tls_get_addr (void); static rtx rs6000_got_sym (void); static int rs6000_tls_symbol_ref_1 (rtx *, void *); @@ -1405,6 +1406,9 @@ static const struct attribute_spec rs600 #undef TARGET_DELEGITIMIZE_ADDRESS #define TARGET_DELEGITIMIZE_ADDRESS rs6000_delegitimize_address +#undef TARGET_CONST_NOT_OK_FOR_DEBUG_P +#define TARGET_CONST_NOT_OK_FOR_DEBUG_P rs6000_const_not_ok_for_debug_p + #undef TARGET_ASM_FUNCTION_PROLOGUE #define TARGET_ASM_FUNCTION_PROLOGUE rs6000_output_function_prologue #undef TARGET_ASM_FUNCTION_EPILOGUE @@ -5815,6 +5819,25 @@ rs6000_delegitimize_address (rtx orig_x) return orig_x; } +/* Return true if X shouldn't be emitted into the debug info. + The linker doesn't like .toc section references from + .debug_* sections, so reject .toc section symbols. */ + +static bool +rs6000_const_not_ok_for_debug_p (rtx x) +{ + if (GET_CODE (x) == SYMBOL_REF + && CONSTANT_POOL_ADDRESS_P (x)) + { + rtx c = get_pool_constant (x); + enum machine_mode cmode = get_pool_mode (x); + if (ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (c, cmode)) + return true; + } + + return false; +} + /* Construct the SYMBOL_REF for the tls_get_addr function. */ static GTY(()) rtx rs6000_tls_symbol; --- gcc/testsuite/gcc.dg/pr51957-1.c.jj 2012-01-23 14:06:45.024357678 +0100 +++ gcc/testsuite/gcc.dg/pr51957-1.c 2012-01-23 14:12:02.895492825 +0100 @@ -0,0 +1,29 @@ +/* PR target/51957 */ +/* { dg-do link } */ +/* { dg-options "-O2 -g -fprofile-use" } */ +/* { dg-additional-sources "pr51957-2.c" } */ + +int v[128]; +#include "pr51957-1.h" + +void +foo (U *x) +{ + T *a = x->u; + while (1) + { + union R *b; + b = fn1 (); + if (b != w[0] && !(v[b->p->c] == 1)) + { + fn2 (a->t, "foobar", b->p); + b = w[0]; + } + if (b != w[0]) + fn3 (); + if (w[0] && b != w[0]) + fn4 (b->p); + if (b != w[0] && (v[b->p->c] == 1) && fn4 (b->p)) + break; + } +} --- gcc/testsuite/gcc.dg/pr51957-1.h.jj 2012-01-23 14:06:56.491290757 +0100 +++ gcc/testsuite/gcc.dg/pr51957-1.h 2012-01-23 14:07:35.243060438 +0100 @@ -0,0 +1,9 @@ +union R { int c; union R *p; }; +extern union R *w[]; +typedef struct { int t; } T; +typedef struct { void *u; } U; +union R *fn1 (void); +void fn2 (int, const char *, union R *); +void fn3 (void); +int fn4 (union R *); +void foo (U *x); --- gcc/testsuite/gcc.dg/pr51957-2.c.jj 2012-01-23 14:10:59.840862700 +0100 +++ gcc/testsuite/gcc.dg/pr51957-2.c 2012-01-23 14:10:52.870904033 +0100 @@ -0,0 +1,35 @@ +/* PR target/51957 */ +/* { dg-do compile } */ +/* { dg-options "-O0" } */ + +#include "pr51957-1.h" + +union R *w[10]; + +union R * +fn1 (void) +{ + return (union R *) 0; +} + +void +fn2 (int x, const char *y, union R *z) +{ +} + +void +fn3 (void) +{ +} + +int +fn4 (union R *x) +{ + return 0; +} + +int +main () +{ + return 0; +}
Hi! profiledbootstrap currently fails on powerpc64-linux (64-bit). The problem is that the linker errors on .toc section references from within non-allocated sections. Alan changed the linker some months ago to not die with assertion failure, but error out on these, and the attached testcase reduced from cp/parser.c during bootstrap shows that we still can hit it in some cases. The problem here is that var-tracking.c (adjust_insn) isn't able to delegitimize it, because the UNSPEC_TOCREL isn't added there to r2 register, but to a debug_expr (which only afterwards is found to contain r2 value). So the UNSPEC_TOCREL makes it through to mem_loc_descriptor, where it is already properly delegitimized into a (mem (symbol_ref (".LC0")) where .LC0 is a constant pool (.toc) symbol. We in some places try to prefer avoid_constant_pool_reference values over the mems, in this case avoid_constant_pool returns (symbol_ref "w"), which is a non-local symbol which isn't const_ok_for_output though. So mem_loc_descriptor on the avoid_constant_pool_reference value fails and we try the original (mem (symbol_ref (".LC0")) - on other targets this is still useful, if we know the right value is in say some .got entry etc. and we can reference that location from within the .debug_info, we win. Unfortunately the ppc64 linker doesn't like this, so we need to reject those. The following patch adds a target hook and rejects there from emitting into .debug_info/.debug_loc any SYMBOL_REFs that are in the .toc section. Bootstrapped/regtested on x86_64-linux and i686-linux, profiledbootstrapped on powerpc64-linux (both --with-cpu=default32 and defaulting to 64-bit, the last one failed without this patch), powerpc64-linux regtest is in progress (but the testcase in the patch has been tested already to fail without the patch and succeed with the patch). Ok for trunk? 2012-01-23 Jakub Jelinek <jakub@redhat.com> PR target/51957 * target.def (const_not_ok_for_debug_p): New hook. * doc/tm.texi.in (TARGET_CONST_NOT_OK_FOR_DEBUG_P): New hook documentation. * doc/tm.texi: Regenerated. * dwarf2out.c (const_ok_for_output_1): If targetm.const_not_ok_for_debug_p returns true, fail. * config/rs6000/rs6000.c (rs6000_const_not_ok_for_debug_p): New function. (TARGET_CONST_NOT_OK_FOR_DEBUG_P): Redefine. * gcc.dg/pr51957-1.c: New test. * gcc.dg/pr51957-1.h: New file. * gcc.dg/pr51957-2.c: New test. Jakub