| Message ID | 4F246144.4070109@mentor.com |
|---|---|
| State | New |
| Headers | show |
On 01/29/2012 07:57 AM, Tom de Vries wrote: > Richard, > > [now cc-ing gcc-patches] > > this patch fixes PR50283 in a target-independent way. > > it asserts on frame-related insns in the delay slot of insns that can throw, > and prevents the assert by testing for the same condition in > eligible_for_{delay,annul_true,annul_false}. > > build and reg-tested on mips64el. > > I don't know of any tests currently failing on this, so I think it's not > appropriate for stage4. OK for stage1 ? > If we can't tell one unwinder (eh) what to do with this, how do we expect to be able to tell another unwinder (gdb)? Thus I'm not sure how "throwing" has anything to do with it. I certainly agree that we shouldn't do anything during stage4 unless we can come up with a wrong-code test case. r~
> On 01/29/2012 07:57 AM, Tom de Vries wrote: > > Richard, > > > > [now cc-ing gcc-patches] > > > > this patch fixes PR50283 in a target-independent way. > > > > it asserts on frame-related insns in the delay slot of insns that can > > throw, and prevents the assert by testing for the same condition in > > eligible_for_{delay,annul_true,annul_false}. > > > > build and reg-tested on mips64el. > > > > I don't know of any tests currently failing on this, so I think it's not > > appropriate for stage4. OK for stage1 ? > > If we can't tell one unwinder (eh) what to do with this, how do we > expect to be able to tell another unwinder (gdb)? Thus I'm not sure > how "throwing" has anything to do with it. If you need a reliable backtrace from any point then I guess you shouldn't allow frame related insns in delay slots at all. However this is a issue of debug experiance rather than code correctness. My guess is most architectures don't allow you to singlestep into delay slots, so any use of delay slots may cause lossage[1], not just frame related ones. Debugging optimized code is always a compromise. It's worth noting that -fasync-unwind-tables does not guarantee backtrace/unwind from arbitrary points. Only from those instructions that may result in a synchronous trap (which matches the semantics of this patch). I'd guess that putting frame related insns in delay slots of libcalls is probably a worthwhile optimization. Paul [1] If the insns come from different source lines.
On 01/30/2012 11:07 AM, Paul Brook wrote: > However this is a issue of debug experiance rather than code correctness. My > guess is most architectures don't allow you to singlestep into delay slots, so > any use of delay slots may cause lossage[1], not just frame related ones. Certainly Sparc can single-step delay slots. I assumed others do as well. r~
On 01/30/2012 11:07 AM, Paul Brook wrote: > It's worth noting that -fasync-unwind-tables does not guarantee > backtrace/unwind from arbitrary points. Only from those instructions that may > result in a synchronous trap (which matches the semantics of this patch). ... and we're not talking about arbitrary points. We're talking about call sites. Whether or not the call itself is marked nothrow. r~
> On 01/30/2012 11:07 AM, Paul Brook wrote: > > It's worth noting that -fasync-unwind-tables does not guarantee > > backtrace/unwind from arbitrary points. Only from those instructions > > that may result in a synchronous trap (which matches the semantics of > > this patch). > > ... and we're not talking about arbitrary points. We're talking about call > sites. Whether or not the call itself is marked nothrow. What about backtracing from a signal handler? Paul
Index: gcc/genattrtab.c =================================================================== --- gcc/genattrtab.c (revision 183557) +++ gcc/genattrtab.c (working copy) @@ -4280,6 +4280,12 @@ write_eligible_delay (const char *kind) printf (" if (!INSN_P (candidate_insn))\n"); printf (" return 0;\n"); printf ("\n"); + /* Frame-related instructions should not be put into delay slots of + instructions that can throw. */ + printf (" if (insn_could_throw_p (delay_insn)\n"); + printf (" && RTX_FRAME_RELATED_P (candidate_insn))\n"); + printf (" return 0;\n"); + printf ("\n"); /* If more than one delay type, find out which type the delay insn is. */ Index: gcc/dwarf2cfi.c =================================================================== --- gcc/dwarf2cfi.c (revision 183557) +++ gcc/dwarf2cfi.c (working copy) @@ -2474,6 +2474,8 @@ scan_trace (dw_trace_info *trace) for (i = 1; i < n; ++i) { elt = XVECEXP (pat, 0, i); + gcc_assert (!(insn_could_throw_p (control) + && RTX_FRAME_RELATED_P (elt))); scan_insn_after (elt); }
Richard, [now cc-ing gcc-patches] this patch fixes PR50283 in a target-independent way. it asserts on frame-related insns in the delay slot of insns that can throw, and prevents the assert by testing for the same condition in eligible_for_{delay,annul_true,annul_false}. build and reg-tested on mips64el. I don't know of any tests currently failing on this, so I think it's not appropriate for stage4. OK for stage1 ? Thanks, - Tom 2012-01-27 Andrew Pinski <apinski@cavium.com> Tom de Vries <tom@codesourcery.com> * dwarf2cfi.c (scan_trace): Add assert that frame-related insns should not occur in delay-slots of insns that can throw. * genattrtab.c (write_eligible_delay): Prevent frame-related insns from occurring in delay-slots of insns that can throw.