Patchwork Add unwind information to mips epilogues

login
register
mail settings
Submitter Bernd Schmidt
Date Aug. 31, 2011, 5:25 p.m.
Message ID <4E5E6E6D.8050905@codesourcery.com>
Download mbox | patch
Permalink /patch/112628/
State New
Headers show

Comments

Bernd Schmidt - Aug. 31, 2011, 5:25 p.m.
This is necessary when adding shrink-wrapping; otherwise dwarf2cfi sees
inconsistent information and aborts.

Tested on mips64-elf together with the rest of the shrink-wrapping
patches. Ok?


Bernd
* config/mips/mips.c (cfa_restores): New static variable.
	(mips_restore_reg): Add to it.
	(mips_expand_epilogue): Initialize it.  Annotate RTL to ensure
	dwarf2cfi sees the effects of the epilogue.
Richard Sandiford - Aug. 31, 2011, 6:43 p.m.
Bernd Schmidt <bernds@codesourcery.com> writes:
> This is necessary when adding shrink-wrapping; otherwise dwarf2cfi sees
> inconsistent information and aborts.
>
> Tested on mips64-elf together with the rest of the shrink-wrapping
> patches. Ok?

It looks like the current code doesn't handle the RESTORE instruction.
Could you also test that somehow?  A mipsisa32-elf run with -mips16
ought to work, but some sort of spot-checking of shrink-wrapping +
RESTORE would be fine if that's easier.

Also, for the frame_pointer_required case, it looks like there's a
window between the restoration of the frame pointer and the deallocation
of the stack in which the CFA is still defined in terms of the frame
pointer register.  Is that significant?  If not (e.g. because we
should never need to unwind at that point) then why do we still
update the CFA here:

> @@ -10324,12 +10330,26 @@ mips_expand_epilogue (bool sibcall_p)
>        if (!TARGET_MIPS16)
>  	target = stack_pointer_rtx;
>  
> -      emit_insn (gen_add3_insn (target, base, adjust));
> +      insn = emit_insn (gen_add3_insn (target, base, adjust));
> +      if (!frame_pointer_needed && target == stack_pointer_rtx)
> +	{
> +	  RTX_FRAME_RELATED_P (insn) = 1;
> +	  add_reg_note (insn, REG_CFA_DEF_CFA,
> +			plus_constant (stack_pointer_rtx, step2));
> +	}

and here:

>    /* Copy TARGET into the stack pointer.  */
>    if (target != stack_pointer_rtx)
> -    mips_emit_move (stack_pointer_rtx, target);
> +    {
> +      insn = mips_emit_move (stack_pointer_rtx, target);
> +      if (!frame_pointer_needed)
> +	{
> +	  add_reg_note (insn, REG_CFA_DEF_CFA,
> +			plus_constant (stack_pointer_rtx, step2));
> +	  RTX_FRAME_RELATED_P (insn) = 1;
> +	}
> +    }

?  If the window is significant, could we avoid it by removing the
!frame_pointer_needed checks from the code above?

Richard
Bernd Schmidt - Aug. 31, 2011, 9:37 p.m.
On 08/31/11 20:43, Richard Sandiford wrote:
> Bernd Schmidt <bernds@codesourcery.com> writes:
>> This is necessary when adding shrink-wrapping; otherwise dwarf2cfi sees
>> inconsistent information and aborts.
>>
>> Tested on mips64-elf together with the rest of the shrink-wrapping
>> patches. Ok?
> 
> It looks like the current code doesn't handle the RESTORE instruction.
> Could you also test that somehow?  A mipsisa32-elf run with -mips16
> ought to work, but some sort of spot-checking of shrink-wrapping +
> RESTORE would be fine if that's easier.

Will look into that. You mean the mips16e_build_save_restore function?

> Also, for the frame_pointer_required case, it looks like there's a
> window between the restoration of the frame pointer and the deallocation
> of the stack in which the CFA is still defined in terms of the frame
> pointer register.  Is that significant?  If not (e.g. because we
> should never need to unwind at that point) then why do we still
> update the CFA here:

CC'ing rth, as I'm still not terribly familiar with these issues. I've
tried to follow alpha.c, which seems to ignore this issue.

>> @@ -10324,12 +10330,26 @@ mips_expand_epilogue (bool sibcall_p)
>>        if (!TARGET_MIPS16)
>>  	target = stack_pointer_rtx;
>>  
>> -      emit_insn (gen_add3_insn (target, base, adjust));
>> +      insn = emit_insn (gen_add3_insn (target, base, adjust));
>> +      if (!frame_pointer_needed && target == stack_pointer_rtx)
>> +	{
>> +	  RTX_FRAME_RELATED_P (insn) = 1;
>> +	  add_reg_note (insn, REG_CFA_DEF_CFA,
>> +			plus_constant (stack_pointer_rtx, step2));
>> +	}

Here, with !frame_pointer_needed, SP should be the CFA register, so if
we modify it we have to use REG_CFA_DEF_CFA. Either that, or I'm confused.

> ?  If the window is significant, could we avoid it by removing the
> !frame_pointer_needed checks from the code above?

That causes aborts due to inconsistent information in dwarf2cfi, since
we can reach the same instruction with either fp or sp as the CFA register.


Bernd
Richard Sandiford - Sept. 1, 2011, 7:49 a.m.
Bernd Schmidt <bernds@codesourcery.com> writes:
> On 08/31/11 20:43, Richard Sandiford wrote:
>> Bernd Schmidt <bernds@codesourcery.com> writes:
>>> This is necessary when adding shrink-wrapping; otherwise dwarf2cfi sees
>>> inconsistent information and aborts.
>>>
>>> Tested on mips64-elf together with the rest of the shrink-wrapping
>>> patches. Ok?
>> 
>> It looks like the current code doesn't handle the RESTORE instruction.
>> Could you also test that somehow?  A mipsisa32-elf run with -mips16
>> ought to work, but some sort of spot-checking of shrink-wrapping +
>> RESTORE would be fine if that's easier.
>
> Will look into that. You mean the mips16e_build_save_restore function?

Yeah.

>> Also, for the frame_pointer_required case, it looks like there's a
>> window between the restoration of the frame pointer and the deallocation
>> of the stack in which the CFA is still defined in terms of the frame
>> pointer register.  Is that significant?  If not (e.g. because we
>> should never need to unwind at that point) then why do we still
>> update the CFA here:
>
> CC'ing rth, as I'm still not terribly familiar with these issues.

Me too.

>>> @@ -10324,12 +10330,26 @@ mips_expand_epilogue (bool sibcall_p)
>>>        if (!TARGET_MIPS16)
>>>  	target = stack_pointer_rtx;
>>>  
>>> -      emit_insn (gen_add3_insn (target, base, adjust));
>>> +      insn = emit_insn (gen_add3_insn (target, base, adjust));
>>> +      if (!frame_pointer_needed && target == stack_pointer_rtx)
>>> +	{
>>> +	  RTX_FRAME_RELATED_P (insn) = 1;
>>> +	  add_reg_note (insn, REG_CFA_DEF_CFA,
>>> +			plus_constant (stack_pointer_rtx, step2));
>>> +	}
>
> Here, with !frame_pointer_needed, SP should be the CFA register, so if
> we modify it we have to use REG_CFA_DEF_CFA. Either that, or I'm confused.

What I meant was: if we can ignore the state between the restoration
of the frame pointer and final stack deallocation, why can't we also
ignore the state between this intermediate deallocation and the final one?
I.e. why isn't it enough to attach a DEF_CFA to the final deallocation only?
In principle, I mean; I realise dwarf2cfi might not like it.

Richard
Bernd Schmidt - Sept. 1, 2011, 5:19 p.m.
On 08/31/11 20:43, Richard Sandiford wrote:
> Bernd Schmidt <bernds@codesourcery.com> writes:
>> This is necessary when adding shrink-wrapping; otherwise dwarf2cfi sees
>> inconsistent information and aborts.
>>
>> Tested on mips64-elf together with the rest of the shrink-wrapping
>> patches. Ok?
> 
> It looks like the current code doesn't handle the RESTORE instruction.
> Could you also test that somehow?  A mipsisa32-elf run with -mips16
> ought to work

Hmm, I'm having some trouble with that.
 * mipsisa32-elf doesn't build on trunk
 * 4.6 doesn't allow plain "-mips16"; I have to add "-msoft-float"
 * With that, 4.6 produces rather a lot of testsuite failures.
   (>400 execute failures in my current run and it's not even gotten
   very far)

Just to make sure - am I missing anything here, or is this stuff just in
bad shape?


Bernd
Richard Sandiford - Sept. 1, 2011, 6:41 p.m.
Bernd Schmidt <bernds@codesourcery.com> writes:
> On 08/31/11 20:43, Richard Sandiford wrote:
>> Bernd Schmidt <bernds@codesourcery.com> writes:
>>> This is necessary when adding shrink-wrapping; otherwise dwarf2cfi sees
>>> inconsistent information and aborts.
>>>
>>> Tested on mips64-elf together with the rest of the shrink-wrapping
>>> patches. Ok?
>> 
>> It looks like the current code doesn't handle the RESTORE instruction.
>> Could you also test that somehow?  A mipsisa32-elf run with -mips16
>> ought to work
>
> Hmm, I'm having some trouble with that.
>  * mipsisa32-elf doesn't build on trunk
>  * 4.6 doesn't allow plain "-mips16"; I have to add "-msoft-float"
>  * With that, 4.6 produces rather a lot of testsuite failures.
>    (>400 execute failures in my current run and it's not even gotten
>    very far)
>
> Just to make sure - am I missing anything here, or is this stuff just in
> bad shape?

Gah, my bad, sorry.  I'd forgotten mipsisa32-elf is EABI32
rather than o32.  mips-elf with -mips32/-mips16 would test
what I was after.  Or the kind of spot-checks I was thinking
of could be done with any mips* compiler.  Compile some tests
that are interesting for shrinking wrapping with:

    -c -mips16 -mips32 -mabi=32 -Owhatever

and see if (a) it compiles, (b) it uses shrink-wrap and RESTORE
and (c) the CFI dump (from readelf -wf or whatever) looks OK.

Thanks for trying though.  I'll look into the mipsisa32-elf build failure
and (if I get time) the EABI32 -mips16/-msoft-float problem.

(For the record, I do test -mips16 fairly regularly, but at a lower
ISA level.  I suppose I should try MIPS16e more myself too....)

Richard
Richard Sandiford - Sept. 1, 2011, 7:19 p.m.
Richard Sandiford <rdsandiford@googlemail.com> writes:
> Gah, my bad, sorry.  I'd forgotten mipsisa32-elf is EABI32
> rather than o32.  mips-elf with -mips32/-mips16 would test
> what I was after.

Sigh.  Obviously not my day.  I just remembered that the default
mips-elf simulator won't accept -mips32 instructions, so you need
to play some tricks.  Could you just try some of the spot checks
instead?  I'll test GNU/Linux on qemu once everything is in.

Richard
Bernd Schmidt - Sept. 1, 2011, 7:23 p.m.
On 09/01/11 21:19, Richard Sandiford wrote:
> Richard Sandiford <rdsandiford@googlemail.com> writes:
>> Gah, my bad, sorry.  I'd forgotten mipsisa32-elf is EABI32
>> rather than o32.  mips-elf with -mips32/-mips16 would test
>> what I was after.
> 
> Sigh.  Obviously not my day.  I just remembered that the default
> mips-elf simulator won't accept -mips32 instructions, so you need
> to play some tricks.  Could you just try some of the spot checks
> instead?  I'll test GNU/Linux on qemu once everything is in.

I seem to have made some progress getting 4.6 mipsisa32-elf to accept
"-mips16 -msoft-float", now with only 287 unexpected failures. The
-mips32 case shouldn't be too different from mips64 testing, should it?


Bernd
Richard Sandiford - Sept. 1, 2011, 7:36 p.m.
Bernd Schmidt <bernds@codesourcery.com> writes:
> On 09/01/11 21:19, Richard Sandiford wrote:
>> Richard Sandiford <rdsandiford@googlemail.com> writes:
>>> Gah, my bad, sorry.  I'd forgotten mipsisa32-elf is EABI32
>>> rather than o32.  mips-elf with -mips32/-mips16 would test
>>> what I was after.
>> 
>> Sigh.  Obviously not my day.  I just remembered that the default
>> mips-elf simulator won't accept -mips32 instructions, so you need
>> to play some tricks.  Could you just try some of the spot checks
>> instead?  I'll test GNU/Linux on qemu once everything is in.
>
> I seem to have made some progress getting 4.6 mipsisa32-elf to accept
> "-mips16 -msoft-float", now with only 287 unexpected failures.

Unfortunately it'll be the wrong ABI though.  SAVE and RESTORE
are very much geared to o32 and won't be used for EABI32.
mipsisa32-elfoabi would be OK though.

Sorry again for messing this up...

Richard
Richard Henderson - Sept. 5, 2011, 1:36 a.m.
On 09/01/2011 03:07 AM, Bernd Schmidt wrote:
> On 08/31/11 20:43, Richard Sandiford wrote:
>> Bernd Schmidt <bernds@codesourcery.com> writes:
>>> This is necessary when adding shrink-wrapping; otherwise dwarf2cfi sees
>>> inconsistent information and aborts.
>>>
>>> Tested on mips64-elf together with the rest of the shrink-wrapping
>>> patches. Ok?
>>
>> It looks like the current code doesn't handle the RESTORE instruction.
>> Could you also test that somehow?  A mipsisa32-elf run with -mips16
>> ought to work, but some sort of spot-checking of shrink-wrapping +
>> RESTORE would be fine if that's easier.
> 
> Will look into that. You mean the mips16e_build_save_restore function?
> 
>> Also, for the frame_pointer_required case, it looks like there's a
>> window between the restoration of the frame pointer and the deallocation
>> of the stack in which the CFA is still defined in terms of the frame
>> pointer register.  Is that significant?  If not (e.g. because we
>> should never need to unwind at that point) then why do we still
>> update the CFA here:
> 
> CC'ing rth, as I'm still not terribly familiar with these issues. I've
> tried to follow alpha.c, which seems to ignore this issue.

Alpha isn't a great example here, because it hasn't been updated to
generate unwind info for epilogues at all.

At the point you restore the frame pointer, you should add a 
REG_{DEF,ADJUST}_CFA note.


r~
Richard Henderson - Sept. 5, 2011, 1:50 a.m.
On 09/01/2011 12:13 AM, Richard Sandiford wrote:
> Also, for the frame_pointer_required case, it looks like there's a
> window between the restoration of the frame pointer and the deallocation
> of the stack in which the CFA is still defined in terms of the frame
> pointer register.  Is that significant?  If not (e.g. because we
> should never need to unwind at that point) then why do we still
> update the CFA here:

What window are you referring to?

Best I can tell from looking at the patch we restore the CFA to the
stack pointer before anything else.

The patch looks right to me.


r~
Richard Sandiford - Sept. 5, 2011, 8:06 a.m.
Richard Henderson <rth@redhat.com> writes:
> On 09/01/2011 12:13 AM, Richard Sandiford wrote:
>> Also, for the frame_pointer_required case, it looks like there's a
>> window between the restoration of the frame pointer and the deallocation
>> of the stack in which the CFA is still defined in terms of the frame
>> pointer register.  Is that significant?  If not (e.g. because we
>> should never need to unwind at that point) then why do we still
>> update the CFA here:
>
> What window are you referring to?
>
> Best I can tell from looking at the patch we restore the CFA to the
> stack pointer before anything else.

Well, I'm probably showing my ignorance here, but what I meant was:
we attach the DEF_CFA and the CFA_RESTORE notes to the final stack
deallocation.  I was just worried that, immediately before that
deallocation, the CFA is still defined in terms of the frame pointer,
even though the frame pointer has already been restored.  So it seemed
like someone trying to unwind at that point would get the wrong CFA.

So I just wasn't sure whether we expected anyone to try to unwind
at that point or not.  Or whether I'm missing something else...

Richard
Richard Henderson - Sept. 6, 2011, 3:30 a.m.
On 09/05/2011 01:36 PM, Richard Sandiford wrote:
> Richard Henderson <rth@redhat.com> writes:
>> On 09/01/2011 12:13 AM, Richard Sandiford wrote:
>>> Also, for the frame_pointer_required case, it looks like there's a
>>> window between the restoration of the frame pointer and the deallocation
>>> of the stack in which the CFA is still defined in terms of the frame
>>> pointer register.  Is that significant?  If not (e.g. because we
>>> should never need to unwind at that point) then why do we still
>>> update the CFA here:
>>
>> What window are you referring to?
>>
>> Best I can tell from looking at the patch we restore the CFA to the
>> stack pointer before anything else.
> 
> Well, I'm probably showing my ignorance here, but what I meant was:
> we attach the DEF_CFA and the CFA_RESTORE notes to the final stack
> deallocation.  I was just worried that, immediately before that
> deallocation, the CFA is still defined in terms of the frame pointer,
> even though the frame pointer has already been restored.  So it seemed
> like someone trying to unwind at that point would get the wrong CFA.

I don't see that.  I'm pasting from mainline but not the patch, but here:

>   /* Copy TARGET into the stack pointer.  */
>   if (target != stack_pointer_rtx)
>     mips_emit_move (stack_pointer_rtx, target);

We restore the stack pointer from the frame pointer before restoring
the frame pointer.  The patch changes this spot with DEF_CFA.  So the
problem you're suggesting ought not be true.


r~
Richard Sandiford - Sept. 6, 2011, 8:05 a.m.
Richard Henderson <rth@redhat.com> writes:
> On 09/05/2011 01:36 PM, Richard Sandiford wrote:
>> Richard Henderson <rth@redhat.com> writes:
>>> On 09/01/2011 12:13 AM, Richard Sandiford wrote:
>>>> Also, for the frame_pointer_required case, it looks like there's a
>>>> window between the restoration of the frame pointer and the deallocation
>>>> of the stack in which the CFA is still defined in terms of the frame
>>>> pointer register.  Is that significant?  If not (e.g. because we
>>>> should never need to unwind at that point) then why do we still
>>>> update the CFA here:
>>>
>>> What window are you referring to?
>>>
>>> Best I can tell from looking at the patch we restore the CFA to the
>>> stack pointer before anything else.
>> 
>> Well, I'm probably showing my ignorance here, but what I meant was:
>> we attach the DEF_CFA and the CFA_RESTORE notes to the final stack
>> deallocation.  I was just worried that, immediately before that
>> deallocation, the CFA is still defined in terms of the frame pointer,
>> even though the frame pointer has already been restored.  So it seemed
>> like someone trying to unwind at that point would get the wrong CFA.
>
> I don't see that.  I'm pasting from mainline but not the patch, but here:
>
>>   /* Copy TARGET into the stack pointer.  */
>>   if (target != stack_pointer_rtx)
>>     mips_emit_move (stack_pointer_rtx, target);
>
> We restore the stack pointer from the frame pointer before restoring
> the frame pointer.  The patch changes this spot with DEF_CFA.  So the
> problem you're suggesting ought not be true.

But that DEF_CFA is conditional on !frame_pointer_needed:

   /* Copy TARGET into the stack pointer.  */
   if (target != stack_pointer_rtx)
-    mips_emit_move (stack_pointer_rtx, target);
+    {
+      insn = mips_emit_move (stack_pointer_rtx, target);
+      if (!frame_pointer_needed)
+	{
+	  add_reg_note (insn, REG_CFA_DEF_CFA,
+			plus_constant (stack_pointer_rtx, step2));
+	  RTX_FRAME_RELATED_P (insn) = 1;
+	}
+    }

So I think this DEF_CFA is only added if the CFA is already defined in
terms of the stack pointer.  I don't think it triggers if the CFA is
currently defined in terms of the frame pointer (which is the case
where that window might occur).  When I asked Bernd about that, he said:

>> ?  If the window is significant, could we avoid it by removing the
>> !frame_pointer_needed checks from the code above?
>
> That causes aborts due to inconsistent information in dwarf2cfi, since
> we can reach the same instruction with either fp or sp as the CFA register.

Richard

Patch

Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	(revision 178135)
+++ gcc/config/mips/mips.c	(working copy)
@@ -10227,7 +10227,10 @@  mips_expand_prologue (void)
     emit_insn (gen_blockage ());
 }
 
-/* Emit instructions to restore register REG from slot MEM.  */
+static rtx cfa_restores = NULL_RTX;
+
+/* Emit instructions to restore register REG from slot MEM.  Also update
+   the cfa_restores list.  */
 
 static void
 mips_restore_reg (rtx reg, rtx mem)
@@ -10238,6 +10241,7 @@  mips_restore_reg (rtx reg, rtx mem)
     reg = gen_rtx_REG (GET_MODE (reg), GP_REG_FIRST + 7);
 
   mips_emit_save_slot_move (reg, mem, MIPS_EPILOGUE_TEMP (GET_MODE (reg)));
+  cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
 }
 
 /* Emit any instructions needed before a return.  */
@@ -10268,6 +10272,8 @@  mips_expand_epilogue (bool sibcall_p)
   HOST_WIDE_INT step1, step2;
   rtx base, target, insn;
 
+  cfa_restores = NULL_RTX;
+
   if (!sibcall_p && mips_can_use_return_insn ())
     {
       emit_jump_insn (gen_return ());
@@ -10324,12 +10330,26 @@  mips_expand_epilogue (bool sibcall_p)
       if (!TARGET_MIPS16)
 	target = stack_pointer_rtx;
 
-      emit_insn (gen_add3_insn (target, base, adjust));
+      insn = emit_insn (gen_add3_insn (target, base, adjust));
+      if (!frame_pointer_needed && target == stack_pointer_rtx)
+	{
+	  RTX_FRAME_RELATED_P (insn) = 1;
+	  add_reg_note (insn, REG_CFA_DEF_CFA,
+			plus_constant (stack_pointer_rtx, step2));
+	}
     }
 
   /* Copy TARGET into the stack pointer.  */
   if (target != stack_pointer_rtx)
-    mips_emit_move (stack_pointer_rtx, target);
+    {
+      insn = mips_emit_move (stack_pointer_rtx, target);
+      if (!frame_pointer_needed)
+	{
+	  add_reg_note (insn, REG_CFA_DEF_CFA,
+			plus_constant (stack_pointer_rtx, step2));
+	  RTX_FRAME_RELATED_P (insn) = 1;
+	}
+    }
 
   /* If we're using addressing macros, $gp is implicitly used by all
      SYMBOL_REFs.  We must emit a blockage insn before restoring $gp
@@ -10393,9 +10413,14 @@  mips_expand_epilogue (bool sibcall_p)
 
 	  /* If we don't use shoadow register set, we need to update SP.  */
 	  if (!cfun->machine->use_shadow_register_set_p && step2 > 0)
-	    emit_insn (gen_add3_insn (stack_pointer_rtx,
-				      stack_pointer_rtx,
-				      GEN_INT (step2)));
+	    {
+	      insn = emit_insn (gen_add3_insn (stack_pointer_rtx,
+					       stack_pointer_rtx,
+					       GEN_INT (step2)));
+	      REG_NOTES (insn) = cfa_restores;
+	      RTX_FRAME_RELATED_P (insn) = 1;
+	      add_reg_note (insn, REG_CFA_DEF_CFA, stack_pointer_rtx);
+	    }
 
 	  /* Move to COP0 Status.  */
 	  emit_insn (gen_cop0_move (gen_rtx_REG (SImode, COP0_STATUS_REG_NUM),
@@ -10405,9 +10430,14 @@  mips_expand_epilogue (bool sibcall_p)
 	{
 	  /* Deallocate the final bit of the frame.  */
 	  if (step2 > 0)
-	    emit_insn (gen_add3_insn (stack_pointer_rtx,
-				      stack_pointer_rtx,
-				      GEN_INT (step2)));
+	    {
+	      insn = emit_insn (gen_add3_insn (stack_pointer_rtx,
+					       stack_pointer_rtx,
+					       GEN_INT (step2)));
+	      REG_NOTES (insn) = cfa_restores;
+	      RTX_FRAME_RELATED_P (insn) = 1;	
+	      add_reg_note (insn, REG_CFA_DEF_CFA, stack_pointer_rtx);
+	    }
 	}
     }