| Message ID | 20150420031049.GB12627@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Apr 20, 2015 at 12:40:49PM +0930, Alan Modra wrote: > with the log for the ubsan fails > /src/gcc-5/gcc/testsuite/c-c++-common/ubsan/object-size-10.c:19:11: runtime error: index 128 out of bounds for type 'char [128]' > /src/gcc-5/gcc/testsuite/c-c++-common/ubsan/object-size-10.c:19:11: runtime error: load of address 0x0804a000 with insufficient space for an object of type 'char' > 0x0804a000: note: pointer points here > <memory cannot be printed> The issue here is that libsanitizer wants to print some context around the variable, and doesn't try too hard, so if the variable is too close to the end of the RW PT_LOAD, you get different message from what is expected. In your case, most likely the end of the array happens to be exactly at the end of the PT_LOAD segment. So, the fix is either to try harder in ubsan renderMemorySnippet function (it first computes the region it wishes to print, then has if (!IsAccessibleMemoryRange(Min, Max - Min)) { Printf("<memory cannot be printed>\n"); return; } ). Supposedly it could, if there are any page boundary crosses in the Min .. Max region lower a little bit (to the page boundary) the end and/or increase to the page boundary the start, and retry with that if it is accessible. Or we'd need to make the testcases that suffer from this accept also the <memory cannot be printed> in place of the memory content line, line with ^ marker (don't remember if there is yet another one). > gcc/ > PR debug/65779 > * shrink-wrap.c (insn_uses_reg): New function. > (move_insn_for_shrink_wrap): Remove debug insns using regs set > by the moved insn. > gcc/testsuite/ > * gcc.dg/pr65779.c: New. > > Index: gcc/shrink-wrap.c > =================================================================== > --- gcc/shrink-wrap.c (revision 222160) > +++ gcc/shrink-wrap.c (working copy) > @@ -182,6 +182,21 @@ live_edge_for_reg (basic_block bb, int regno, int > return live_edge; > } > > +static bool > +insn_uses_reg (rtx_insn *insn, unsigned int regno, unsigned int end_regno) > +{ > + df_ref use; > + > + FOR_EACH_INSN_USE (use, insn) > + { > + rtx reg = DF_REF_REG (use); > + > + if (REG_P (reg) && REGNO (reg) >= regno && REGNO (reg) < end_regno) > + return true; > + } > + return false; > +} > + > /* Try to move INSN from BB to a successor. Return true on success. > USES and DEFS are the set of registers that are used and defined > after INSN in BB. SPLIT_P indicates whether a live edge from BB > @@ -340,10 +355,15 @@ move_insn_for_shrink_wrap (basic_block bb, rtx_ins > *split_p = true; > } > > + vec<basic_block> live_bbs; > + if (MAY_HAVE_DEBUG_INSNS) > + live_bbs.create (5); Just wonder if using an auto_vec<basic_block, 5> live_bbs; > + FOR_BB_INSNS_REVERSE (tmp_bb, dinsn) > + { > + if (dinsn == insn) > + break; > + if (DEBUG_INSN_P (dinsn) > + && insn_uses_reg (dinsn, dregno, end_dregno)) > + { > + if (*split_p) > + /* If split, then we will be moving insn into a > + newly created block immediately after the entry > + block. Move the debug info there too. */ > + emit_debug_insn_after (PATTERN (dinsn), bb_note (bb)); > + delete_insn (dinsn); Debug insns should never be deleted, nor moved. You should either reset them (INSN_VAR_LOCATION_LOC (insn) = gen_rtx_UNKNOWN_VAR_LOC (); plus df_insn_rescan_debug_internal (insn);), or try to adjust them based on the instruction setting the register (say, if insn sets the register to some other register + 10 and the other register is still live, you could replace the uses of the register with (plus (the other register) (const_int 10)). > + live_bbs.release (); If live_bbs is auto_vec, this would not be needed. Jakub
On Mon, Apr 20, 2015 at 09:35:07AM +0200, Jakub Jelinek wrote: > On Mon, Apr 20, 2015 at 12:40:49PM +0930, Alan Modra wrote: > > with the log for the ubsan fails > > /src/gcc-5/gcc/testsuite/c-c++-common/ubsan/object-size-10.c:19:11: runtime error: index 128 out of bounds for type 'char [128]' > > /src/gcc-5/gcc/testsuite/c-c++-common/ubsan/object-size-10.c:19:11: runtime error: load of address 0x0804a000 with insufficient space for an object of type 'char' > > 0x0804a000: note: pointer points here > > <memory cannot be printed> > > The issue here is that libsanitizer wants to print some context around the > variable, and doesn't try too hard, so if the variable is too close to the > end of the RW PT_LOAD, you get different message from what is expected. Thanks for the info. I don't tend to run sanitizer tests, so this was the first time I'd seen such a failure. > Just wonder if using an > auto_vec<basic_block, 5> live_bbs; OK, done. > > + FOR_BB_INSNS_REVERSE (tmp_bb, dinsn) > > + { > > + if (dinsn == insn) > > + break; > > + if (DEBUG_INSN_P (dinsn) > > + && insn_uses_reg (dinsn, dregno, end_dregno)) > > + { > > + if (*split_p) > > + /* If split, then we will be moving insn into a > > + newly created block immediately after the entry > > + block. Move the debug info there too. */ > > + emit_debug_insn_after (PATTERN (dinsn), bb_note (bb)); > > + delete_insn (dinsn); > > Debug insns should never be deleted, nor moved. You should either > reset them > (INSN_VAR_LOCATION_LOC (insn) = gen_rtx_UNKNOWN_VAR_LOC (); plus > df_insn_rescan_debug_internal (insn);), or I had it that way in my first patch, then decided to try deleting.. I can certainly change it back even if only to do it the standard way for safety's sake, but I'm curious as to why they can't be deleted in this special case. My thinking was that we're on a chain of blocks starting at the entry where there is a single outgoing live edge for the register being used. So there shouldn't be any need for a debug insn to mark info about the variable as invalid.
On Mon, Apr 20, 2015 at 06:12:26PM +0930, Alan Modra wrote: > I had it that way in my first patch, then decided to try deleting.. > > I can certainly change it back even if only to do it the standard way > for safety's sake, but I'm curious as to why they can't be deleted in > this special case. My thinking was that we're on a chain of blocks > starting at the entry where there is a single outgoing live edge for > the register being used. So there shouldn't be any need for a debug > insn to mark info about the variable as invalid. The debug insns can be for arbitrary variables, there is no "the variable", and there could be other debug insns for the same variable on that path, say saying that decl lives in some other register, or can be computed using an expression involving other registers, or memory etc. Say you could have (set regX (whatever)) ... (debug_insn var5 (some expression not referring to regX)) ... (debug_insn var5 (some expression referring to regX)) ... (debug_insn var5 (other expression not referring to regX)) ... (use regX) where ... contains unrelated insns (not referring to regX) and edges live for regX. If shrink wrapping attempts to move the first set somewhere into the last ..., if you delete debug insns referring to regX, you extend the lifetime of the previous debug_insn, which is wrong, the registers referenced in the first debug_insn might not contain the right values afterwards. And if you move the debug insn later, you might shorten the lifetime of the third debug_insn, while regX is supposed to contain the same value, perhaps some other register referenced there might have been changed already. Jakub
On Mon, Apr 20, 2015 at 10:55:56AM +0200, Jakub Jelinek wrote: > On Mon, Apr 20, 2015 at 06:12:26PM +0930, Alan Modra wrote: > > I had it that way in my first patch, then decided to try deleting.. > > > > I can certainly change it back even if only to do it the standard way > > for safety's sake, but I'm curious as to why they can't be deleted in > > this special case. My thinking was that we're on a chain of blocks > > starting at the entry where there is a single outgoing live edge for > > the register being used. So there shouldn't be any need for a debug > > insn to mark info about the variable as invalid. > > The debug insns can be for arbitrary variables, there is no "the variable", Sure. > and there could be other debug insns for the same variable on that path, > say saying that decl lives in some other register, or can be computed using > an expression involving other registers, or memory etc. Say you could have Yes, that's true in the general case. For the shrink-wrap case, any bb (or tail of the entry block) that we move over has no use or def of the register. So I'm left wondering how it would be possible for the var to live in some other register or memory? Probably lack of imagination on my part, but the only scenarios I see would involve a failure of cse. Anyway, I rewrote the patch to do as you suggested, and started looking at .debug_loc in gcc/*.o for files that differed between the two approaches. (Only 32 files differed, besides the expected shrinkwrap.o and checksum files.) That was a bit of a revelation, and no wonder powerpc debugging is such a pain.. The first file I looked at was reload.o, and the first difference is in location lists for get_secondary_mem mode param. It looks like virgin: 000086c5 0000000000005c90 0000000000005cd7 (DW_OP_reg4 (r4)) 000086d8 0000000000005cd7 0000000000005cd8 (DW_OP_GNU_entry_value: (DW_OP_reg4 (r4)); DW_OP_stack_value) 000086ee 0000000000005cd8 0000000000005cf8 (DW_OP_reg30 (r30)) 00008701 0000000000005d2c 0000000000005d38 (DW_OP_reg30 (r30)) delete: 000086ae 0000000000005c90 0000000000005cd7 (DW_OP_reg4 (r4)) 000086c1 0000000000005cd7 0000000000005ec0 (DW_OP_GNU_entry_value: (DW_OP_reg4 (r4)); DW_OP_stack_value) zap: 000086ae 0000000000005c90 0000000000005cd7 (DW_OP_reg4 (r4)) 000086c1 0000000000005cd7 0000000000005cd8 (DW_OP_GNU_entry_value: (DW_OP_reg4 (r4)); DW_OP_stack_value) and the code: 5cd0: 48 00 00 01 bl 5cd0 <._Z17get_secondary_memP7rtx_def12machine_modei11reload_type+0x40> 5cd0: R_PPC64_REL24 _Z35rs6000_secondary_memory_needed_mode12machine_mode 5cd4: 60 00 00 00 nop 5cd8: 7c 7d 07 b4 extsw r29,r3 5cdc: 1f fd 00 1e mulli r31,r29,30 5ce0: 7d 3f da 14 add r9,r31,r27 5ce4: 79 29 1f 24 rldicr r9,r9,3,60 5ce8: 7d 3c 4a 14 add r9,r28,r9 5cec: e9 29 55 a8 ld r9,21928(r9) 5cf0: 2f a9 00 00 cmpdi cr7,r9,0 5cf4: 41 9e 00 3c beq cr7,5d30 <._Z17get_secondary_memP7rtx_def12machine_modei11reload_type+0xa0> 5cf8: 38 21 00 c0 addi r1,r1,192 5cfc: 7d 23 4b 78 mr r3,r9 ..rest of epilogue 5d30: 7b b9 1f 24 rldicr r25,r29,3,60 5d34: 7c 7e 1b 78 mr r30,r3 5d38: 7f 1c ca 14 add r24,r28,r25 Arrgh! In the first place the ranges are wrong since r4 dies after the bl, not the toc restoring nop. Worse, both deleting and zapping (ie UNKNOWN_VAR_LOC) the debug insn is wrong. My simplistic patch isn't correct. In fact it makes the debug info worse. However, leaving the debug insn alone says "mode" lives in r30, but that is wrong since the copy to r30 insn has been moved, to 5d34. Apparently the move causes the virgin location lists to say "mode" disappears at that point too. What a mess! Of course, all this moving for shrink-wrap is senseless in a block that contains a call.
On Mon, Apr 20, 2015 at 10:30:32PM +0930, Alan Modra wrote: > > and there could be other debug insns for the same variable on that path, > > say saying that decl lives in some other register, or can be computed using > > an expression involving other registers, or memory etc. Say you could have > > Yes, that's true in the general case. For the shrink-wrap case, any > bb (or tail of the entry block) that we move over has no use or def of > the register. So I'm left wondering how it would be possible for the > var to live in some other register or memory? Probably lack of > imagination on my part, but the only scenarios I see would involve a > failure of cse. E.g. the variable might have different values in the source code. You can have int var1 = 5; // some statements var1 = parmx + 20; // some statements var1 = 30; // ... in the source and consider that the shrink wrapping insn is attempting to move (set (regX) (plus (reg parmx) (const_int 20))) later. You could have different debug insns for the same var1, and trying to move the debug_insn later would break things. Usually, gcc only adds further debug stmts (or insns), e.g. if in a range between earlier two debug stmts (or insns) the var is known to contain some particular value, but expression having that value or something used in it (SSA_NAME, pseudo REG), is optimized away, if there is some other way to get the same value, the range could be split into two with two different expressions etc. > Arrgh! In the first place the ranges are wrong since r4 dies after > the bl, not the toc restoring nop. Worse, both deleting and zapping > (ie UNKNOWN_VAR_LOC) the debug insn is wrong. My simplistic patch > isn't correct. In fact it makes the debug info worse. However, Zapping is conservatively correct, if you don't know where the var lives in or how to compute it, you tell the debugger you don't know it. Of course, it is a QoI issue, if there is an easy way how to reconstruct the value otherwise, it is always better to do so. > leaving the debug insn alone says "mode" lives in r30, but that is > wrong since the copy to r30 insn has been moved, to 5d34. Apparently > the move causes the virgin location lists to say "mode" disappears at > that point too. What a mess! > > Of course, all this moving for shrink-wrap is senseless in a block > that contains a call. Yeah, such blocks clearly aren't going to be shrink-wrapped, so there is no point to move it that far, right? Jakub
On 04/19/2015 09:10 PM, Alan Modra wrote: > This patch removes bogus debug info left around by shrink-wrapping, > which on some powerpc targets with just the right register allocation > led to assembly errors. > > Bootstrapped and regression tested powerpc64-linux and x86_64-linux. > I did see some regressions, but completely unrelated to this patch. > See pr65810 for the powerpc64 regressions. x86_64-linux showed fails > of > +FAIL: c-c++-common/ubsan/object-size-10.c -O2 execution test > +FAIL: c-c++-common/ubsan/object-size-10.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test > +FAIL: c-c++-common/ubsan/object-size-10.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects execution test > +FAIL: gfortran.dg/class_allocate_18.f90 -O0 execution test > +FAIL: gfortran.dg/class_allocate_18.f90 -O1 execution test > FAIL: gfortran.dg/class_allocate_18.f90 -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions execution test > -FAIL: gfortran.dg/class_allocate_18.f90 -Os execution test > +FAIL: gfortran.dg/class_allocate_18.f90 -O3 -g execution test > > with the log for the ubsan fails > /src/gcc-5/gcc/testsuite/c-c++-common/ubsan/object-size-10.c:19:11: runtime error: index 128 out of bounds for type 'char [128]' > /src/gcc-5/gcc/testsuite/c-c++-common/ubsan/object-size-10.c:19:11: runtime error: load of address 0x0804a000 with insufficient space for an object of type 'char' > 0x0804a000: note: pointer points here > <memory cannot be printed> > I assume I was thrashing my ubuntu 14.10 x86_64 box too hard and just > ran out of memory. Running the test by hand resulted in the expected > output. > > The class_allocate_18.f90 failure are intermittent, and occur > occasionally when running the testcase by hand. :-( Yea, I think folks are still trying to sort out what's happening with class_allocate_18.f90. > > gcc/ > PR debug/65779 > * shrink-wrap.c (insn_uses_reg): New function. > (move_insn_for_shrink_wrap): Remove debug insns using regs set > by the moved insn. > gcc/testsuite/ > * gcc.dg/pr65779.c: New. > > Index: gcc/shrink-wrap.c > =================================================================== > --- gcc/shrink-wrap.c (revision 222160) > +++ gcc/shrink-wrap.c (working copy) > @@ -182,6 +182,21 @@ live_edge_for_reg (basic_block bb, int regno, int > return live_edge; > } > > +static bool > +insn_uses_reg (rtx_insn *insn, unsigned int regno, unsigned int end_regno) > +{ > + df_ref use; > + > + FOR_EACH_INSN_USE (use, insn) > + { > + rtx reg = DF_REF_REG (use); > + > + if (REG_P (reg) && REGNO (reg) >= regno && REGNO (reg) < end_regno) > + return true; > + } > + return false; > +} Need a comment for this function. So just one question. Why handle the split case differently? In the split case you effectively move the debug insn to the new block. In the !split case, you just delete the debug insn. I'm sure there's a reason, it would be worth noting it as a comment in this code. OK with the comments added. jeff
On Mon, Apr 20, 2015 at 12:17:08PM -0600, Jeff Law wrote: > So just one question. Why handle the split case differently? In the split > case you effectively move the debug insn to the new block. In the !split > case, you just delete the debug insn. The idea was that when split we have a new block with no debug info, but !split the block already has debug info about the var/reg. Now that I've looked at a lot more .debug_loc sections than I ever wanted, and the debug_insns that generate that info, I realize my assumption about the !split case is wrong. Working on a new patch to fix some of the things I've discovered.
Index: gcc/shrink-wrap.c =================================================================== --- gcc/shrink-wrap.c (revision 222160) +++ gcc/shrink-wrap.c (working copy) @@ -182,6 +182,21 @@ live_edge_for_reg (basic_block bb, int regno, int return live_edge; } +static bool +insn_uses_reg (rtx_insn *insn, unsigned int regno, unsigned int end_regno) +{ + df_ref use; + + FOR_EACH_INSN_USE (use, insn) + { + rtx reg = DF_REF_REG (use); + + if (REG_P (reg) && REGNO (reg) >= regno && REGNO (reg) < end_regno) + return true; + } + return false; +} + /* Try to move INSN from BB to a successor. Return true on success. USES and DEFS are the set of registers that are used and defined after INSN in BB. SPLIT_P indicates whether a live edge from BB @@ -340,10 +355,15 @@ move_insn_for_shrink_wrap (basic_block bb, rtx_ins *split_p = true; } + vec<basic_block> live_bbs; + if (MAY_HAVE_DEBUG_INSNS) + live_bbs.create (5); /* At this point we are committed to moving INSN, but let's try to move it as far as we can. */ do { + if (MAY_HAVE_DEBUG_INSNS) + live_bbs.safe_push (bb); live_out = df_get_live_out (bb); live_in = df_get_live_in (next_block); bb = next_block; @@ -426,6 +446,34 @@ move_insn_for_shrink_wrap (basic_block bb, rtx_ins SET_REGNO_REG_SET (bb_uses, i); } + /* Remove debug insns using regs set by the insn we are moving. */ + if (MAY_HAVE_DEBUG_INSNS) + { + while (!live_bbs.is_empty ()) + { + rtx_insn *dinsn; + basic_block tmp_bb = live_bbs.pop (); + + FOR_BB_INSNS_REVERSE (tmp_bb, dinsn) + { + if (dinsn == insn) + break; + if (DEBUG_INSN_P (dinsn) + && insn_uses_reg (dinsn, dregno, end_dregno)) + { + if (*split_p) + /* If split, then we will be moving insn into a + newly created block immediately after the entry + block. Move the debug info there too. */ + emit_debug_insn_after (PATTERN (dinsn), bb_note (bb)); + delete_insn (dinsn); + break; + } + } + } + live_bbs.release (); + } + emit_insn_after (PATTERN (insn), bb_note (bb)); delete_insn (insn); return true; Index: gcc/testsuite/gcc.dg/pr65779.c =================================================================== --- gcc/testsuite/gcc.dg/pr65779.c (revision 0) +++ gcc/testsuite/gcc.dg/pr65779.c (working copy) @@ -0,0 +1,64 @@ +/* { dg-do run } */ +/* { dg-options "-O2 -g" } */ +/* { dg-additional-options "-mrelocatable" { target powerpc-*-rtems* } } */ + +unsigned long __attribute__ ((noinline)) +adler32 (unsigned long adler, unsigned char *buf, unsigned int len) +{ + unsigned long s1 = adler & 0xffff; + unsigned long s2 = (adler >> 16) & 0xffff; + int k; + + if (buf == 0) + return 1L; + + while (len > 0) + { + k = len < 5552 ? len : 5552; + len -= k; + while (k >= 16) + { + s1 += *buf++; s2 += s1; + s1 += *buf++; s2 += s1; + s1 += *buf++; s2 += s1; + s1 += *buf++; s2 += s1; + s1 += *buf++; s2 += s1; + s1 += *buf++; s2 += s1; + s1 += *buf++; s2 += s1; + s1 += *buf++; s2 += s1; + s1 += *buf++; s2 += s1; + s1 += *buf++; s2 += s1; + s1 += *buf++; s2 += s1; + s1 += *buf++; s2 += s1; + s1 += *buf++; s2 += s1; + s1 += *buf++; s2 += s1; + s1 += *buf++; s2 += s1; + s1 += *buf++; s2 += s1; + k -= 16; + } + if (k != 0) + do + { + s1 += *buf++; s2 += s1; + } while (--k); + s1 &= 0xffffffffUL; + s2 &= 0xffffffffUL; + s1 %= 65521L; + s2 %= 65521L; + } + return (s2 << 16) | s1; +} + +unsigned char buf[] = { 0, 1, 2, 3, 4, 5, 6, 7, + 8, 9, 10, 11, 12, 13, 14, 15, + 0x80, 0x81, 0x82, 0x83, 0x84, 0x85, 0x86, 0x87, + 0x88, 0x89, 0x8a, 0x8b, 0x8c, 0x8d, 0x8e, 0x8f, + 0x55, 0xaa }; +int +main () +{ + unsigned long x = adler32 (0, buf, sizeof buf); + if (x != 0x640409efUL) + __builtin_abort (); + return 0; +}