Patchwork Prevent frame-related insn from occurring in delay slots of insns that throw

login
register
mail settings
Submitter Tom de Vries
Date Jan. 28, 2012, 8:57 p.m.
Message ID <4F246144.4070109@mentor.com>
Download mbox | patch
Permalink /patch/138415/
State New
Headers show

Comments

Tom de Vries - Jan. 28, 2012, 8:57 p.m.
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.
Richard Henderson - Jan. 29, 2012, 8:46 p.m.
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~
Paul Brook - Jan. 30, 2012, 12:07 a.m.
> 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.
Richard Henderson - Jan. 30, 2012, 12:09 a.m.
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~
Richard Henderson - Jan. 30, 2012, 12:12 a.m.
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~
Paul Brook - Jan. 30, 2012, 6:14 a.m.
> 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

Patch

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);
 	    }