[PATCH 0/4] Fix gdb.trace/entry-values.exp fails in thumb mode

[PATCH 0/4] Fix gdb.trace/entry-values.exp fails in thumb mode

[Yao Qi]

We see some fails in gdb.trace/entry-values.exp in thumb mode
(-mthumb -march={armv4t,armv7-a}), which are caused by two problems in
gdb,

 - prologue analysis in gdb for thumb code is broken.  Patch #3 is to
   fix it by stopping prologue analysis when it goes to the epilogue.
 - dwarf assembler in gdb.trace/entry-values.exp uses functions whose
   address's bit 0 is set, which is incorrect.  Patch #4 is to fix it.

Patch 1 is preparatory patch and patch 2 is a refactor one.  Each of them
is regression tested on arm-none-linux-gnueabi.

*** BLURB HERE ***

Yao Qi (4):
  Restrict matching add/sub sp, #imm
  Match instruction adjusts SP in thumb
  Stop prologue analysis when past the epilogue
  Fix gdb.trace/entry-values.exp for thumb mode

 gdb/arm-tdep.c                           | 41 +++++++++++++++++---------------
 gdb/testsuite/gdb.trace/entry-values.c   |  6 +++++
 gdb/testsuite/gdb.trace/entry-values.exp | 10 ++++----
 3 files changed, 33 insertions(+), 24 deletions(-)

-- 
1.9.0

[PATCH 2/4] Match instruction adjusts SP in thumb

[Yao Qi]

This is a refactor patch, that moves matching instructions adjusting
SP into a new function, thumb_instruction_restores_sp.  The second
call to thumb_instruction_restores_sp in thumb_in_function_epilogue_p
is a little different from the original.  The original code matches
'POP <registers> without PC', but thumb_in_function_epilogue_p matches
'POP <registers> (with and without PC)'.  However, GDB found one
instruction about return and is scanning the previous instruction,
which should be an instruction about return too, so the code change
doesn't affect the functionality.

gdb:

2014-07-02  Yao Qi  <yao@codesourcery.com>

	* arm-tdep.c (thumb_instruction_restores_sp): New function.
	(thumb_in_function_epilogue_p): Call
	thumb_instruction_restores_sp.
---
 gdb/arm-tdep.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 0fc7fc1..153ef42 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -685,6 +685,17 @@ thumb2_instruction_changes_pc (unsigned short inst1, unsigned short inst2)
   return 0;
 }
 
+/* Return 1 if the 16-bit Thumb instruction INSN restores SP in
+   epilogue, 0 otherwise.  */
+
+static int
+thumb_instruction_restores_sp (unsigned short insn)
+{
+  return (insn == 0x46bd  /* mov sp, r7 */
+	  || (insn & 0xff80) == 0xb000  /* add sp, imm */
+	  || (insn & 0xfe00) == 0xbc00);  /* pop <registers> */
+}
+
 /* Analyze a Thumb prologue, looking for a recognizable stack frame
    and frame pointer.  Scan until we encounter a store that could
    clobber the stack frame unexpectedly, or an unknown instruction.
@@ -3257,14 +3268,10 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 	found_return = 1;
       else if (insn == 0x46f7)  /* mov pc, lr */
 	found_return = 1;
-      else if (insn == 0x46bd)  /* mov sp, r7 */
-	found_stack_adjust = 1;
-      else if ((insn & 0xff80) == 0xb000)  /* add sp, imm */
-	found_stack_adjust = 1;
-      else if ((insn & 0xfe00) == 0xbc00)  /* pop <registers> */
+      else if (thumb_instruction_restores_sp (insn))
 	{
 	  found_stack_adjust = 1;
-	  if (insn & 0x0100)  /* <registers> include PC.  */
+	  if ((insn & 0xfe00) == 0xbd00)  /* pop <registers, PC> */
 	    found_return = 1;
 	}
       else if (thumb_insn_size (insn) == 4)  /* 32-bit Thumb-2 instruction */
@@ -3317,11 +3324,7 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
       insn = extract_unsigned_integer (buf, 2, byte_order_for_code);
       insn2 = extract_unsigned_integer (buf + 2, 2, byte_order_for_code);
 
-      if (insn2 == 0x46bd)  /* mov sp, r7 */
-	found_stack_adjust = 1;
-      else if ((insn2 & 0xff80) == 0xb000)  /* add sp, imm */
-	found_stack_adjust = 1;
-      else if ((insn2 & 0xff00) == 0xbc00)  /* pop <registers> without PC */
+      if (thumb_instruction_restores_sp (insn2))
 	found_stack_adjust = 1;
       else if (insn == 0xe8bd)  /* ldm.w sp!, <registers> */
 	found_stack_adjust = 1;
-- 
1.9.0

Re: [PATCH 2/4] Match instruction adjusts SP in thumb

[Will Newton]

On 3 July 2014 07:09, Yao Qi <yao@codesourcery.com> wrote:
> This is a refactor patch, that moves matching instructions adjusting
> SP into a new function, thumb_instruction_restores_sp.  The second
> call to thumb_instruction_restores_sp in thumb_in_function_epilogue_p
> is a little different from the original.  The original code matches
> 'POP <registers> without PC', but thumb_in_function_epilogue_p matches
> 'POP <registers> (with and without PC)'.  However, GDB found one
> instruction about return and is scanning the previous instruction,
> which should be an instruction about return too, so the code change
> doesn't affect the functionality.
>
> gdb:
>
> 2014-07-02  Yao Qi  <yao@codesourcery.com>
>
>         * arm-tdep.c (thumb_instruction_restores_sp): New function.
>         (thumb_in_function_epilogue_p): Call
>         thumb_instruction_restores_sp.
> ---
>  gdb/arm-tdep.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)

This patch looks good to me.

> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 0fc7fc1..153ef42 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -685,6 +685,17 @@ thumb2_instruction_changes_pc (unsigned short inst1, unsigned short inst2)
>    return 0;
>  }
>
> +/* Return 1 if the 16-bit Thumb instruction INSN restores SP in
> +   epilogue, 0 otherwise.  */
> +
> +static int
> +thumb_instruction_restores_sp (unsigned short insn)
> +{
> +  return (insn == 0x46bd  /* mov sp, r7 */
> +         || (insn & 0xff80) == 0xb000  /* add sp, imm */
> +         || (insn & 0xfe00) == 0xbc00);  /* pop <registers> */
> +}
> +
>  /* Analyze a Thumb prologue, looking for a recognizable stack frame
>     and frame pointer.  Scan until we encounter a store that could
>     clobber the stack frame unexpectedly, or an unknown instruction.
> @@ -3257,14 +3268,10 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>         found_return = 1;
>        else if (insn == 0x46f7)  /* mov pc, lr */
>         found_return = 1;
> -      else if (insn == 0x46bd)  /* mov sp, r7 */
> -       found_stack_adjust = 1;
> -      else if ((insn & 0xff80) == 0xb000)  /* add sp, imm */
> -       found_stack_adjust = 1;
> -      else if ((insn & 0xfe00) == 0xbc00)  /* pop <registers> */
> +      else if (thumb_instruction_restores_sp (insn))
>         {
>           found_stack_adjust = 1;
> -         if (insn & 0x0100)  /* <registers> include PC.  */
> +         if ((insn & 0xfe00) == 0xbd00)  /* pop <registers, PC> */
>             found_return = 1;
>         }
>        else if (thumb_insn_size (insn) == 4)  /* 32-bit Thumb-2 instruction */
> @@ -3317,11 +3324,7 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>        insn = extract_unsigned_integer (buf, 2, byte_order_for_code);
>        insn2 = extract_unsigned_integer (buf + 2, 2, byte_order_for_code);
>
> -      if (insn2 == 0x46bd)  /* mov sp, r7 */
> -       found_stack_adjust = 1;
> -      else if ((insn2 & 0xff80) == 0xb000)  /* add sp, imm */
> -       found_stack_adjust = 1;
> -      else if ((insn2 & 0xff00) == 0xbc00)  /* pop <registers> without PC */
> +      if (thumb_instruction_restores_sp (insn2))
>         found_stack_adjust = 1;
>        else if (insn == 0xe8bd)  /* ldm.w sp!, <registers> */
>         found_stack_adjust = 1;
> --
> 1.9.0
>



-- 
Will Newton
Toolchain Working Group, Linaro

Re: [PATCH 2/4] Match instruction adjusts SP in thumb

[Joel Brobecker]

> 2014-07-02  Yao Qi  <yao@codesourcery.com>
> 
> 	* arm-tdep.c (thumb_instruction_restores_sp): New function.
> 	(thumb_in_function_epilogue_p): Call
> 	thumb_instruction_restores_sp.

OK.

-- 
Joel

Re: [PATCH 2/4] Match instruction adjusts SP in thumb

[Yao Qi]

On 07/03/2014 02:09 PM, Yao Qi wrote:
> -      else if ((insn & 0xfe00) == 0xbc00)  /* pop <registers> */
> +      else if (thumb_instruction_restores_sp (insn))
>  	{
>  	  found_stack_adjust = 1;
> -	  if (insn & 0x0100)  /* <registers> include PC.  */
> +	  if ((insn & 0xfe00) == 0xbd00)  /* pop <registers, PC> */
>  	    found_return = 1;
>  	}

This patch fixes a typo in the bit mask I've made in my previous code
refactor.  If PC is in the register list, the bit 8 is one, so bit
mask 0xff00 should be used.  Current condition is a constant false.
Regression tested on arm-linux-gnueabi.  Patch is pushed in.

gdb:

2014-09-24  Yao Qi  <yao@codesourcery.com>

	* arm-tdep.c (thumb_in_function_epilogue_p): Fix typo in the
	bitmask.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 742053f..5dccf0a 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -3273,7 +3273,7 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 	found_return = 1;
       else if (thumb_instruction_restores_sp (insn))
 	{
-	  if ((insn & 0xfe00) == 0xbd00)  /* pop <registers, PC> */
+	  if ((insn & 0xff00) == 0xbd00)  /* pop <registers, PC> */
 	    found_return = 1;
 	}
       else if (thumb_insn_size (insn) == 4)  /* 32-bit Thumb-2 instruction */

[PATCH 3/4] Stop prologue analysis when past the epilogue

[Yao Qi]

We see a fail in gdb.trace/entry-values.exp on armv4t thumb,

bt^M
#0  0x000086fc in foo (i=0, i@entry=<optimized out>, j=2, j@entry=<optimized out>)^M
#1  0x00000002 in ?? ()^M
Backtrace stopped: previous frame identical to this frame (corrupt stack?)^M
(gdb) FAIL: gdb.trace/entry-values.exp: bt (1) (pattern 1)

The fail is caused by incorrect prologue analysis, which can be illustrated by
setting a breakpoint on function foo,

(gdb) disassemble foo
Dump of assembler code for function foo:
   0x000086e8 <+0>:	push	{r7, lr}
   0x000086ea <+2>:	sub	sp, #8
   0x000086ec <+4>:	add	r7, sp, #0
   0x000086ee <+6>:	str	r0, [r7, #4]
   0x000086f0 <+8>:	str	r1, [r7, #0]
   0x000086f2 <+10>:	movs	r3, #0
   0x000086f4 <+12>:	adds	r0, r3, #0
   0x000086f6 <+14>:	mov	sp, r7
   0x000086f8 <+16>:	add	sp, #8
   0x000086fa <+18>:	pop	{r7}
   0x000086fc <+20>:	pop	{r1}
   0x000086fe <+22>:	bx	r1
End of assembler dump.
(gdb) b foo
Breakpoint 1 at 0x86fc

As we can see, GDB analyzes the prologue and skip the prologue to the last
instruction but one.  The breakpoint is set within the epilogue, and GDB
skips too many instruction for prologue.  This patch teaches GDB to stop
prologue analysis when goes into the epilogue.  With this patch applied,
GDB is able to unwind correctly,

(gdb) bt
#0  0x000086f6 in foo (i=0, i@entry=2, j=2, j@entry=3)
#1  0x00008718 in bar (i=<optimized out>)
#2  0x00008758 in main ()

gdb:

2014-07-02  Yao Qi  <yao@codesourcery.com>

	* arm-tdep.c (thumb_analyze_prologue): Break the loop if
	thumb_instruction_restores_sp return true.
---
 gdb/arm-tdep.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 153ef42..72beeb1 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -754,6 +754,11 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
 	  regs[ARM_SP_REGNUM] = pv_add_constant (regs[ARM_SP_REGNUM],
 						 -offset);
 	}
+      else if (thumb_instruction_restores_sp (insn))
+	{
+	  /* Don't scan past the epilogue.  */
+	  break;
+	}
       else if ((insn & 0xf800) == 0xa800)	/* add Rd, sp, #imm */
 	regs[bits (insn, 8, 10)] = pv_add_constant (regs[ARM_SP_REGNUM],
 						    (insn & 0xff) << 2);
-- 
1.9.0

Re: [PATCH 3/4] Stop prologue analysis when past the epilogue

[Will Newton]

On 3 July 2014 07:09, Yao Qi <yao@codesourcery.com> wrote:
> We see a fail in gdb.trace/entry-values.exp on armv4t thumb,
>
> bt^M
> #0  0x000086fc in foo (i=0, i@entry=<optimized out>, j=2, j@entry=<optimized out>)^M
> #1  0x00000002 in ?? ()^M
> Backtrace stopped: previous frame identical to this frame (corrupt stack?)^M
> (gdb) FAIL: gdb.trace/entry-values.exp: bt (1) (pattern 1)
>
> The fail is caused by incorrect prologue analysis, which can be illustrated by
> setting a breakpoint on function foo,
>
> (gdb) disassemble foo
> Dump of assembler code for function foo:
>    0x000086e8 <+0>:     push    {r7, lr}
>    0x000086ea <+2>:     sub     sp, #8
>    0x000086ec <+4>:     add     r7, sp, #0
>    0x000086ee <+6>:     str     r0, [r7, #4]
>    0x000086f0 <+8>:     str     r1, [r7, #0]
>    0x000086f2 <+10>:    movs    r3, #0
>    0x000086f4 <+12>:    adds    r0, r3, #0
>    0x000086f6 <+14>:    mov     sp, r7
>    0x000086f8 <+16>:    add     sp, #8
>    0x000086fa <+18>:    pop     {r7}
>    0x000086fc <+20>:    pop     {r1}
>    0x000086fe <+22>:    bx      r1
> End of assembler dump.
> (gdb) b foo
> Breakpoint 1 at 0x86fc
>
> As we can see, GDB analyzes the prologue and skip the prologue to the last
> instruction but one.  The breakpoint is set within the epilogue, and GDB
> skips too many instruction for prologue.  This patch teaches GDB to stop
> prologue analysis when goes into the epilogue.  With this patch applied,
> GDB is able to unwind correctly,
>
> (gdb) bt
> #0  0x000086f6 in foo (i=0, i@entry=2, j=2, j@entry=3)
> #1  0x00008718 in bar (i=<optimized out>)
> #2  0x00008758 in main ()
>
> gdb:
>
> 2014-07-02  Yao Qi  <yao@codesourcery.com>
>
>         * arm-tdep.c (thumb_analyze_prologue): Break the loop if
>         thumb_instruction_restores_sp return true.
> ---
>  gdb/arm-tdep.c | 5 +++++
>  1 file changed, 5 insertions(+)

This patch looks good to me too.

> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 153ef42..72beeb1 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -754,6 +754,11 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
>           regs[ARM_SP_REGNUM] = pv_add_constant (regs[ARM_SP_REGNUM],
>                                                  -offset);
>         }
> +      else if (thumb_instruction_restores_sp (insn))
> +       {
> +         /* Don't scan past the epilogue.  */
> +         break;
> +       }
>        else if ((insn & 0xf800) == 0xa800)      /* add Rd, sp, #imm */
>         regs[bits (insn, 8, 10)] = pv_add_constant (regs[ARM_SP_REGNUM],
>                                                     (insn & 0xff) << 2);
> --
> 1.9.0
>



-- 
Will Newton
Toolchain Working Group, Linaro

Re: [PATCH 3/4] Stop prologue analysis when past the epilogue

[Joel Brobecker]

> 2014-07-02  Yao Qi  <yao@codesourcery.com>
> 
> 	* arm-tdep.c (thumb_analyze_prologue): Break the loop if
> 	thumb_instruction_restores_sp return true.

OK to push.

-- 
Joel

[PATCH 4/4] Fix gdb.trace/entry-values.exp for thumb mode

[Yao Qi]

We see some fails in gdb.trace/entry-values.exp in thumb mode
(-mthumb -march={armv4t,armv7-a}).

In thumb mode, the lsb of references to 'foo' and 'bar' in the assembly
(produced by dwarf assember) is set, so the generated debug
information is incorrect.

This patch copies the approach used by

  [PATCH 4/4] Fix dw2-ifort-parameter.exp on PPC64
  https://sourceware.org/ml/gdb-patches/2014-03/msg00202.html

to introduce new labels 'foo_start' and 'bar_start' which are about
the correct function address (without lsb set).  This patch fixes
these fails we've seen.  This should also fix the fails on PPC64, but
I didn't test.

gdb/testsuite:

2014-07-02  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/entry-values.c: Define labels 'foo_start' and
	'bar_start' at the beginning of functions 'foo' and 'bar'
	respectively.
	* gdb.trace/entry-values.exp: Use 'foo_start' and 'bar_start'
	instead of 'foo' and 'bar'.
---
 gdb/testsuite/gdb.trace/entry-values.c   |  6 ++++++
 gdb/testsuite/gdb.trace/entry-values.exp | 10 +++++-----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/entry-values.c b/gdb/testsuite/gdb.trace/entry-values.c
index 4f80eb0..11bb739 100644
--- a/gdb/testsuite/gdb.trace/entry-values.c
+++ b/gdb/testsuite/gdb.trace/entry-values.c
@@ -15,12 +15,18 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
+asm (".section	\".text\"");
+asm (".balign 8");
+asm ("foo_start: .globl foo_start");
+
 int
 foo (int i, int j)
 {
   return 0;
 }
 
+asm ("bar_start: .globl bar_start");
+
 int
 bar (int i)
 {
diff --git a/gdb/testsuite/gdb.trace/entry-values.exp b/gdb/testsuite/gdb.trace/entry-values.exp
index f10ffa6..3ccb4d2 100644
--- a/gdb/testsuite/gdb.trace/entry-values.exp
+++ b/gdb/testsuite/gdb.trace/entry-values.exp
@@ -145,8 +145,8 @@ Dwarf::assemble $asm_file {
 	    foo_label: subprogram {
 		{name foo}
 		{decl_file 1}
-		{low_pc foo addr}
-		{high_pc "foo + $foo_length" addr}
+		{low_pc foo_start addr}
+		{high_pc "foo_start + $foo_length" addr}
 	    } {
 		formal_parameter {
 		    {type :$int_label}
@@ -163,8 +163,8 @@ Dwarf::assemble $asm_file {
 	    subprogram {
 		{name bar}
 		{decl_file 1}
-		{low_pc bar addr}
-		{high_pc "bar + $bar_length" addr}
+		{low_pc bar_start addr}
+		{high_pc "bar_start + $bar_length" addr}
 		{GNU_all_call_sites 1}
 	    } {
 		formal_parameter {
@@ -173,7 +173,7 @@ Dwarf::assemble $asm_file {
 		}
 
 		GNU_call_site {
-		    {low_pc "bar + $bar_call_foo" addr}
+		    {low_pc "bar_start + $bar_call_foo" addr}
 		    {abstract_origin :$foo_label}
 		} {
 		    # Faked entry values are reference to variables 'global1'
-- 
1.9.0

Re: [PATCH 4/4] Fix gdb.trace/entry-values.exp for thumb mode

[Joel Brobecker]

> 2014-07-02  Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.trace/entry-values.c: Define labels 'foo_start' and
> 	'bar_start' at the beginning of functions 'foo' and 'bar'
> 	respectively.
> 	* gdb.trace/entry-values.exp: Use 'foo_start' and 'bar_start'
> 	instead of 'foo' and 'bar'.

This is OK.


Thank you,
-- 
Joel

Re: [PATCH 4/4] Fix gdb.trace/entry-values.exp for thumb mode

[Yao Qi]

On 07/07/2014 11:15 PM, Joel Brobecker wrote:
>> 2014-07-02  Yao Qi  <yao@codesourcery.com>
>>
>> 	* gdb.trace/entry-values.c: Define labels 'foo_start' and
>> 	'bar_start' at the beginning of functions 'foo' and 'bar'
>> 	respectively.
>> 	* gdb.trace/entry-values.exp: Use 'foo_start' and 'bar_start'
>> 	instead of 'foo' and 'bar'.
> 
> This is OK.
> 

Patch is pushed in.

-- 
Yao (齐尧)

[PATCH 1/4] Restrict matching add/sub sp, #imm

[Yao Qi]

Currently, GDB matches both add/sub sp, #imm in prologue and epilogue,
which is not very precise.  On the instruction level, the immediate
number in both instruction can't be negative, so 'sub sp, #imm' only
appears in prologue while 'add sp, #imm' only appears in epilogue.
Note that on assembly level, we can write 'add sp, -8', but gas will
translate to 'sub sp, 8' instruction.

This patch is to only match 'sub sp, #imm' in prologue and match
'add sp, #immm' in epilogue.  It paves the way for the following
patch.

gdb:

2014-07-02  Yao Qi  <yao@codesourcery.com>

	* arm-tdep.c (thumb_analyze_prologue): Don't match instruction
	'add sp, #immm'.
	(thumb_in_function_epilogue_p): Don't match 'sub sp, #imm'.
---
 gdb/arm-tdep.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 8cc60a4..0fc7fc1 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -737,16 +737,11 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
 		pv_area_store (stack, regs[ARM_SP_REGNUM], 4, regs[regno]);
 	      }
 	}
-      else if ((insn & 0xff00) == 0xb000)	/* add sp, #simm  OR  
-						   sub sp, #simm */
+      else if ((insn & 0xff80) == 0xb080)	/* sub sp, #simm */
 	{
 	  offset = (insn & 0x7f) << 2;		/* get scaled offset */
-	  if (insn & 0x80)			/* Check for SUB.  */
-	    regs[ARM_SP_REGNUM] = pv_add_constant (regs[ARM_SP_REGNUM],
-						   -offset);
-	  else
-	    regs[ARM_SP_REGNUM] = pv_add_constant (regs[ARM_SP_REGNUM],
-						   offset);
+	  regs[ARM_SP_REGNUM] = pv_add_constant (regs[ARM_SP_REGNUM],
+						 -offset);
 	}
       else if ((insn & 0xf800) == 0xa800)	/* add Rd, sp, #imm */
 	regs[bits (insn, 8, 10)] = pv_add_constant (regs[ARM_SP_REGNUM],
@@ -3264,7 +3259,7 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 	found_return = 1;
       else if (insn == 0x46bd)  /* mov sp, r7 */
 	found_stack_adjust = 1;
-      else if ((insn & 0xff00) == 0xb000)  /* add sp, imm or sub sp, imm  */
+      else if ((insn & 0xff80) == 0xb000)  /* add sp, imm */
 	found_stack_adjust = 1;
       else if ((insn & 0xfe00) == 0xbc00)  /* pop <registers> */
 	{
@@ -3324,7 +3319,7 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 
       if (insn2 == 0x46bd)  /* mov sp, r7 */
 	found_stack_adjust = 1;
-      else if ((insn2 & 0xff00) == 0xb000)  /* add sp, imm or sub sp, imm  */
+      else if ((insn2 & 0xff80) == 0xb000)  /* add sp, imm */
 	found_stack_adjust = 1;
       else if ((insn2 & 0xff00) == 0xbc00)  /* pop <registers> without PC */
 	found_stack_adjust = 1;
-- 
1.9.0

Re: [PATCH 1/4] Restrict matching add/sub sp, #imm

[Will Newton]

On 3 July 2014 07:09, Yao Qi <yao@codesourcery.com> wrote:
> Currently, GDB matches both add/sub sp, #imm in prologue and epilogue,
> which is not very precise.  On the instruction level, the immediate
> number in both instruction can't be negative, so 'sub sp, #imm' only
> appears in prologue while 'add sp, #imm' only appears in epilogue.
> Note that on assembly level, we can write 'add sp, -8', but gas will
> translate to 'sub sp, 8' instruction.
>
> This patch is to only match 'sub sp, #imm' in prologue and match
> 'add sp, #immm' in epilogue.  It paves the way for the following
> patch.
>
> gdb:
>
> 2014-07-02  Yao Qi  <yao@codesourcery.com>
>
>         * arm-tdep.c (thumb_analyze_prologue): Don't match instruction
>         'add sp, #immm'.

One too many ms?

>         (thumb_in_function_epilogue_p): Don't match 'sub sp, #imm'.
> ---
>  gdb/arm-tdep.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 8cc60a4..0fc7fc1 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -737,16 +737,11 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
>                 pv_area_store (stack, regs[ARM_SP_REGNUM], 4, regs[regno]);
>               }
>         }
> -      else if ((insn & 0xff00) == 0xb000)      /* add sp, #simm  OR
> -                                                  sub sp, #simm */
> +      else if ((insn & 0xff80) == 0xb080)      /* sub sp, #simm */

I wonder if we should adjust the comment to just #imm, as #simm
implies it is a signed quantity.

>         {
>           offset = (insn & 0x7f) << 2;          /* get scaled offset */
> -         if (insn & 0x80)                      /* Check for SUB.  */
> -           regs[ARM_SP_REGNUM] = pv_add_constant (regs[ARM_SP_REGNUM],
> -                                                  -offset);
> -         else
> -           regs[ARM_SP_REGNUM] = pv_add_constant (regs[ARM_SP_REGNUM],
> -                                                  offset);
> +         regs[ARM_SP_REGNUM] = pv_add_constant (regs[ARM_SP_REGNUM],
> +                                                -offset);
>         }
>        else if ((insn & 0xf800) == 0xa800)      /* add Rd, sp, #imm */
>         regs[bits (insn, 8, 10)] = pv_add_constant (regs[ARM_SP_REGNUM],
> @@ -3264,7 +3259,7 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>         found_return = 1;
>        else if (insn == 0x46bd)  /* mov sp, r7 */
>         found_stack_adjust = 1;
> -      else if ((insn & 0xff00) == 0xb000)  /* add sp, imm or sub sp, imm  */
> +      else if ((insn & 0xff80) == 0xb000)  /* add sp, imm */
>         found_stack_adjust = 1;
>        else if ((insn & 0xfe00) == 0xbc00)  /* pop <registers> */
>         {
> @@ -3324,7 +3319,7 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>
>        if (insn2 == 0x46bd)  /* mov sp, r7 */
>         found_stack_adjust = 1;
> -      else if ((insn2 & 0xff00) == 0xb000)  /* add sp, imm or sub sp, imm  */
> +      else if ((insn2 & 0xff80) == 0xb000)  /* add sp, imm */
>         found_stack_adjust = 1;
>        else if ((insn2 & 0xff00) == 0xbc00)  /* pop <registers> without PC */
>         found_stack_adjust = 1;

Otherwise the patch looks good to me.

-- 
Will Newton
Toolchain Working Group, Linaro

Re: [PATCH 1/4] Restrict matching add/sub sp, #imm

[Yao Qi]

On 07/03/2014 04:31 PM, Will Newton wrote:
> One too many ms?
> 

Oh, sorry.

>> >         (thumb_in_function_epilogue_p): Don't match 'sub sp, #imm'.
>> > ---
>> >  gdb/arm-tdep.c | 15 +++++----------
>> >  1 file changed, 5 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>> > index 8cc60a4..0fc7fc1 100644
>> > --- a/gdb/arm-tdep.c
>> > +++ b/gdb/arm-tdep.c
>> > @@ -737,16 +737,11 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
>> >                 pv_area_store (stack, regs[ARM_SP_REGNUM], 4, regs[regno]);
>> >               }
>> >         }
>> > -      else if ((insn & 0xff00) == 0xb000)      /* add sp, #simm  OR
>> > -                                                  sub sp, #simm */
>> > +      else if ((insn & 0xff80) == 0xb080)      /* sub sp, #simm */
> I wonder if we should adjust the comment to just #imm, as #simm
> implies it is a signed quantity.
> 

Fixed.

-- 
Yao (齐尧)

gdb:

2014-07-07  Yao Qi  <yao@codesourcery.com>

	* arm-tdep.c (thumb_analyze_prologue): Don't match instruction
	'add sp, #imm'.
	(thumb_in_function_epilogue_p): Don't match 'sub sp, #imm'.
---
 gdb/arm-tdep.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 8cc60a4..6b1cf3c 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -737,16 +737,11 @@ thumb_analyze_prologue (struct gdbarch *gdbarch,
 		pv_area_store (stack, regs[ARM_SP_REGNUM], 4, regs[regno]);
 	      }
 	}
-      else if ((insn & 0xff00) == 0xb000)	/* add sp, #simm  OR  
-						   sub sp, #simm */
+      else if ((insn & 0xff80) == 0xb080)	/* sub sp, #imm */
 	{
 	  offset = (insn & 0x7f) << 2;		/* get scaled offset */
-	  if (insn & 0x80)			/* Check for SUB.  */
-	    regs[ARM_SP_REGNUM] = pv_add_constant (regs[ARM_SP_REGNUM],
-						   -offset);
-	  else
-	    regs[ARM_SP_REGNUM] = pv_add_constant (regs[ARM_SP_REGNUM],
-						   offset);
+	  regs[ARM_SP_REGNUM] = pv_add_constant (regs[ARM_SP_REGNUM],
+						 -offset);
 	}
       else if ((insn & 0xf800) == 0xa800)	/* add Rd, sp, #imm */
 	regs[bits (insn, 8, 10)] = pv_add_constant (regs[ARM_SP_REGNUM],
@@ -3264,7 +3259,7 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 	found_return = 1;
       else if (insn == 0x46bd)  /* mov sp, r7 */
 	found_stack_adjust = 1;
-      else if ((insn & 0xff00) == 0xb000)  /* add sp, imm or sub sp, imm  */
+      else if ((insn & 0xff80) == 0xb000)  /* add sp, imm */
 	found_stack_adjust = 1;
       else if ((insn & 0xfe00) == 0xbc00)  /* pop <registers> */
 	{
@@ -3324,7 +3319,7 @@ thumb_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 
       if (insn2 == 0x46bd)  /* mov sp, r7 */
 	found_stack_adjust = 1;
-      else if ((insn2 & 0xff00) == 0xb000)  /* add sp, imm or sub sp, imm  */
+      else if ((insn2 & 0xff80) == 0xb000)  /* add sp, imm */
 	found_stack_adjust = 1;
       else if ((insn2 & 0xff00) == 0xbc00)  /* pop <registers> without PC */
 	found_stack_adjust = 1;
-- 
1.9.0

Re: [PATCH 1/4] Restrict matching add/sub sp, #imm

[Joel Brobecker]

> 2014-07-07  Yao Qi  <yao@codesourcery.com>
> 
> 	* arm-tdep.c (thumb_analyze_prologue): Don't match instruction
> 	'add sp, #imm'.
> 	(thumb_in_function_epilogue_p): Don't match 'sub sp, #imm'.

Sorry for the delay, Yao. The updated version is OK to push.


-- 
Joel

Re: [PATCH 1/4] Restrict matching add/sub sp, #imm

[Yao Qi]

On 07/11/2014 09:25 PM, Joel Brobecker wrote:
> Sorry for the delay, Yao. The updated version is OK to push.

Thanks for the review, Joel.  All of them are pushed in.

-- 
Yao (齐尧)

Re: [PATCH 0/4] Fix gdb.trace/entry-values.exp fails in thumb mode

[Yao Qi]

On 07/03/2014 02:09 PM, Yao Qi wrote:
> We see some fails in gdb.trace/entry-values.exp in thumb mode
> (-mthumb -march={armv4t,armv7-a}), which are caused by two problems in
> gdb,
> 
>  - prologue analysis in gdb for thumb code is broken.  Patch #3 is to
>    fix it by stopping prologue analysis when it goes to the epilogue.
>  - dwarf assembler in gdb.trace/entry-values.exp uses functions whose
>    address's bit 0 is set, which is incorrect.  Patch #4 is to fix it.
> 
> Patch 1 is preparatory patch and patch 2 is a refactor one.  Each of them
> is regression tested on arm-none-linux-gnueabi.

Ping.  Patch 4 was approved by Joel and was pushed in, but patches 1~3
still need approval.

-- 
Yao (齐尧)