MSP430: Fix CFA generation during function epilogues

Message ID 20200909103342.3gj2c6vipuex4qby@jozef-acer-manjaro
State New
Headers show
Series
  • MSP430: Fix CFA generation during function epilogues
Related show

Commit Message

Jozef Lawrynowicz Sept. 9, 2020, 10:33 a.m.
There is no CFA information generated for instructions which manipulate the
stack during function epilogues. This means a debugger cannot determine the
position of variables on the stack whilst the epilogue is in progress.

This can cause the debugger to give erroneous information when printing a
backtrace whilst stepping through the epilogue, or cause software watchpoints
set on stack variables to become invalidated after a function epilogue
is executed.

The patch fixes this by marking stack manipulation insns as
frame_related, and adding reg_note RTXs to stack pop instructions in the
epilogue.

Successfully regtested on trunk for msp430-elf in the default, -mlarge,
-mcpu=msp430 and -mlarge/-mcode-region=either/-mdata-region=either
configurations.

This fixes some tests from watchpoint.exp in the GDB testsuite.

Ok for trunk?

Thanks,
Jozef
From 272b38a374eddf7327a61ff9b1730f0a2dd40233 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>

Date: Mon, 7 Sep 2020 20:34:40 +0100
Subject: [PATCH] MSP430: Fix CFA generation during function epilogues

There is no CFA information generated for instructions which manipulate the
stack during function epilogues. This means a debugger cannot determine the
position of variables on the stack whilst the epilogue is in progress.

This can cause the debugger to give erroneous information when printing a
backtrace whilst stepping through the epilogue, or cause software watchpoints
set on stack variables to become invalidated after a function epilogue
is executed.

The patch fixes this by marking stack manipulation insns as
frame_related, and adding reg_note RTXs to stack pop instructions in the
epilogue.

gcc/ChangeLog:

	* config/msp430/msp430.c (increment_stack): Mark insns which increment
	the stack as frame_related.
	(msp430_expand_prologue): Add comments.
	(msp430_expand_epilogue): Mark insns which decrement
	the stack as frame_related.
	Add reg_note to stack pop insns describing position of register
	variables on the stack.

---
 gcc/config/msp430/msp430.c | 72 +++++++++++++++++++++++++++++++-------
 1 file changed, 59 insertions(+), 13 deletions(-)

-- 
2.28.0

Comments

H.J. Lu via Gcc-patches Sept. 17, 2020, 6:22 p.m. | #1
On 9/9/20 4:33 AM, Jozef Lawrynowicz wrote:
> There is no CFA information generated for instructions which manipulate the

> stack during function epilogues. This means a debugger cannot determine the

> position of variables on the stack whilst the epilogue is in progress.

>

> This can cause the debugger to give erroneous information when printing a

> backtrace whilst stepping through the epilogue, or cause software watchpoints

> set on stack variables to become invalidated after a function epilogue

> is executed.

>

> The patch fixes this by marking stack manipulation insns as

> frame_related, and adding reg_note RTXs to stack pop instructions in the

> epilogue.

>

> Successfully regtested on trunk for msp430-elf in the default, -mlarge,

> -mcpu=msp430 and -mlarge/-mcode-region=either/-mdata-region=either

> configurations.

>

> This fixes some tests from watchpoint.exp in the GDB testsuite.

>

> Ok for trunk?

>

> Thanks,

> Jozef

>

> 0001-MSP430-Fix-CFA-generation-during-function-epilogues.patch

>

> From 272b38a374eddf7327a61ff9b1730f0a2dd40233 Mon Sep 17 00:00:00 2001

> From: Jozef Lawrynowicz <jozef.l@mittosystems.com>

> Date: Mon, 7 Sep 2020 20:34:40 +0100

> Subject: [PATCH] MSP430: Fix CFA generation during function epilogues

>

> There is no CFA information generated for instructions which manipulate the

> stack during function epilogues. This means a debugger cannot determine the

> position of variables on the stack whilst the epilogue is in progress.

>

> This can cause the debugger to give erroneous information when printing a

> backtrace whilst stepping through the epilogue, or cause software watchpoints

> set on stack variables to become invalidated after a function epilogue

> is executed.

>

> The patch fixes this by marking stack manipulation insns as

> frame_related, and adding reg_note RTXs to stack pop instructions in the

> epilogue.

>

> gcc/ChangeLog:

>

> 	* config/msp430/msp430.c (increment_stack): Mark insns which increment

> 	the stack as frame_related.

> 	(msp430_expand_prologue): Add comments.

> 	(msp430_expand_epilogue): Mark insns which decrement

> 	the stack as frame_related.

> 	Add reg_note to stack pop insns describing position of register

> 	variables on the stack.


OK

jeff

Patch

diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
index 129b916715e..1cb1b8f8626 100644
--- a/gcc/config/msp430/msp430.c
+++ b/gcc/config/msp430/msp430.c
@@ -1700,9 +1700,9 @@  increment_stack (HOST_WIDE_INT amount)
     {
       inc = GEN_INT (amount);
       if (TARGET_LARGE)
-	emit_insn (gen_addpsi3 (sp, sp, inc));
+	F (emit_insn (gen_addpsi3 (sp, sp, inc)));
       else
-	emit_insn (gen_addhi3 (sp, sp, inc));
+	F (emit_insn (gen_addhi3 (sp, sp, inc)));
     }
 }
 
@@ -2413,6 +2413,8 @@  msp430_expand_prologue (void)
   for (i = 15; i >= 4; i--)
     if (cfun->machine->need_to_save[i])
       {
+	/* We need to save COUNT sequential registers starting from regnum
+	   I.  */
 	int seq, count;
 	rtx note;
 
@@ -2427,6 +2429,7 @@  msp430_expand_prologue (void)
 	    p = F (emit_insn (gen_pushm (gen_rtx_REG (Pmode, i),
 					 GEN_INT (count))));
 
+	    /* Document the stack decrement as a result of PUSHM.  */
 	    note = gen_rtx_SEQUENCE (VOIDmode, rtvec_alloc (count + 1));
 
 	    XVECEXP (note, 0, 0)
@@ -2475,8 +2478,10 @@  msp430_expand_prologue (void)
 void
 msp430_expand_epilogue (int is_eh)
 {
-  int i;
+  int i, j;
   int fs;
+  rtx sp = stack_pointer_rtx;
+  rtx p;
   int helper_n = 0;
 
   if (is_naked_func ())
@@ -2545,19 +2550,27 @@  msp430_expand_epilogue (int is_eh)
   for (i = 4; i <= 15; i++)
     if (cfun->machine->need_to_save[i])
       {
-	int seq, count;
+	/* We need to restore COUNT sequential registers starting from regnum
+	   I.  */
+	int seq;
+	int count = 1;
+	int helper_used = 0;
+	rtx note, addr;
 
-	for (seq = i + 1; seq <= 15 && cfun->machine->need_to_save[seq]; seq ++)
-	  ;
-	count = seq - i;
+	if (msp430x)
+	  {
+	    for (seq = i + 1; seq <= 15 && cfun->machine->need_to_save[seq];
+		 seq++)
+	      ;
+	    count = seq - i;
+	  }
 
 	if (msp430x)
 	  {
 	    /* Note: With TARGET_LARGE we still use
 	       POPM as POPX.A is two bytes bigger.  */
-	    emit_insn (gen_popm (stack_pointer_rtx, GEN_INT (seq - 1),
-				 GEN_INT (count)));
-	    i += count - 1;
+	    p = F (emit_insn (gen_popm (stack_pointer_rtx, GEN_INT (seq - 1),
+					GEN_INT (count))));
 	  }
 	else if (i == 11 - helper_n
 		 && ! msp430_is_interrupt_func ()
@@ -2569,11 +2582,44 @@  msp430_expand_epilogue (int is_eh)
 		 && helper_n > 1
 		 && !is_eh)
 	  {
-	    emit_jump_insn (gen_epilogue_helper (GEN_INT (helper_n)));
-	    return;
+	    p = F (emit_jump_insn (gen_epilogue_helper (GEN_INT (helper_n))));
+	    count = helper_n;
+	    helper_used = 1;
 	  }
 	else
-	  emit_insn (gen_pop (gen_rtx_REG (Pmode, i)));
+	  p = F (emit_insn (gen_pop (gen_rtx_REG (Pmode, i))));
+
+	/* Document the stack increment as a result of POPM.  */
+	note = gen_rtx_SEQUENCE (VOIDmode, rtvec_alloc (count + 1));
+
+	addr = gen_rtx_PLUS (Pmode, stack_pointer_rtx,
+			     GEN_INT (count * (TARGET_LARGE ? 4 : 2)));
+
+	XVECEXP (note, 0, 0) = F (gen_rtx_SET (stack_pointer_rtx, addr));
+
+
+	/* *sp++ = R[i+j] */
+	/* sp	R4
+	   ...
+	   sp+N	R10.  */
+	for (j = 0; j < count; j++)
+	  {
+	    int ofs = j * (TARGET_LARGE ? 4 : 2);
+
+	    if (ofs)
+	      addr = gen_rtx_PLUS (Pmode, sp, GEN_INT (ofs));
+	    else
+	      addr = stack_pointer_rtx;
+
+	    XVECEXP (note, 0, j + 1)
+	      = F (gen_rtx_SET (gen_rtx_MEM (Pmode, addr),
+				gen_rtx_REG (Pmode, i + j)));
+	  }
+	add_reg_note (p, REG_FRAME_RELATED_EXPR, note);
+	i += count - 1;
+
+	if (helper_used)
+	  return;
       }
 
   if (is_eh)