Add unwind information to mips epilogues

Add unwind information to mips epilogues

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 199 bytes --]

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


[-- Attachment #2: mipsep.diff --]
[-- Type: text/plain, Size: 3528 bytes --]

	* 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.

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 ());
 }
 \f
-/* 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);
+	    }
 	}
     }
 

Re: Add unwind information to mips epilogues

[Richard Sandiford]

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

Re: Add unwind information to mips epilogues

[Richard Sandiford]

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

Re: Add unwind information to mips epilogues

[Richard Henderson]

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~

Re: Add unwind information to mips epilogues

[Bernd Schmidt]

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

Re: Add unwind information to mips epilogues

[Richard Sandiford]

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

Re: Add unwind information to mips epilogues

[Richard Sandiford]

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

Re: Add unwind information to mips epilogues

[Bernd Schmidt]

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

Re: Add unwind information to mips epilogues

[Richard Sandiford]

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

Re: Add unwind information to mips epilogues

[Richard Henderson]

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~

Re: Add unwind information to mips epilogues

[Richard Sandiford]

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

Re: Add unwind information to mips epilogues

[Richard Henderson]

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~

Re: Add unwind information to mips epilogues

[Richard Sandiford]

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

Re: Add unwind information to mips epilogues

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

Here's a new version, which adds support for mips16 and tries to avoid
the window with the frame pointer restore.

Testing mips16 is problematic, all the execute tests fail before and
after - I interpret one of your earlier mails to say that this is
expected. There are no new compilation failures with this patch, but
incorrect earlier versions triggered a few, indicating that the testing
is at least somewhat useful.

I get the following output for restore insns:

        move    $sp,$17
        restore 8,$16,$17
$LCFI2 = .
        .cfi_remember_state
        .cfi_restore 16
        .cfi_restore 17
        .cfi_def_cfa 29, 0

which I think is correct.

Question for Richard H.: What is this actually good for, other than
presenting consistent information to dwarf2cfi? Do we actually expect
code to unwind through the middle of an epilogue?


Bernd

[-- Attachment #2: mipsep0907.diff --]
[-- Type: text/plain, Size: 6951 bytes --]

	* config/mips/mips.c (cfa_restores, cfa_sp_offset): New static
	variables.
	(mips16e_save_restore_reg): Add to cfa_restores.
	(mips_restore_reg): Add to cfa_restores, and add a REG_CFA_DEF_CFA
	note to insns restoring the frame pointer.
	(mips_expand_epilogue): Initialize the new variables.  Annotate RTL
	to ensure dwarf2cfi sees the effects of the epilogue.  Generate a
	simple_return only if the return address is in r31.

Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	(revision 178135)
+++ gcc/config/mips/mips.c	(working copy)
@@ -501,6 +501,11 @@ int sdb_label_count;
 int mips_dbx_regno[FIRST_PSEUDO_REGISTER];
 int mips_dwarf_regno[FIRST_PSEUDO_REGISTER];
 
+/* Used to collect REG_CFA notes during epilogue generation.  */
+static rtx cfa_restores = NULL_RTX;
+/* Used to produce REG_CFA_DEF_CFA notes during epilogue generation.  */
+static HOST_WIDE_INT cfa_sp_offset;
+
 /* The nesting depth of the PRINT_OPERAND '%(', '%<' and '%[' constructs.  */
 struct mips_asm_switch mips_noreorder = { "reorder", 0 };
 struct mips_asm_switch mips_nomacro = { "macro", 0 };
@@ -8371,8 +8376,8 @@ mips16e_collect_argument_saves (void)
 }
 
 /* Return a move between register REGNO and memory location SP + OFFSET.
-   Make the move a load if RESTORE_P, otherwise make it a frame-related
-   store.  */
+   Make the move a load and update cfa_restores if RESTORE_P, otherwise
+   make it a frame-related store.  */
 
 static rtx
 mips16e_save_restore_reg (bool restore_p, HOST_WIDE_INT offset,
@@ -8382,6 +8387,9 @@ mips16e_save_restore_reg (bool restore_p
 
   mem = gen_frame_mem (SImode, plus_constant (stack_pointer_rtx, offset));
   reg = gen_rtx_REG (SImode, regno);
+  if (restore_p)
+    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
+
   return (restore_p
 	  ? gen_rtx_SET (VOIDmode, reg, mem)
 	  : mips_frame_set (mem, reg));
@@ -10227,17 +10236,30 @@ mips_expand_prologue (void)
     emit_insn (gen_blockage ());
 }
 \f
-/* Emit instructions to restore register REG from slot MEM.  */
+/* Emit instructions to restore register REG from slot MEM.  Also update
+   the cfa_restores list.  */
 
 static void
 mips_restore_reg (rtx reg, rtx mem)
 {
+  rtx orig_reg = reg;
+  rtx last;
+
   /* There's no MIPS16 instruction to load $31 directly.  Load into
      $7 instead and adjust the return insn appropriately.  */
   if (TARGET_MIPS16 && REGNO (reg) == RETURN_ADDR_REGNUM)
     reg = gen_rtx_REG (GET_MODE (reg), GP_REG_FIRST + 7);
 
   mips_emit_save_slot_move (reg, mem, MIPS_EPILOGUE_TEMP (GET_MODE (reg)));
+  if (reg == orig_reg)
+    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
+
+  if (!frame_pointer_needed || REGNO (reg) != HARD_FRAME_POINTER_REGNUM)
+    return;
+  last = get_last_insn ();
+  add_reg_note (last, REG_CFA_DEF_CFA, plus_constant (stack_pointer_rtx,
+						      cfa_sp_offset));
+  RTX_FRAME_RELATED_P (last) = 1;
 }
 
 /* Emit any instructions needed before a return.  */
@@ -10268,6 +10290,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 ());
@@ -10306,6 +10330,8 @@ mips_expand_epilogue (bool sibcall_p)
       step1 -= step2;
     }
 
+  cfa_sp_offset = step2;
+
   /* Set TARGET to BASE + STEP1.  */
   target = base;
   if (step1 > 0)
@@ -10324,12 +10350,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
@@ -10357,7 +10397,10 @@ mips_expand_epilogue (bool sibcall_p)
 
       /* Restore the remaining registers and deallocate the final bit
 	 of the frame.  */
-      emit_insn (restore);
+      restore = emit_insn (restore);
+      REG_NOTES (restore) = cfa_restores;
+      RTX_FRAME_RELATED_P (restore) = 1;
+      add_reg_note (restore, REG_CFA_DEF_CFA, stack_pointer_rtx);
     }
   else
     {
@@ -10393,9 +10436,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 +10453,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);
+	    }
 	}
     }
 
@@ -10442,7 +10495,7 @@ mips_expand_epilogue (bool sibcall_p)
 	}
       else
 	{
-	  unsigned int regno;
+	  rtx pat;
 
 	  /* When generating MIPS16 code, the normal
 	     mips_for_each_saved_gpr_and_fpr path will restore the return
@@ -10450,11 +10503,16 @@ mips_expand_epilogue (bool sibcall_p)
 	  if (TARGET_MIPS16
 	      && !GENERATE_MIPS16E_SAVE_RESTORE
 	      && BITSET_P (frame->mask, RETURN_ADDR_REGNUM))
-	    regno = GP_REG_FIRST + 7;
+	    {
+	      rtx reg = gen_rtx_REG (Pmode, GP_REG_FIRST + 7);
+	      pat = gen_return_internal (reg);
+	    }
 	  else
-	    regno = RETURN_ADDR_REGNUM;
-	  emit_jump_insn (gen_simple_return_internal (gen_rtx_REG (Pmode,
-								   regno)));
+	    {
+	      rtx reg = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
+	      pat = gen_simple_return_internal (reg);
+	    }
+	  emit_jump_insn (pat);
 	}
     }
 

Re: Add unwind information to mips epilogues

[Richard Guenther]

On Wed, Sep 7, 2011 at 12:28 PM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> Here's a new version, which adds support for mips16 and tries to avoid
> the window with the frame pointer restore.
>
> Testing mips16 is problematic, all the execute tests fail before and
> after - I interpret one of your earlier mails to say that this is
> expected. There are no new compilation failures with this patch, but
> incorrect earlier versions triggered a few, indicating that the testing
> is at least somewhat useful.
>
> I get the following output for restore insns:
>
>        move    $sp,$17
>        restore 8,$16,$17
> $LCFI2 = .
>        .cfi_remember_state
>        .cfi_restore 16
>        .cfi_restore 17
>        .cfi_def_cfa 29, 0
>
> which I think is correct.
>
> Question for Richard H.: What is this actually good for, other than
> presenting consistent information to dwarf2cfi? Do we actually expect
> code to unwind through the middle of an epilogue?

With async signals we can at least get interrupted in the middle of an
epilogue.  What you are allowed to do here (throw an exception?
perform manual unwinding? use gdb which unwinds?) is another
question.  ISTR customer requests for this feature at least (which
I think was doing profiling stuff).

Richard.

>
> Bernd
>

Re: Add unwind information to mips epilogues

[Joseph S. Myers]

On Wed, 7 Sep 2011, Richard Guenther wrote:

> With async signals we can at least get interrupted in the middle of an
> epilogue.  What you are allowed to do here (throw an exception?
> perform manual unwinding? use gdb which unwinds?) is another
> question.  ISTR customer requests for this feature at least (which
> I think was doing profiling stuff).

Profiling this way is discussed in Nathan Froyd et al's paper in the 2006 
Summit proceedings.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Add unwind information to mips epilogues

[Jakub Jelinek]

On Wed, Sep 07, 2011 at 12:28:30PM +0200, Bernd Schmidt wrote:
> Question for Richard H.: What is this actually good for, other than
> presenting consistent information to dwarf2cfi? Do we actually expect
> code to unwind through the middle of an epilogue?

With -fasynchronous-unwind-tables and e.g. async cancellation enabled
pthread_signal may be handled anywhere.  There have been several bug reports
e.g. against glibc, which is why Richard implemented the epilogue unwinding
on various targets.  Additionally, if the debugger uses the unwind info,
when you step through the epilogue, you want precise unwind info too.

	Jakub

Re: Add unwind information to mips epilogues

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 317 bytes --]

Testing with the shrink-wrapping patch added reveals a problem with the
mips16 "save" insn: sometimes we store registers that shouldn't be
considered saved registers; we have to clear RTX_FRAME_RELATED_P for
these. Testing in progress with mips-elf, "ips16/arch=mips32r2/abi=32"
and some other multilibs. Ok?


Bernd

[-- Attachment #2: not-related.diff --]
[-- Type: text/plain, Size: 640 bytes --]

	* config/mips/mips.c (mips16e_build_save_restore): Clear
	RTX_FRAME_RELATED_P for argument stores stolen from the first
	block.

Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	(revision 178135)
+++ gcc/config/mips/mips.c	(working copy)
@@ -8448,6 +8456,7 @@ mips16e_build_save_restore (bool restore
       offset = top_offset + i * UNITS_PER_WORD;
       set = mips16e_save_restore_reg (restore_p, offset, GP_ARG_FIRST + i);
       XVECEXP (pattern, 0, n++) = set;
+      RTX_FRAME_RELATED_P (set) = 0;
     }
 
   /* Then fill in the other register moves.  */

Re: Add unwind information to mips epilogues

[Richard Sandiford]

Bernd Schmidt <bernds@codesourcery.com> writes:
> Testing with the shrink-wrapping patch added reveals a problem with the
> mips16 "save" insn: sometimes we store registers that shouldn't be
> considered saved registers; we have to clear RTX_FRAME_RELATED_P for
> these. Testing in progress with mips-elf, "ips16/arch=mips32r2/abi=32"
> and some other multilibs. Ok?
>
>
> Bernd
>
> 	* config/mips/mips.c (mips16e_build_save_restore): Clear
> 	RTX_FRAME_RELATED_P for argument stores stolen from the first
> 	block.
>
> Index: gcc/config/mips/mips.c
> ===================================================================
> --- gcc/config/mips/mips.c	(revision 178135)
> +++ gcc/config/mips/mips.c	(working copy)
> @@ -8448,6 +8456,7 @@ mips16e_build_save_restore (bool restore
>        offset = top_offset + i * UNITS_PER_WORD;
>        set = mips16e_save_restore_reg (restore_p, offset, GP_ARG_FIRST + i);
>        XVECEXP (pattern, 0, n++) = set;
> +      RTX_FRAME_RELATED_P (set) = 0;
>      }
>  
>    /* Then fill in the other register moves.  */

Sorry to be a pain, but I'd prefer the attached, which feels a bit
more direct.  (It's also completely untested, but I'll try to give
it a spin this weekend.)

Richard


gcc/
	* config/mips/mips.c (mips16e_save_restore_reg): Add a reg_parm_p
	argument.
	(mips16e_build_save_restore): Update accordingly.

Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	2011-09-06 12:01:22.000000000 +0100
+++ gcc/config/mips/mips.c	2011-09-08 14:53:22.885218099 +0100
@@ -8365,20 +8365,22 @@ mips16e_collect_argument_saves (void)
 }
 
 /* Return a move between register REGNO and memory location SP + OFFSET.
-   Make the move a load if RESTORE_P, otherwise make it a frame-related
-   store.  */
+   REG_PARM_P is true if SP + OFFSET belongs to REG_PARM_STACK_SPACE.
+   Make the move a load if RESTORE_P, otherwise make it a store.  */
 
 static rtx
-mips16e_save_restore_reg (bool restore_p, HOST_WIDE_INT offset,
-			  unsigned int regno)
+mips16e_save_restore_reg (bool restore_p, bool reg_parm_p,
+			  HOST_WIDE_INT offset, unsigned int regno)
 {
   rtx reg, mem;
 
   mem = gen_frame_mem (SImode, plus_constant (stack_pointer_rtx, offset));
   reg = gen_rtx_REG (SImode, regno);
-  return (restore_p
-	  ? gen_rtx_SET (VOIDmode, reg, mem)
-	  : mips_frame_set (mem, reg));
+  if (restore_p)
+    return gen_rtx_SET (VOIDmode, reg, mem);
+  if (reg_parm_p)
+    return gen_rtx_SET (VOIDmode, mem, reg);
+  return mips_frame_set (mem, reg);
 }
 
 /* Return RTL for a MIPS16e SAVE or RESTORE instruction; RESTORE_P says which.
@@ -8440,7 +8442,8 @@ mips16e_build_save_restore (bool restore
   for (i = 0; i < nargs; i++)
     {
       offset = top_offset + i * UNITS_PER_WORD;
-      set = mips16e_save_restore_reg (restore_p, offset, GP_ARG_FIRST + i);
+      set = mips16e_save_restore_reg (restore_p, true, offset,
+				      GP_ARG_FIRST + i);
       XVECEXP (pattern, 0, n++) = set;
     }
 
@@ -8452,7 +8455,7 @@ mips16e_build_save_restore (bool restore
       if (BITSET_P (*mask_ptr, regno))
 	{
 	  offset -= UNITS_PER_WORD;
-	  set = mips16e_save_restore_reg (restore_p, offset, regno);
+	  set = mips16e_save_restore_reg (restore_p, false, offset, regno);
 	  XVECEXP (pattern, 0, n++) = set;
 	  *mask_ptr &= ~(1 << regno);
 	}

Re: Add unwind information to mips epilogues

[Richard Sandiford]

Bernd Schmidt <bernds@codesourcery.com> writes:
> @@ -10227,17 +10236,30 @@ mips_expand_prologue (void)
>      emit_insn (gen_blockage ());
>  }
>  \f
> -/* Emit instructions to restore register REG from slot MEM.  */
> +/* Emit instructions to restore register REG from slot MEM.  Also update
> +   the cfa_restores list.  */
>  
>  static void
>  mips_restore_reg (rtx reg, rtx mem)
>  {
> +  rtx orig_reg = reg;
> +  rtx last;
> +
>    /* There's no MIPS16 instruction to load $31 directly.  Load into
>       $7 instead and adjust the return insn appropriately.  */
>    if (TARGET_MIPS16 && REGNO (reg) == RETURN_ADDR_REGNUM)
>      reg = gen_rtx_REG (GET_MODE (reg), GP_REG_FIRST + 7);
>  
>    mips_emit_save_slot_move (reg, mem, MIPS_EPILOGUE_TEMP (GET_MODE (reg)));
> +  if (reg == orig_reg)
> +    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
> +
> +  if (!frame_pointer_needed || REGNO (reg) != HARD_FRAME_POINTER_REGNUM)
> +    return;
> +  last = get_last_insn ();
> +  add_reg_note (last, REG_CFA_DEF_CFA, plus_constant (stack_pointer_rtx,
> +						      cfa_sp_offset));
> +  RTX_FRAME_RELATED_P (last) = 1;

I suppose I still don't get why this is OK but this:

> @@ -10324,12 +10350,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));
> +	}

triggered ICEs without the !frame_pointer_required.  I think I need
to play around with it a bit before I understand enough to review.
I'll try to find time this weekend.

Also, this:

@@ -10442,7 +10495,7 @@ mips_expand_epilogue (bool sibcall_p)
 	}
       else
 	{
-	  unsigned int regno;
+	  rtx pat;
 
 	  /* When generating MIPS16 code, the normal
 	     mips_for_each_saved_gpr_and_fpr path will restore the return
@@ -10450,11 +10503,16 @@ mips_expand_epilogue (bool sibcall_p)
 	  if (TARGET_MIPS16
 	      && !GENERATE_MIPS16E_SAVE_RESTORE
 	      && BITSET_P (frame->mask, RETURN_ADDR_REGNUM))
-	    regno = GP_REG_FIRST + 7;
+	    {
+	      rtx reg = gen_rtx_REG (Pmode, GP_REG_FIRST + 7);
+	      pat = gen_return_internal (reg);
+	    }
 	  else
-	    regno = RETURN_ADDR_REGNUM;
-	  emit_jump_insn (gen_simple_return_internal (gen_rtx_REG (Pmode,
-								   regno)));
+	    {
+	      rtx reg = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
+	      pat = gen_simple_return_internal (reg);
+	    }
+	  emit_jump_insn (pat);
 	}
     }
 
looks like a logically separate change.  I'm not sure I understand
why it's needed: both returns are simple returns in the rtx sense.

Sorry for being awkward here :-(

Richard

Re: Add unwind information to mips epilogues

[Bernd Schmidt]

On 09/08/11 16:08, Richard Sandiford wrote:
> Also, this:
> 
> @@ -10442,7 +10495,7 @@ mips_expand_epilogue (bool sibcall_p)
>  	}
>        else
>  	{
> -	  unsigned int regno;
> +	  rtx pat;
>  
>  	  /* When generating MIPS16 code, the normal
>  	     mips_for_each_saved_gpr_and_fpr path will restore the return
> @@ -10450,11 +10503,16 @@ mips_expand_epilogue (bool sibcall_p)
>  	  if (TARGET_MIPS16
>  	      && !GENERATE_MIPS16E_SAVE_RESTORE
>  	      && BITSET_P (frame->mask, RETURN_ADDR_REGNUM))
> -	    regno = GP_REG_FIRST + 7;
> +	    {
> +	      rtx reg = gen_rtx_REG (Pmode, GP_REG_FIRST + 7);
> +	      pat = gen_return_internal (reg);
> +	    }
>  	  else
> -	    regno = RETURN_ADDR_REGNUM;
> -	  emit_jump_insn (gen_simple_return_internal (gen_rtx_REG (Pmode,
> -								   regno)));
> +	    {
> +	      rtx reg = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
> +	      pat = gen_simple_return_internal (reg);
> +	    }
> +	  emit_jump_insn (pat);
>  	}
>      }
>  
> looks like a logically separate change.  I'm not sure I understand
> why it's needed: both returns are simple returns in the rtx sense.

Should have explained that bit - it's needed only with shrink-wrapping.
If we find that the epilogue ends in a simple_return, and we need
conditional simple_returns elsewhere and don't have a pattern for them,
we try to reuse the epilogue's simple_return by placing a label in
front of it and redirecting these conditional simple_returns there. This
doesn't work if we need to use different return registers, and a simple
way to prevent it is to make the last insn look like a normal return
instead.


Bernd

Re: Add unwind information to mips epilogues

[Richard Sandiford]

Bernd Schmidt <bernds@codesourcery.com> writes:
> On 09/08/11 16:08, Richard Sandiford wrote:
>> Also, this:
>> 
>> @@ -10442,7 +10495,7 @@ mips_expand_epilogue (bool sibcall_p)
>>  	}
>>        else
>>  	{
>> -	  unsigned int regno;
>> +	  rtx pat;
>>  
>>  	  /* When generating MIPS16 code, the normal
>>  	     mips_for_each_saved_gpr_and_fpr path will restore the return
>> @@ -10450,11 +10503,16 @@ mips_expand_epilogue (bool sibcall_p)
>>  	  if (TARGET_MIPS16
>>  	      && !GENERATE_MIPS16E_SAVE_RESTORE
>>  	      && BITSET_P (frame->mask, RETURN_ADDR_REGNUM))
>> -	    regno = GP_REG_FIRST + 7;
>> +	    {
>> +	      rtx reg = gen_rtx_REG (Pmode, GP_REG_FIRST + 7);
>> +	      pat = gen_return_internal (reg);
>> +	    }
>>  	  else
>> -	    regno = RETURN_ADDR_REGNUM;
>> -	  emit_jump_insn (gen_simple_return_internal (gen_rtx_REG (Pmode,
>> -								   regno)));
>> +	    {
>> +	      rtx reg = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
>> +	      pat = gen_simple_return_internal (reg);
>> +	    }
>> +	  emit_jump_insn (pat);
>>  	}
>>      }
>>  
>> looks like a logically separate change.  I'm not sure I understand
>> why it's needed: both returns are simple returns in the rtx sense.
>
> Should have explained that bit - it's needed only with shrink-wrapping.
> If we find that the epilogue ends in a simple_return, and we need
> conditional simple_returns elsewhere and don't have a pattern for them,
> we try to reuse the epilogue's simple_return by placing a label in
> front of it and redirecting these conditional simple_returns there. This
> doesn't work if we need to use different return registers, and a simple
> way to prevent it is to make the last insn look like a normal return
> instead.

Ah!  Yeah.  So this part is OK on its own with a comment like:

	      /* simple_returns cannot rely on values that are only available
		 on paths through the epilogue (because return paths that do
		 not pass through the epilogue may nevertheless reuse a
		 simple_return that occurs at the end of the epilogue).
		 Use a normal return here instead.  */
	      rtx reg = gen_rtx_REG (Pmode, GP_REG_FIRST + 7);
	      pat = gen_return_internal (reg);

Thanks,
Richard

Re: Add unwind information to mips epilogues

[Richard Henderson]

On 09/08/2011 03:08 PM, Richard Sandiford wrote:
> Bernd Schmidt <bernds@codesourcery.com> writes:
>> @@ -10227,17 +10236,30 @@ mips_expand_prologue (void)
>>      emit_insn (gen_blockage ());
>>  }
>>  \f
>> -/* Emit instructions to restore register REG from slot MEM.  */
>> +/* Emit instructions to restore register REG from slot MEM.  Also update
>> +   the cfa_restores list.  */
>>  
>>  static void
>>  mips_restore_reg (rtx reg, rtx mem)
>>  {
>> +  rtx orig_reg = reg;
>> +  rtx last;
>> +
>>    /* There's no MIPS16 instruction to load $31 directly.  Load into
>>       $7 instead and adjust the return insn appropriately.  */
>>    if (TARGET_MIPS16 && REGNO (reg) == RETURN_ADDR_REGNUM)
>>      reg = gen_rtx_REG (GET_MODE (reg), GP_REG_FIRST + 7);
>>  
>>    mips_emit_save_slot_move (reg, mem, MIPS_EPILOGUE_TEMP (GET_MODE (reg)));
>> +  if (reg == orig_reg)
>> +    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
>> +
>> +  if (!frame_pointer_needed || REGNO (reg) != HARD_FRAME_POINTER_REGNUM)
>> +    return;
>> +  last = get_last_insn ();
>> +  add_reg_note (last, REG_CFA_DEF_CFA, plus_constant (stack_pointer_rtx,
>> +						      cfa_sp_offset));
>> +  RTX_FRAME_RELATED_P (last) = 1;
> 
> I suppose I still don't get why this is OK but this:
> 
>> @@ -10324,12 +10350,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));
>> +	}
> 
> triggered ICEs without the !frame_pointer_required.  I think I need
> to play around with it a bit before I understand enough to review.
> I'll try to find time this weekend.

I'd be curious to understand in what situation the second hunk causes
failures without frame_pointer_required.  But as I'm supposed to be on
holiday atm I can't spend any real time on it til I get back.  ;-)

That said, the first hunk causes the CFA to get swapped back to the
stack pointer on the very insn that restores the frame pointer.

Which to me looks to do the right thing in all situations.  Is there
a different window you are worried about?


r~

Re: Add unwind information to mips epilogues

[Richard Sandiford]

Richard Henderson <rth@redhat.com> writes:
> On 09/08/2011 03:08 PM, Richard Sandiford wrote:
>> Bernd Schmidt <bernds@codesourcery.com> writes:
>>> @@ -10227,17 +10236,30 @@ mips_expand_prologue (void)
>>>      emit_insn (gen_blockage ());
>>>  }
>>>  \f
>>> -/* Emit instructions to restore register REG from slot MEM.  */
>>> +/* Emit instructions to restore register REG from slot MEM.  Also update
>>> +   the cfa_restores list.  */
>>>  
>>>  static void
>>>  mips_restore_reg (rtx reg, rtx mem)
>>>  {
>>> +  rtx orig_reg = reg;
>>> +  rtx last;
>>> +
>>>    /* There's no MIPS16 instruction to load $31 directly.  Load into
>>>       $7 instead and adjust the return insn appropriately.  */
>>>    if (TARGET_MIPS16 && REGNO (reg) == RETURN_ADDR_REGNUM)
>>>      reg = gen_rtx_REG (GET_MODE (reg), GP_REG_FIRST + 7);
>>>  
>>>    mips_emit_save_slot_move (reg, mem, MIPS_EPILOGUE_TEMP (GET_MODE (reg)));
>>> +  if (reg == orig_reg)
>>> +    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
>>> +
>>> +  if (!frame_pointer_needed || REGNO (reg) != HARD_FRAME_POINTER_REGNUM)
>>> +    return;
>>> +  last = get_last_insn ();
>>> +  add_reg_note (last, REG_CFA_DEF_CFA, plus_constant (stack_pointer_rtx,
>>> +						      cfa_sp_offset));
>>> +  RTX_FRAME_RELATED_P (last) = 1;
>> 
>> I suppose I still don't get why this is OK but this:
>> 
>>> @@ -10324,12 +10350,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));
>>> +	}
>> 
>> triggered ICEs without the !frame_pointer_required.  I think I need
>> to play around with it a bit before I understand enough to review.
>> I'll try to find time this weekend.
>
> I'd be curious to understand in what situation the second hunk causes
> failures without frame_pointer_required.  But as I'm supposed to be on
> holiday atm I can't spend any real time on it til I get back.  ;-)
>
> That said, the first hunk causes the CFA to get swapped back to the
> stack pointer on the very insn that restores the frame pointer.
>
> Which to me looks to do the right thing in all situations.  Is there
> a different window you are worried about?

Nah, nothing like that.  I agree the new patch is setting the CFA at the
right time.  It's just that if those !frame_pointer_needed tests are
needed to avoid ICEs, then it feels like there's still some magic that
I don't understand.

(I'm not saying the checks should be removed from the new patch --
at best it would achieve nothing, and at worst it would bloat the
dwarf2 output.  I'm just curious.)

Richard

Re: Add unwind information to mips epilogues

[Bernd Schmidt]

On 09/08/11 16:08, Richard Sandiford wrote:
> I suppose I still don't get why this is OK but this:
> 
>> @@ -10324,12 +10350,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));
>> +	}
> 
> triggered ICEs without the !frame_pointer_required.  I think I need
> to play around with it a bit before I understand enough to review.
> I'll try to find time this weekend.

I've rebuilt and tested it again (shrink-wrapping included, not sure if
it makes a difference). What seems to happen is that we have

(insn/f 60 59 61 2 (set (reg/f:SI 30 $fp) (reg/f:SI 29 $sp))

(note 61 60 8 2 NOTE_INSN_PROLOGUE_END)

(jump_insn 8 61 49 2 (set (pc)
        (if_then_else (eq (reg/v:SI 4 $4 [orig:196 b ] [196])
                (reg:SI 2 $2 [198]))
            (label_ref:SI 29) (pc)))

[...]

(jump_insn 10 9 48 3 (set (pc)
        (if_then_else (eq (reg/v:SI 4 $4 [orig:196 b ] [196])
                (reg:SI 2 $2 [199]))
            (label_ref:SI 29) (pc)))

(note 48 10 62 4 [bb 4] NOTE_INSN_BASIC_BLOCK)

(note 62 48 63 4 NOTE_INSN_EPILOGUE_BEG)

(insn/f 63 62 64 4 (set (reg/f:SI 29 $sp) (reg/f:SI 30 $fp))
     (REG_CFA_DEF_CFA (plus:SI (reg/f:SI 29 $sp) (const_int 8))

and reorg.c manages to hoist insn 63 into a delay slot for jump_insn 10.
Presumably it saw insn 60 somehow? In any case, we can now arrive at
label 29 with either FP or SP as CFA reg.

This is gcc.dg/pr49994-1.c.


Bernd

Re: Add unwind information to mips epilogues

[Richard Sandiford]

Bernd Schmidt <bernds@codesourcery.com> writes:
> On 09/08/11 16:08, Richard Sandiford wrote:
>> I suppose I still don't get why this is OK but this:
>> 
>>> @@ -10324,12 +10350,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));
>>> +	}
>> 
>> triggered ICEs without the !frame_pointer_required.  I think I need
>> to play around with it a bit before I understand enough to review.
>> I'll try to find time this weekend.
>
> I've rebuilt and tested it again (shrink-wrapping included, not sure if
> it makes a difference). What seems to happen is that we have
>
> (insn/f 60 59 61 2 (set (reg/f:SI 30 $fp) (reg/f:SI 29 $sp))
>
> (note 61 60 8 2 NOTE_INSN_PROLOGUE_END)
>
> (jump_insn 8 61 49 2 (set (pc)
>         (if_then_else (eq (reg/v:SI 4 $4 [orig:196 b ] [196])
>                 (reg:SI 2 $2 [198]))
>             (label_ref:SI 29) (pc)))
>
> [...]
>
> (jump_insn 10 9 48 3 (set (pc)
>         (if_then_else (eq (reg/v:SI 4 $4 [orig:196 b ] [196])
>                 (reg:SI 2 $2 [199]))
>             (label_ref:SI 29) (pc)))
>
> (note 48 10 62 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
>
> (note 62 48 63 4 NOTE_INSN_EPILOGUE_BEG)
>
> (insn/f 63 62 64 4 (set (reg/f:SI 29 $sp) (reg/f:SI 30 $fp))
>      (REG_CFA_DEF_CFA (plus:SI (reg/f:SI 29 $sp) (const_int 8))
>
> and reorg.c manages to hoist insn 63 into a delay slot for jump_insn 10.
> Presumably it saw insn 60 somehow? In any case, we can now arrive at
> label 29 with either FP or SP as CFA reg.
>
> This is gcc.dg/pr49994-1.c.

Thanks.  I suppose the transformation would be valid if it really had
seen insn 60, but it looks like:

(insn 39 38 40 (set (reg/f:SI 29 $sp)
        (mem:SI (plus:SI (reg/f:SI 15 $15 [orig:197 CHAIN.1 ] [197])
                (const_int 4 [0x4])) [0 S4 A32])) ../../../gcc/gcc/testsuite/gcc.dg/pr49994-1.c:14 278 {*movsi_internal}
     (expr_list:REG_DEAD (reg/f:SI 15 $15 [orig:197 CHAIN.1 ] [197])
        (nil)))

stops resource.c from considering the stack pointer to be live on
fallthrough, and so reorg.c is simply speculating a move to what
it thinks is a dead register.  Which seems like a bug.

(It's probably safe in this example, but we haven't done the work to
prove that.  In particular, we haven't proven that the MEM in insn 39
isn't an access to the current frame.)

But like you say, reorg.c does analyse register contents to some extent,
and it's hard to prove that it wouldn't be able to pull the same trick
for valid reasons.  Should we simply forbid speculation of frame-related
instructions?  E.g. by replacing the may_trap_or_fault_p calls with something
that also checks RTX_FRAME_RELATED_P?  (We'd still be able to use frame-
related insns for annulled and always-true branches.)  Or is that too
big a hammer?

Richard

Re: Add unwind information to mips epilogues

[Richard Sandiford]

Richard Sandiford <richard.sandiford@linaro.org> writes:
> I think I need to play around with it a bit before I understand enough
> to review.  I'll try to find time this weekend.

Does the attached patch look OK?  It should fix a couple of things.
First, on MIPS16, this code:

  /* Set TARGET to BASE + STEP1.  */
  target = base;
  if (step1 > 0)
    {
      rtx adjust;

      /* Get an rtx for STEP1 that we can add to BASE.  */
      adjust = GEN_INT (step1);
      if (!SMALL_OPERAND (step1))
	{
	  mips_emit_move (MIPS_EPILOGUE_TEMP (Pmode), adjust);
	  adjust = MIPS_EPILOGUE_TEMP (Pmode);
	}

      /* Normal mode code can copy the result straight into $sp.  */
      if (!TARGET_MIPS16)
	target = stack_pointer_rtx;

      emit_insn (gen_add3_insn (target, base, adjust));
    }

adds STEP1 to the frame pointer, then:

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

moves it to the stack pointer.  So there's a window in which the frame
pointer would have been adjusted, but the CFA is still defined in terms
of the old frame pointer.  I think:

      emit_insn (gen_add3_insn (target, base, adjust));

always needs a DEF_CFA note for TARGET + STEP2.  For MIPS16, this will
redefine the CFA in terms of the new frame pointer, otherwise it will
define the CFA in terms of the stack pointer.  It's not necessary to
do this immediately for frame_pointer_needed && !TARGET_MIPS16, but we'd
eventually emit the same thing when restoring the frame pointer, so we
might as well do it here.

With that change, we can drop the DEF_CFA for:

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

and simply set it when restoring the frame pointer, just as for
non-MIPS16 code.

Second thing is that we never emitted .cfi_restores for interrupt
handlers that use the shadow register set (which don't need to
deallocate the stack).

I tested this with the other shrink-wrapping patches on mips64-linux-gnu
using --with-arch=mips64 (-mabi=32, -mabi=64, -mabi=n32 and -mips16/-mabi=32,
which tests SAVE and RESTORE).  The results were good apart from:

FAIL: gcc.c-torture/compile/iftrap-1.c  -O3 -g  (internal compiler error)
FAIL: gcc.c-torture/compile/iftrap-1.c  -O3 -g  (test for excess errors)

when testing with -mips16/-mabi=32.  Not sure if you've already
seen/mentioned this or not, but just in case: the ICE is coming from:

void f8(int p)
{
  if (p)
    __builtin_trap();
  else
    {
      bar();
      __builtin_trap();
    }
}

When generating prologues and epilogues, there are still two trap
instructions (each with only fake edges to the exit block, since
the trap is unconditional).  We decide to shrink-wrap the "else",
meaning that the two traps have different CFAs.  Then the cfg cleanups
in csa notice that we have two basic blocks that end in traps,
and decides to redirect the "then" branch to the end of the "else".
That's correct execution-wise, but it means we have two incoming edges
with different CFAs.

Richard


gcc/
2011-09-11  Bernd Schmidt  <bernds@codesourcery.com>
	    Richard Sandiford  <rdsandiford@googlemail.com>

	* config/mips/mips.c (mips_epilogue): New structure.
	(mips16e_save_restore_reg): Queue REG_CFA_RESTORE notes when
	restoring registers.
	(mips_epilogue_emit_cfa_restores): New function.
	(mips_epilogue_set_cfa): Likewise.
	(mips_restore_reg): Queue REG_CFA_RESTORE notes.  When restoring
	the current CFA register from the stack, redefine the CFA in terms
	of the stack pointer.
	(mips_expand_epilogue): Set up mips_epilogue.  Attach CFA information
	to the epilogue instructions.

gcc/testsuite/
	* gcc.target/mips/mips.exp (mips_option_groups): Add debug options.
	* gcc.target/mips/interrupt_handler-2.c: New test.
	* gcc.target/mips/interrupt_handler-3.c: Likewise.

Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	2011-09-10 09:30:00.000000000 +0100
+++ gcc/config/mips/mips.c	2011-09-11 09:07:47.000000000 +0100
@@ -501,6 +501,21 @@ const char *current_function_file = "";
 int mips_dbx_regno[FIRST_PSEUDO_REGISTER];
 int mips_dwarf_regno[FIRST_PSEUDO_REGISTER];
 
+/* Information about the current function's epilogue, used only while
+   expanding it.  */
+static struct {
+  /* A list of queued REG_CFA_RESTORE notes.  */
+  rtx cfa_restores;
+
+  /* The CFA is currently defined as CFA_REG + CFA_OFFSET.  */
+  rtx cfa_reg;
+  HOST_WIDE_INT cfa_offset;
+
+  /* The offset of the CFA from the stack pointer while restoring
+     registers.  */
+  HOST_WIDE_INT cfa_restore_sp_offset;
+} mips_epilogue;
+
 /* The nesting depth of the PRINT_OPERAND '%(', '%<' and '%[' constructs.  */
 struct mips_asm_switch mips_noreorder = { "reorder", 0 };
 struct mips_asm_switch mips_nomacro = { "macro", 0 };
@@ -8377,7 +8392,11 @@ mips16e_save_restore_reg (bool restore_p
   mem = gen_frame_mem (SImode, plus_constant (stack_pointer_rtx, offset));
   reg = gen_rtx_REG (SImode, regno);
   if (restore_p)
-    return gen_rtx_SET (VOIDmode, reg, mem);
+    {
+      mips_epilogue.cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg,
+						   mips_epilogue.cfa_restores);
+      return gen_rtx_SET (VOIDmode, reg, mem);
+    }
   if (reg_parm_p)
     return gen_rtx_SET (VOIDmode, mem, reg);
   return mips_frame_set (mem, reg);
@@ -10222,7 +10241,47 @@ mips_expand_prologue (void)
     emit_insn (gen_blockage ());
 }
 \f
-/* Emit instructions to restore register REG from slot MEM.  */
+/* Attach all pending register saves to the previous instruction.
+   Return that instruction.  */
+
+static rtx
+mips_epilogue_emit_cfa_restores (void)
+{
+  rtx insn;
+
+  insn = get_last_insn ();
+  gcc_assert (insn && !REG_NOTES (insn));
+  if (mips_epilogue.cfa_restores)
+    {
+      RTX_FRAME_RELATED_P (insn) = 1;
+      REG_NOTES (insn) = mips_epilogue.cfa_restores;
+      mips_epilogue.cfa_restores = 0;
+    }
+  return insn;
+}
+
+/* Like mips_epilogue_emit_cfa_restores, but also record that the CFA is
+   now at REG + OFFSET.  */
+
+static void
+mips_epilogue_set_cfa (rtx reg, HOST_WIDE_INT offset)
+{
+  rtx insn;
+
+  insn = mips_epilogue_emit_cfa_restores ();
+  if (reg != mips_epilogue.cfa_reg || offset != mips_epilogue.cfa_offset)
+    {
+      RTX_FRAME_RELATED_P (insn) = 1;
+      REG_NOTES (insn) = alloc_reg_note (REG_CFA_DEF_CFA,
+					 plus_constant (reg, offset),
+					 REG_NOTES (insn));
+      mips_epilogue.cfa_reg = reg;
+      mips_epilogue.cfa_offset = offset;
+    }
+}
+
+/* Emit instructions to restore register REG from slot MEM.  Also update
+   the cfa_restores list.  */
 
 static void
 mips_restore_reg (rtx reg, rtx mem)
@@ -10231,8 +10290,17 @@ mips_restore_reg (rtx reg, rtx mem)
      $7 instead and adjust the return insn appropriately.  */
   if (TARGET_MIPS16 && REGNO (reg) == RETURN_ADDR_REGNUM)
     reg = gen_rtx_REG (GET_MODE (reg), GP_REG_FIRST + 7);
+  else
+    mips_epilogue.cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg,
+						 mips_epilogue.cfa_restores);
 
   mips_emit_save_slot_move (reg, mem, MIPS_EPILOGUE_TEMP (GET_MODE (reg)));
+  if (REGNO (reg) == REGNO (mips_epilogue.cfa_reg))
+    /* The CFA is currently defined in terms of the register whose
+       value we have just restored.  Redefine the CFA in terms of
+       the stack pointer.  */
+    mips_epilogue_set_cfa (stack_pointer_rtx,
+			   mips_epilogue.cfa_restore_sp_offset);
 }
 
 /* Emit any instructions needed before a return.  */
@@ -10291,6 +10359,9 @@ mips_expand_epilogue (bool sibcall_p)
       base = hard_frame_pointer_rtx;
       step1 -= frame->hard_frame_pointer_offset;
     }
+  mips_epilogue.cfa_reg = base;
+  mips_epilogue.cfa_offset = step1;
+  mips_epilogue.cfa_restores = NULL_RTX;
 
   /* If we need to restore registers, deallocate as much stack as
      possible in the second step without going out of range.  */
@@ -10320,6 +10391,7 @@ mips_expand_epilogue (bool sibcall_p)
 	target = stack_pointer_rtx;
 
       emit_insn (gen_add3_insn (target, base, adjust));
+      mips_epilogue_set_cfa (target, step2);
     }
 
   /* Copy TARGET into the stack pointer.  */
@@ -10332,6 +10404,7 @@ mips_expand_epilogue (bool sibcall_p)
   if (TARGET_CALL_SAVED_GP && !TARGET_EXPLICIT_RELOCS)
     emit_insn (gen_blockage ());
 
+  mips_epilogue.cfa_restore_sp_offset = step2;
   if (GENERATE_MIPS16E_SAVE_RESTORE && frame->mask != 0)
     {
       unsigned int regno, mask;
@@ -10353,6 +10426,7 @@ mips_expand_epilogue (bool sibcall_p)
       /* Restore the remaining registers and deallocate the final bit
 	 of the frame.  */
       emit_insn (restore);
+      mips_epilogue_set_cfa (stack_pointer_rtx, 0);
     }
   else
     {
@@ -10388,9 +10462,15 @@ 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)));
+	    {
+	      emit_insn (gen_add3_insn (stack_pointer_rtx,
+					stack_pointer_rtx,
+					GEN_INT (step2)));
+	      mips_epilogue_set_cfa (stack_pointer_rtx, 0);
+	    }
+	  else
+	    /* The choice of position is somewhat arbitrary in this case.  */
+	    mips_epilogue_emit_cfa_restores ();
 
 	  /* Move to COP0 Status.  */
 	  emit_insn (gen_cop0_move (gen_rtx_REG (SImode, COP0_STATUS_REG_NUM),
@@ -10400,11 +10480,15 @@ 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)));
+	    {
+	      emit_insn (gen_add3_insn (stack_pointer_rtx,
+					stack_pointer_rtx,
+					GEN_INT (step2)));
+	      mips_epilogue_set_cfa (stack_pointer_rtx, 0);
+	    }
 	}
     }
+  gcc_assert (!mips_epilogue.cfa_restores);
 
   /* Add in the __builtin_eh_return stack adjustment.  We need to
      use a temporary in MIPS16 code.  */
Index: gcc/testsuite/gcc.target/mips/mips.exp
===================================================================
--- gcc/testsuite/gcc.target/mips/mips.exp	2011-09-11 09:19:47.000000000 +0100
+++ gcc/testsuite/gcc.target/mips/mips.exp	2011-09-11 09:19:53.000000000 +0100
@@ -226,6 +226,7 @@ set mips_option_groups {
     abi "-mabi=.*"
     addressing "addressing=.*"
     arch "-mips([1-5]|32.*|64.*)|-march=.*|isa(|_rev)(=|<=|>=).*"
+    debug "-g*"
     dump_pattern "-dp"
     endianness "-E(L|B)|-me(l|b)"
     float "-m(hard|soft)-float"
Index: gcc/testsuite/gcc.target/mips/interrupt_handler-2.c
===================================================================
--- /dev/null	2011-09-11 09:48:11.061662158 +0100
+++ gcc/testsuite/gcc.target/mips/interrupt_handler-2.c	2011-09-11 09:27:18.000000000 +0100
@@ -0,0 +1,14 @@
+/* Make sure that we emit .cfa_restore notes for LO and HI.  */
+/* { dg-options "-mips32r2 -msoft-float -O -g" } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 64\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 65\n" } } */
+/* { dg-final { scan-assembler-not "\\\.cfi_def_cfa( |\t)" } } */
+/* { dg-final { scan-assembler-not "\\\.cfi_def_cfa_register( |\t)" } } */
+
+extern void f (void);
+
+NOMIPS16 void __attribute__ ((interrupt, use_shadow_register_set))
+v1 (void)
+{
+  f ();
+}
Index: gcc/testsuite/gcc.target/mips/interrupt_handler-3.c
===================================================================
--- /dev/null	2011-09-11 09:48:11.061662158 +0100
+++ gcc/testsuite/gcc.target/mips/interrupt_handler-3.c	2011-09-11 09:25:54.000000000 +0100
@@ -0,0 +1,33 @@
+/* Make sure that we emit .cfa_restore notes for LO, HI and GPRs.  */
+/* { dg-options "-mips32r2 -msoft-float -O -g" } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 1\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 2\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 3\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 4\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 5\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 6\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 7\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 8\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 9\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 10\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 11\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 12\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 13\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 14\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 15\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 24\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 25\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 31\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 64\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 65\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_def_cfa_offset 0\n" } } */
+/* { dg-final { scan-assembler-not "\\\.cfi_def_cfa( |\t)" } } */
+/* { dg-final { scan-assembler-not "\\\.cfi_def_cfa_register( |\t)" } } */
+
+extern void f (void);
+
+NOMIPS16 void __attribute__ ((interrupt))
+v1 (void)
+{
+  f ();
+}

Re: Add unwind information to mips epilogues

[Bernd Schmidt]

On 09/11/11 11:03, Richard Sandiford wrote:
> Richard Sandiford <richard.sandiford@linaro.org> writes:
>> I think I need to play around with it a bit before I understand enough
>> to review.  I'll try to find time this weekend.
> 
> Does the attached patch look OK?  It should fix a couple of things.

Sure!


Bernd

Re: Add unwind information to mips epilogues

[Richard Sandiford]

Bernd Schmidt <bernds@codesourcery.com> writes:
> On 09/11/11 11:03, Richard Sandiford wrote:
>> Richard Sandiford <richard.sandiford@linaro.org> writes:
>>> I think I need to play around with it a bit before I understand enough
>>> to review.  I'll try to find time this weekend.
>> 
>> Does the attached patch look OK?  It should fix a couple of things.
>
> Sure!

Thanks, applied, along with the fix for mips16e argument saves.

Richard