[MIPS, committed] Add CFI information to mips16 fpret stubs

[Richard Sandiford <rdsandiford at googlemail dot com>]

Richard Henderson <rth@redhat.com> writes:
> On 02/16/2012 10:58 AM, Richard Sandiford wrote:
>>> As a workaround for 4.7, you can try this hack:
>>>
>>> 	.cfi_startproc simple
>>> 	.cfi_def_cfa	29, -1		# fake cfa one byte below sp
>>> 	.cfi_register	29, 29		# "save" sp in itself so we don't use the fake cfa
>>> 	move	$18,$31
>>> 	.cfi_register 31, 18
>>> 	...
>> 
>> Ooh, nice (if that's the word).  It certainly fixes the testcase,
>> although I had to use -4 rather than -1 in order to defeat
>> DWARF2_CIE_DATA_ALIGNMENT.  That should still be OK, since the
>> stack is 8-byte aligned.
>> 
>> GDB doesn't seem to be able to backtrace through this, but that
>> has to come second to correctness.  I'll aim to get a tested fix
>> in this weekend.
>
> Hmm.  I wonder if GDB would be happier with a val_expression,
> rather than the "odd" .cfi_register:
>
> 	// DW_CFA_val_expression r29, { DW_OP_reg29 }
> 	.cfi_escape	0x16,29,1,0x6d

In the end I went for this version.  Although the plain .cfi_register
thing seemed to work for the reduced testcase, it caused the original
one to segfault.  The unwinder was copying the sp's GRPtr from the old
context to the new one, but since there was no "real" sp save slot
to use, this GRPtr ended up being the local tmp_sp variable from
uw_update_context_1.  We'd then end up using tmp_sp after the
function had exited.

The .cfi_escape is safe because we're forced to calculate a value
rather than a pointer.  (Which I guess is why you suggested it --
was a bit slow on the uptake.)

As mentioned on the gcc@ thread, gdb can't currently handle this.
But (a) it can't handle the current situation either and
(b) it sounds from Tom's message that later gdbs will cope,
so I think this patch is a strict improvement.

Tested on mips64-linux-gnu, mips-sde-elf and various other mips*-elf targets.
Applied.  I'll get back to the unwinder patch once 4.8 is open.

Richard


gcc/
	* config/mips/mips.c (mips16_build_call_stub): Add CFI information
	to stubs with non-sibling calls.

libgcc/
	* config/mips/mips16.S (CALL_STUB_RET): Add CFI information.

Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	2012-02-18 09:15:10.285775771 +0000
+++ gcc/config/mips/mips.c	2012-02-18 18:11:13.120840632 +0000
@@ -6387,7 +6387,20 @@ mips16_build_call_stub (rtx retval, rtx
       assemble_start_function (stubdecl, stubname);
       mips_start_function_definition (stubname, false);
 
-      if (!fp_ret_p)
+      if (fp_ret_p)
+	{
+	  fprintf (asm_out_file, "\t.cfi_startproc\n");
+
+	  /* Create a fake CFA 4 bytes below the stack pointer.
+	     This works around unwinders (like libgcc's) that expect
+	     the CFA for non-signal frames to be unique.  */
+	  fprintf (asm_out_file, "\t.cfi_def_cfa 29,-4\n");
+
+	  /* "Save" $sp in itself so we don't use the fake CFA.
+	     This is: DW_CFA_val_expression r29, { DW_OP_reg29 }.  */
+	  fprintf (asm_out_file, "\t.cfi_escape 0x16,29,1,0x6d\n");
+	}
+      else
 	{
 	  /* Load the address of the MIPS16 function into $25.  Do this
 	     first so that targets with coprocessor interlocks can use
@@ -6405,12 +6418,7 @@ mips16_build_call_stub (rtx retval, rtx
 	 registers.  */
       mips_output_args_xfer (fp_code, 't');
 
-      if (!fp_ret_p)
-	{
-	  /* Jump to the previously-loaded address.  */
-	  output_asm_insn ("jr\t%^", NULL);
-	}
-      else
+      if (fp_ret_p)
 	{
 	  /* Save the return address in $18 and call the non-MIPS16 function.
 	     The stub's caller knows that $18 might be clobbered, even though
@@ -6418,6 +6426,7 @@ mips16_build_call_stub (rtx retval, rtx
 	  fprintf (asm_out_file, "\tmove\t%s,%s\n",
 		   reg_names[GP_REG_FIRST + 18], reg_names[RETURN_ADDR_REGNUM]);
 	  output_asm_insn (MIPS_CALL ("jal", &fn, 0, -1), &fn);
+	  fprintf (asm_out_file, "\t.cfi_register 31,18\n");
 
 	  /* Move the result from floating-point registers to
 	     general registers.  */
@@ -6470,6 +6479,12 @@ mips16_build_call_stub (rtx retval, rtx
 	      gcc_unreachable ();
 	    }
 	  fprintf (asm_out_file, "\tjr\t%s\n", reg_names[GP_REG_FIRST + 18]);
+	  fprintf (asm_out_file, "\t.cfi_endproc\n");
+	}
+      else
+	{
+	  /* Jump to the previously-loaded address.  */
+	  output_asm_insn ("jr\t%^", NULL);
 	}
 
 #ifdef ASM_DECLARE_FUNCTION_SIZE
Index: libgcc/config/mips/mips16.S
===================================================================
--- libgcc/config/mips/mips16.S	2012-02-18 09:15:10.284775771 +0000
+++ libgcc/config/mips/mips16.S	2012-02-18 18:10:40.371841584 +0000
@@ -566,15 +566,23 @@ CALL_STUB_NO_RET (__mips16_call_stub_10,
    being called is 16 bits, in which case the copy is unnecessary;
    however, it's faster to always do the copy.  */
 
-#define CALL_STUB_RET(NAME, CODE, MODE)	\
-STARTFN (NAME);				\
-	move	$18,$31;		\
-	STUB_ARGS_##CODE;		\
-	.set	noreorder;		\
-	jalr	$2;			\
-	move	$25,$2;			\
-	.set	reorder;		\
-	MOVE_##MODE##_RET (f, $18);	\
+#define CALL_STUB_RET(NAME, CODE, MODE)					\
+STARTFN (NAME);								\
+	.cfi_startproc;							\
+	/* Create a fake CFA 4 bytes below the stack pointer.  */	\
+	.cfi_def_cfa 29,-4;						\
+	/* "Save" $sp in itself so we don't use the fake CFA.		\
+	   This is: DW_CFA_val_expression r29, { DW_OP_reg29 }.  */	\
+	.cfi_escape 0x16,29,1,0x6d;					\
+	move	$18,$31;						\
+	.cfi_register 31,18;						\
+	STUB_ARGS_##CODE;						\
+	.set	noreorder;						\
+	jalr	$2;							\
+	move	$25,$2;							\
+	.set	reorder;						\
+	MOVE_##MODE##_RET (f, $18);					\
+	.cfi_endproc;							\
 	ENDFN (NAME)
 
 /* First, instantiate the single-float set.  */