Patchwork [AArch64] fix missing Dwarf call frame information in the epilogue

login
register
mail settings
Submitter Yufeng Zhang
Date Nov. 6, 2012, 5:56 p.m.
Message ID <50994F5A.4080104@arm.com>
Download mbox | patch
Permalink /patch/197528/
State New
Headers show

Comments

Yufeng Zhang - Nov. 6, 2012, 5:56 p.m.
Hi,

Many thanks for reviewing.  Please find the updated patch.  The explicit 
calls to gen_rtx_PLUS and GEN_INT have been replaced by plus_constant, 
and the call to aarch64_set_frame_expr has been replaced with 
add_reg_note (REG_CFA_ADJUST_CFA).

I'll clean up other cases in aarch64.c in a separate patch.

OK to commit?

Thanks,
Yufeng


gcc/ChangeLog

2012-11-06  Yufeng Zhang  <yufeng.zhang@arm.com>

          * config/aarch64/aarch64.c (aarch64_expand_prologue): For the
          load-pair with writeback instruction, replace
          aarch64_set_frame_expr with add_reg_note (REG_CFA_ADJUST_CFA);
          add new local variable 'cfa_reg' and use it.

gcc/testsuite/ChangeLog

2012-11-06  Yufeng Zhang  <yufeng.zhang@arm.com>

          * gcc.target/aarch64/dwarf-cfa-reg.c: New file.


On 09/12/12 19:37, Richard Henderson wrote:
> On 09/12/2012 09:10 AM, Yufeng Zhang wrote:
>>   	aarch64_set_frame_expr (gen_rtx_SET
>>   				      (Pmode,
>>   				       stack_pointer_rtx,
>> -				       gen_rtx_PLUS (Pmode, stack_pointer_rtx,
>> +				       gen_rtx_PLUS (Pmode, cfa_reg,
>>   						     GEN_INT (offset))));
>
> We'd prefer to use
>
>    plus_constant (Pmode, cfa_reg, offset)
>
> instead of the explicit call to gen_rtx_PLUS and GEN_INT.
> It would appear that the entire aarch64.c file ought to
> be audited for that.
>
> Also, use of the REG_CFA_* notes is strongly encouraged over
> use of REG_FRAME_RELATED_EXPR.
>
> There's all sorts of work involved in turning R_F_R_E into
> R_CFA_* notes, depending on a rather large state machine.
> This state machine was developed when only prologues were
> annotated for unwinding, and therefore one cannot expect it
> to work reliably for epilogues.
>
> A long-term goal is to convert all targets to use R_CFA_*
> exclusively, as that preserves much more information present
> in the structure of the code of the prologue generator.  It
> means less work within the compiler, and eventually being able
> to remove a rather large hunk of state-machine code.
>
>
> r~
>
Richard Henderson - Nov. 6, 2012, 6:21 p.m.
On 2012-11-06 09:56, Yufeng Zhang wrote:
> 2012-11-06  Yufeng Zhang  <yufeng.zhang@arm.com>
> 
>          * config/aarch64/aarch64.c (aarch64_expand_prologue): For the
>          load-pair with writeback instruction, replace
>          aarch64_set_frame_expr with add_reg_note (REG_CFA_ADJUST_CFA);
>          add new local variable 'cfa_reg' and use it.
> 
> gcc/testsuite/ChangeLog
> 
> 2012-11-06  Yufeng Zhang  <yufeng.zhang@arm.com>
> 
>          * gcc.target/aarch64/dwarf-cfa-reg.c: New file.

Looks good.


r~
Marcus Shawcroft - Nov. 6, 2012, 6:25 p.m.
On 06/11/12 18:21, Richard Henderson wrote:
> On 2012-11-06 09:56, Yufeng Zhang wrote:
>> 2012-11-06  Yufeng Zhang<yufeng.zhang@arm.com>
>>
>>           * config/aarch64/aarch64.c (aarch64_expand_prologue): For the
>>           load-pair with writeback instruction, replace
>>           aarch64_set_frame_expr with add_reg_note (REG_CFA_ADJUST_CFA);
>>           add new local variable 'cfa_reg' and use it.
>>
>> gcc/testsuite/ChangeLog
>>
>> 2012-11-06  Yufeng Zhang<yufeng.zhang@arm.com>
>>
>>           * gcc.target/aarch64/dwarf-cfa-reg.c: New file.
>
> Looks good.
>
>
> r~
>

OK.  Yufeng, please back port this onto ARM/aarch64-4.7-branch.

Cheers
/Marcus

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index b36be90..8a2d7ba 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -1777,7 +1777,7 @@  aarch64_expand_prologue (void)
 	       - original_frame_size
 	       - cfun->machine->frame.saved_regs_size);
 
-  /* Store pairs and load pairs have a range only of +/- 512.  */
+  /* Store pairs and load pairs have a range only -512 to 504.  */
   if (offset >= 512)
     {
       /* When the frame has a large size, an initial decrease is done on
@@ -1923,6 +1923,7 @@  aarch64_expand_epilogue (bool for_sibcall)
   HOST_WIDE_INT original_frame_size, frame_size, offset;
   HOST_WIDE_INT fp_offset;
   rtx insn;
+  rtx cfa_reg;
 
   aarch64_layout_frame ();
   original_frame_size = get_frame_size () + cfun->machine->saved_varargs_size;
@@ -1935,7 +1936,9 @@  aarch64_expand_epilogue (bool for_sibcall)
 	       - original_frame_size
 	       - cfun->machine->frame.saved_regs_size);
 
-  /* Store pairs and load pairs have a range only of +/- 512.  */
+  cfa_reg = frame_pointer_needed ? hard_frame_pointer_rtx : stack_pointer_rtx;
+
+  /* Store pairs and load pairs have a range only -512 to 504.  */
   if (offset >= 512)
     {
       offset = original_frame_size + cfun->machine->frame.saved_regs_size;
@@ -1966,6 +1969,10 @@  aarch64_expand_epilogue (bool for_sibcall)
 				       hard_frame_pointer_rtx,
 				       GEN_INT (- fp_offset)));
       RTX_FRAME_RELATED_P (insn) = 1;
+      /* As SP is set to (FP - fp_offset), according to the rules in
+	 dwarf2cfi.c:dwarf2out_frame_debug_expr, CFA should be calculated
+	 from the value of SP from now on.  */
+      cfa_reg = stack_pointer_rtx;
     }
 
   aarch64_save_or_restore_callee_save_registers
@@ -2003,11 +2010,9 @@  aarch64_expand_epilogue (bool for_sibcall)
 				 GEN_INT (offset),
 				 GEN_INT (GET_MODE_SIZE (DImode) + offset)));
 	      RTX_FRAME_RELATED_P (XVECEXP (PATTERN (insn), 0, 2)) = 1;
-	      aarch64_set_frame_expr (gen_rtx_SET
-				      (Pmode,
-				       stack_pointer_rtx,
-				       gen_rtx_PLUS (Pmode, stack_pointer_rtx,
-						     GEN_INT (offset))));
+	      add_reg_note (insn, REG_CFA_ADJUST_CFA,
+			    (gen_rtx_SET (Pmode, stack_pointer_rtx,
+					  plus_constant (cfa_reg, offset))));
 	    }
 
 	  /* The first part of a frame-related parallel insn
@@ -2027,7 +2032,6 @@  aarch64_expand_epilogue (bool for_sibcall)
 	      RTX_FRAME_RELATED_P (insn) = 1;
 	    }
 	}
-
       else
 	{
 	  insn = emit_insn (gen_add2_insn (stack_pointer_rtx,
diff --git a/gcc/testsuite/gcc.target/aarch64/dwarf-cfa-reg.c b/gcc/testsuite/gcc.target/aarch64/dwarf-cfa-reg.c
new file mode 100644
index 0000000..cce8815
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/dwarf-cfa-reg.c
@@ -0,0 +1,14 @@ 
+/* Verify that CFA register is restored to SP after FP is restored.  */
+/* { dg-do compile } */
+/* { dg-options "-O0 -gdwarf-2" } */
+/* { dg-final { scan-assembler ".cfi_restore 30" } } */
+/* { dg-final { scan-assembler ".cfi_restore 29" } } */
+/* { dg-final { scan-assembler ".cfi_def_cfa 31, 0" } } */
+/* { dg-final { scan-assembler "ret" } } */
+
+int bar (unsigned int);
+
+int foo (void)
+{
+  return bar (0xcafe);
+}