Re: [PATCH] Fix ARM dwarf2cfi ICE and unwind info issues (PR target/59575)

2014-02-06 Thread Richard Henderson 
On 01/30/2014 12:38 PM, Jakub Jelinek wrote:
 2014-01-30  Jakub Jelinek  ja...@redhat.com
 
   PR target/59575
   * config/arm/arm.c (emit_multi_reg_push): Add dwarf_regs_mask argument,
   don't record in REG_FRAME_RELATED_EXPR registers not set in that
   bitmask.
   (arm_expand_prologue): Adjust all callers.
   (arm_unwind_emit_sequence): Allow saved, but not important for unwind
   info, registers also at the lowest numbered registers side.  Use
   gcc_assert instead of abort, and SET_SRC/SET_DEST macros instead of
   XEXP.
 
   * gcc.target/arm/pr59575.c: New test.

Give the ARM guys another 24 hours to comment, but as I said in the PR I think
the patch is good.

r~

Re: [PATCH] Fix ARM dwarf2cfi ICE and unwind info issues (PR target/59575)

2014-02-06 Thread Ramana Radhakrishnan 
On Thu, Jan 30, 2014 at 8:38 PM, Jakub Jelinek ja...@redhat.com wrote:
 Hi!

 For -Os, apparently ARM backend sometimes decides to push (up to 8?) dummy
 registers to stack in addition to the registers that actually need to be
 saved, in order to avoid extra instruction to adjust stack pointer.
 These registers shouldn't be mentioned for .debug_frame though (both because
 it breaks assumptions of dwarf2cfi and because it doesn't make sense), they
 are either call clobbered which don't need saving (r0-r3) or for r4-r7 would
 be dummy if they aren't ever modified in the current function, again, there
 is no point in saving them.  Even for ARM unwind info I think it doesn't
 make sense to mention them in the unwind info, the patch instead adds .pad 
 #NNN
 directive after the .save to adjust sp correspondingly.

 Kyrill has kindly bootstrapped/regtested this on arm, ok for trunk?

This is OK with a small comment on top of arm_unwind_emit_sequence
just describing the reasoning here. I was trying to convince myself
that the changes for the unwind information was safe and yes it
follows similar to what you describe.

regards
Ramana




 2014-01-30  Jakub Jelinek  ja...@redhat.com

 PR target/59575
 * config/arm/arm.c (emit_multi_reg_push): Add dwarf_regs_mask 
 argument,
 don't record in REG_FRAME_RELATED_EXPR registers not set in that
 bitmask.
 (arm_expand_prologue): Adjust all callers.
 (arm_unwind_emit_sequence): Allow saved, but not important for unwind
 info, registers also at the lowest numbered registers side.  Use
 gcc_assert instead of abort, and SET_SRC/SET_DEST macros instead of
 XEXP.

 * gcc.target/arm/pr59575.c: New test.

 --- gcc/config/arm/arm.c.jj 2014-01-29 10:21:11.448031616 +0100
 +++ gcc/config/arm/arm.c2014-01-29 15:32:22.246381587 +0100
 @@ -177,7 +177,7 @@ static rtx arm_expand_builtin (tree, rtx
  static tree arm_builtin_decl (unsigned, bool);
  static void emit_constant_insn (rtx cond, rtx pattern);
  static rtx emit_set_insn (rtx, rtx);
 -static rtx emit_multi_reg_push (unsigned long);
 +static rtx emit_multi_reg_push (unsigned long, unsigned long);
  static int arm_arg_partial_bytes (cumulative_args_t, enum machine_mode,
   tree, bool);
  static rtx arm_function_arg (cumulative_args_t, enum machine_mode,
 @@ -19573,28 +19573,33 @@ arm_emit_strd_push (unsigned long saved_
  /* Generate and emit an insn that we will recognize as a push_multi.
 Unfortunately, since this insn does not reflect very well the actual
 semantics of the operation, we need to annotate the insn for the benefit
 -   of DWARF2 frame unwind information.  */
 +   of DWARF2 frame unwind information.  DWARF_REGS_MASK is a subset of
 +   MASK for registers that should be annotated for DWARF2 frame unwind
 +   information.  */
  static rtx
 -emit_multi_reg_push (unsigned long mask)
 +emit_multi_reg_push (unsigned long mask, unsigned long dwarf_regs_mask)
  {
int num_regs = 0;
 -  int num_dwarf_regs;
 +  int num_dwarf_regs = 0;
int i, j;
rtx par;
rtx dwarf;
int dwarf_par_index;
rtx tmp, reg;

 +  /* We don't record the PC in the dwarf frame information.  */
 +  dwarf_regs_mask = ~(1  PC_REGNUM);
 +
for (i = 0; i = LAST_ARM_REGNUM; i++)
 -if (mask  (1  i))
 -  num_regs++;
 +{
 +  if (mask  (1  i))
 +   num_regs++;
 +  if (dwarf_regs_mask  (1  i))
 +   num_dwarf_regs++;
 +}

gcc_assert (num_regs  num_regs = 16);
 -
 -  /* We don't record the PC in the dwarf frame information.  */
 -  num_dwarf_regs = num_regs;
 -  if (mask  (1  PC_REGNUM))
 -num_dwarf_regs--;
 +  gcc_assert ((dwarf_regs_mask  ~mask) == 0);

/* For the body of the insn we are going to generate an UNSPEC in
   parallel with several USEs.  This allows the insn to be recognized
 @@ -19660,14 +19665,13 @@ emit_multi_reg_push (unsigned long mask)
gen_rtvec (1, reg),
UNSPEC_PUSH_MULT));

 - if (i != PC_REGNUM)
 + if (dwarf_regs_mask  (1  i))
 {
   tmp = gen_rtx_SET (VOIDmode,
  gen_frame_mem (SImode, stack_pointer_rtx),
  reg);
   RTX_FRAME_RELATED_P (tmp) = 1;
 - XVECEXP (dwarf, 0, dwarf_par_index) = tmp;
 - dwarf_par_index++;
 + XVECEXP (dwarf, 0, dwarf_par_index++) = tmp;
 }

   break;
 @@ -19682,7 +19686,7 @@ emit_multi_reg_push (unsigned long mask)

   XVECEXP (par, 0, j) = gen_rtx_USE (VOIDmode, reg);

 - if (i != PC_REGNUM)
 + if (dwarf_regs_mask  (1  i))
 {
   tmp
 = gen_rtx_SET (VOIDmode,
 @@ -20689,7 +20693,7 @@ arm_expand_prologue (void)
   /* Interrupt functions must not corrupt any registers.
  Creating a

Re: [PATCH] Fix ARM dwarf2cfi ICE and unwind info issues (PR target/59575)

2014-02-06 Thread nick clifton 

Hi Jakub,


2014-01-30  Jakub Jelinek  ja...@redhat.com

PR target/59575
* config/arm/arm.c (emit_multi_reg_push): Add dwarf_regs_mask argument,
don't record in REG_FRAME_RELATED_EXPR registers not set in that
bitmask.
(arm_expand_prologue): Adjust all callers.
(arm_unwind_emit_sequence): Allow saved, but not important for unwind
info, registers also at the lowest numbered registers side.  Use
gcc_assert instead of abort, and SET_SRC/SET_DEST macros instead of
XEXP.

* gcc.target/arm/pr59575.c: New test.


Approved - please apply.

Cheers
  Nick

[PATCH] Fix ARM dwarf2cfi ICE and unwind info issues (PR target/59575)

2014-01-30 Thread Jakub Jelinek 
Hi!

For -Os, apparently ARM backend sometimes decides to push (up to 8?) dummy
registers to stack in addition to the registers that actually need to be
saved, in order to avoid extra instruction to adjust stack pointer.
These registers shouldn't be mentioned for .debug_frame though (both because
it breaks assumptions of dwarf2cfi and because it doesn't make sense), they
are either call clobbered which don't need saving (r0-r3) or for r4-r7 would
be dummy if they aren't ever modified in the current function, again, there
is no point in saving them.  Even for ARM unwind info I think it doesn't
make sense to mention them in the unwind info, the patch instead adds .pad #NNN
directive after the .save to adjust sp correspondingly.

Kyrill has kindly bootstrapped/regtested this on arm, ok for trunk?

2014-01-30  Jakub Jelinek  ja...@redhat.com

PR target/59575
* config/arm/arm.c (emit_multi_reg_push): Add dwarf_regs_mask argument,
don't record in REG_FRAME_RELATED_EXPR registers not set in that
bitmask.
(arm_expand_prologue): Adjust all callers.
(arm_unwind_emit_sequence): Allow saved, but not important for unwind
info, registers also at the lowest numbered registers side.  Use
gcc_assert instead of abort, and SET_SRC/SET_DEST macros instead of
XEXP.

* gcc.target/arm/pr59575.c: New test.

--- gcc/config/arm/arm.c.jj 2014-01-29 10:21:11.448031616 +0100
+++ gcc/config/arm/arm.c2014-01-29 15:32:22.246381587 +0100
@@ -177,7 +177,7 @@ static rtx arm_expand_builtin (tree, rtx
 static tree arm_builtin_decl (unsigned, bool);
 static void emit_constant_insn (rtx cond, rtx pattern);
 static rtx emit_set_insn (rtx, rtx);
-static rtx emit_multi_reg_push (unsigned long);
+static rtx emit_multi_reg_push (unsigned long, unsigned long);
 static int arm_arg_partial_bytes (cumulative_args_t, enum machine_mode,
  tree, bool);
 static rtx arm_function_arg (cumulative_args_t, enum machine_mode,
@@ -19573,28 +19573,33 @@ arm_emit_strd_push (unsigned long saved_
 /* Generate and emit an insn that we will recognize as a push_multi.
Unfortunately, since this insn does not reflect very well the actual
semantics of the operation, we need to annotate the insn for the benefit
-   of DWARF2 frame unwind information.  */
+   of DWARF2 frame unwind information.  DWARF_REGS_MASK is a subset of
+   MASK for registers that should be annotated for DWARF2 frame unwind
+   information.  */
 static rtx
-emit_multi_reg_push (unsigned long mask)
+emit_multi_reg_push (unsigned long mask, unsigned long dwarf_regs_mask)
 {
   int num_regs = 0;
-  int num_dwarf_regs;
+  int num_dwarf_regs = 0;
   int i, j;
   rtx par;
   rtx dwarf;
   int dwarf_par_index;
   rtx tmp, reg;
 
+  /* We don't record the PC in the dwarf frame information.  */
+  dwarf_regs_mask = ~(1  PC_REGNUM);
+
   for (i = 0; i = LAST_ARM_REGNUM; i++)
-if (mask  (1  i))
-  num_regs++;
+{
+  if (mask  (1  i))
+   num_regs++;
+  if (dwarf_regs_mask  (1  i))
+   num_dwarf_regs++;
+}
 
   gcc_assert (num_regs  num_regs = 16);
-
-  /* We don't record the PC in the dwarf frame information.  */
-  num_dwarf_regs = num_regs;
-  if (mask  (1  PC_REGNUM))
-num_dwarf_regs--;
+  gcc_assert ((dwarf_regs_mask  ~mask) == 0);
 
   /* For the body of the insn we are going to generate an UNSPEC in
  parallel with several USEs.  This allows the insn to be recognized
@@ -19660,14 +19665,13 @@ emit_multi_reg_push (unsigned long mask)
   gen_rtvec (1, reg),
   UNSPEC_PUSH_MULT));
 
- if (i != PC_REGNUM)
+ if (dwarf_regs_mask  (1  i))
{
  tmp = gen_rtx_SET (VOIDmode,
 gen_frame_mem (SImode, stack_pointer_rtx),
 reg);
  RTX_FRAME_RELATED_P (tmp) = 1;
- XVECEXP (dwarf, 0, dwarf_par_index) = tmp;
- dwarf_par_index++;
+ XVECEXP (dwarf, 0, dwarf_par_index++) = tmp;
}
 
  break;
@@ -19682,7 +19686,7 @@ emit_multi_reg_push (unsigned long mask)
 
  XVECEXP (par, 0, j) = gen_rtx_USE (VOIDmode, reg);
 
- if (i != PC_REGNUM)
+ if (dwarf_regs_mask  (1  i))
{
  tmp
= gen_rtx_SET (VOIDmode,
@@ -20689,7 +20693,7 @@ arm_expand_prologue (void)
  /* Interrupt functions must not corrupt any registers.
 Creating a frame pointer however, corrupts the IP
 register, so we must push it first.  */
- emit_multi_reg_push (1  IP_REGNUM);
+ emit_multi_reg_push (1  IP_REGNUM, 1  IP_REGNUM);
 
  /* Do not set RTX_FRAME_RELATED_P on this insn.
 The dwarf stack unwinding code only wants to see one
@@ -20750,7 +20754,8 @@ arm_expand_prologue (void)
  if (cfun-machine-uses_anonymous_args)