Fix ARM stepping over Thumb-mode "bx pc" or "blx pc"

Fix ARM stepping over Thumb-mode "bx pc" or "blx pc"

[Joseph S. Myers]

arm-tdep.c has code to determine the next instruction for use in
single stepping.  This code fails to handle a Thumb-mode "bx pc" or
"blx pc" correctly; it acts as if the branch target (four bytes after
the current instruction) should be in Thumb mode, when actually these
instructions switch to ARM mode (results being UNPREDICTABLE if the bx
instruction is not at a four-byte aligned address).  In particular,
this breaks stepping through PLT entries called from Thumb-mode code
(those start with a "bx pc; nop" to switch to ARM mode for the rest of
the PLT entry).

This patch fixes this by masking off the low bits from a value that
was computed with the low bit set (I followed another case that masks
off two low bits, though as noted above the results are UNPREDICTABLE
if the address has bit 1 set).  Tested with no regressions with cross
to arm-none-linux-gnueabi; the new test fails with unpatched GDB and
passes with patched GDB.  OK to commit?

2012-08-10  Joseph Myers  <joseph@codesourcery.com>

	* arm-tdep.c (thumb_get_next_pc_raw): Mask off low bits for bx pc
	and blx pc.

testsuite:
2012-08-10  Joseph Myers  <joseph@codesourcery.com>

	* gdb.arch/thumb-bx-pc.S: New file.
	* gdb.arch/thumb-bx-pc.exp: New file.

Index: gdb/arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.365
diff -u -r1.365 arm-tdep.c
--- gdb/arm-tdep.c	25 Jun 2012 12:32:45 -0000	1.365
+++ gdb/arm-tdep.c	10 Aug 2012 15:18:37 -0000
@@ -4541,7 +4541,7 @@
   else if ((inst1 & 0xff00) == 0x4700)	/* bx REG, blx REG */
     {
       if (bits (inst1, 3, 6) == 0x0f)
-	nextpc = pc_val;
+	nextpc = pc_val & 0xfffffffc;
       else
 	nextpc = get_frame_register_unsigned (frame, bits (inst1, 3, 6));
     }
Index: gdb/testsuite/gdb.arch/thumb-bx-pc.S
===================================================================
RCS file: gdb/testsuite/gdb.arch/thumb-bx-pc.S
diff -N gdb/testsuite/gdb.arch/thumb-bx-pc.S
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.arch/thumb-bx-pc.S	10 Aug 2012 15:18:38 -0000
@@ -0,0 +1,34 @@
+/* Test PC adjustment from Thumb-mode "bx pc" instruction.
+
+   Copyright 2012 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+	.syntax unified
+	.thumb
+	.text
+	.p2align 2
+	.global	main
+	.thumb
+	.thumb_func
+	.type main, %function
+main:
+	bx	pc
+	nop
+.code 32
+	mov	r0, #0
+	bx	lr
+	.size	main, .-main
Index: gdb/testsuite/gdb.arch/thumb-bx-pc.exp
===================================================================
RCS file: gdb/testsuite/gdb.arch/thumb-bx-pc.exp
diff -N gdb/testsuite/gdb.arch/thumb-bx-pc.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.arch/thumb-bx-pc.exp	10 Aug 2012 15:18:38 -0000
@@ -0,0 +1,41 @@
+# Copyright 2012 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test PC adjustment from Thumb-mode "bx pc" instruction.
+
+if {![istarget arm*-*]} then {
+    verbose "Skipping ARM tests."
+    return
+}
+
+set testfile "thumb-bx-pc"
+set srcfile ${testfile}.S
+set opts {}
+
+if [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} $opts] {
+    untested ${testfile}.exp
+    return -1
+}
+
+if ![runto_main] then {
+    untested ${testfile}.exp
+    return -1
+}
+
+gdb_test "stepi" "0x\[0-9a-fA-F\]+ in main \\(\\)" "stepi for bx pc"
+
+gdb_test "x /i \$pc" \
+    "0x\[0-9a-fA-F\]+ <main\\+4>:\[ \t\]+mov\[ \t\]+r0,\[ \t\]+#0.*" \
+    "stepi reached correct instruction"

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Fix ARM stepping over Thumb-mode "bx pc" or "blx pc"

[Yao Qi]

On 08/11/2012 01:06 AM, Joseph S. Myers wrote:
> arm-tdep.c has code to determine the next instruction for use in
> single stepping.  This code fails to handle a Thumb-mode "bx pc" or
> "blx pc" correctly; it acts as if the branch target (four bytes after
> the current instruction) should be in Thumb mode, when actually these

Yes, as we can see, at the beginning of thumb_get_next_pc_raw, the
'pc_val' is converted to THUMB_ADDR,

static CORE_ADDR
thumb_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc)
{
[...]
  pc_val = MAKE_THUMB_ADDR (pc_val)

> Index: gdb/arm-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/arm-tdep.c,v
> retrieving revision 1.365
> diff -u -r1.365 arm-tdep.c
> --- gdb/arm-tdep.c	25 Jun 2012 12:32:45 -0000	1.365
> +++ gdb/arm-tdep.c	10 Aug 2012 15:18:37 -0000
> @@ -4541,7 +4541,7 @@
>    else if ((inst1 & 0xff00) == 0x4700)	/* bx REG, blx REG */
>      {
>        if (bits (inst1, 3, 6) == 0x0f)
> -	nextpc = pc_val;
> +	nextpc = pc_val & 0xfffffffc;
>        else
>  	nextpc = get_frame_register_unsigned (frame, bits (inst1, 3, 6));
>      }

I don't have any preference on clearing either the last one bit in
address or the last two bits.  Looks like two ways coexist in arm-tdep.c
nowadays.  As 'pc_val' is set by MAKE_THUMB_ADDR at the beginning, it is
better to revert its change by using UNMAKE_THUMB_ADDR (which only
clears the last one bit of address).

-- 
Yao (齐尧)

Re: Fix ARM stepping over Thumb-mode "bx pc" or "blx pc"

[Joseph S. Myers]

On Sat, 11 Aug 2012, Yao Qi wrote:

> I don't have any preference on clearing either the last one bit in
> address or the last two bits.  Looks like two ways coexist in arm-tdep.c
> nowadays.  As 'pc_val' is set by MAKE_THUMB_ADDR at the beginning, it is
> better to revert its change by using UNMAKE_THUMB_ADDR (which only
> clears the last one bit of address).

This version uses UNMAKE_THUMB_ADDR as requested.  Tested with no
regressions with cross to arm-none-linux-gnueabi.

2012-08-13  Joseph Myers  <joseph@codesourcery.com>

	* arm-tdep.c (thumb_get_next_pc_raw): Mask off low bits for bx pc
	and blx pc.

testsuite:
2012-08-13  Joseph Myers  <joseph@codesourcery.com>

	* gdb.arch/thumb-bx-pc.S: New file.
	* gdb.arch/thumb-bx-pc.exp: New file.

Index: gdb/arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.365
diff -u -r1.365 arm-tdep.c
--- gdb/arm-tdep.c	25 Jun 2012 12:32:45 -0000	1.365
+++ gdb/arm-tdep.c	13 Aug 2012 13:56:04 -0000
@@ -4541,7 +4541,7 @@
   else if ((inst1 & 0xff00) == 0x4700)	/* bx REG, blx REG */
     {
       if (bits (inst1, 3, 6) == 0x0f)
-	nextpc = pc_val;
+	nextpc = UNMAKE_THUMB_ADDR (pc_val);
       else
 	nextpc = get_frame_register_unsigned (frame, bits (inst1, 3, 6));
     }
Index: gdb/testsuite/gdb.arch/thumb-bx-pc.S
===================================================================
RCS file: gdb/testsuite/gdb.arch/thumb-bx-pc.S
diff -N gdb/testsuite/gdb.arch/thumb-bx-pc.S
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.arch/thumb-bx-pc.S	13 Aug 2012 13:56:05 -0000
@@ -0,0 +1,34 @@
+/* Test PC adjustment from Thumb-mode "bx pc" instruction.
+
+   Copyright 2012 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+	.syntax unified
+	.thumb
+	.text
+	.p2align 2
+	.global	main
+	.thumb
+	.thumb_func
+	.type main, %function
+main:
+	bx	pc
+	nop
+.code 32
+	mov	r0, #0
+	bx	lr
+	.size	main, .-main
Index: gdb/testsuite/gdb.arch/thumb-bx-pc.exp
===================================================================
RCS file: gdb/testsuite/gdb.arch/thumb-bx-pc.exp
diff -N gdb/testsuite/gdb.arch/thumb-bx-pc.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.arch/thumb-bx-pc.exp	13 Aug 2012 13:56:05 -0000
@@ -0,0 +1,41 @@
+# Copyright 2012 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test PC adjustment from Thumb-mode "bx pc" instruction.
+
+if {![istarget arm*-*]} then {
+    verbose "Skipping ARM tests."
+    return
+}
+
+set testfile "thumb-bx-pc"
+set srcfile ${testfile}.S
+set opts {}
+
+if [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} $opts] {
+    untested ${testfile}.exp
+    return -1
+}
+
+if ![runto_main] then {
+    untested ${testfile}.exp
+    return -1
+}
+
+gdb_test "stepi" "0x\[0-9a-fA-F\]+ in main \\(\\)" "stepi for bx pc"
+
+gdb_test "x /i \$pc" \
+    "0x\[0-9a-fA-F\]+ <main\\+4>:\[ \t\]+mov\[ \t\]+r0,\[ \t\]+#0.*" \
+    "stepi reached correct instruction"

-- 
Joseph S. Myers
joseph@codesourcery.com

Ping Re: Fix ARM stepping over Thumb-mode "bx pc" or "blx pc"

[Joseph S. Myers]

On Mon, 13 Aug 2012, Joseph S. Myers wrote:

> On Sat, 11 Aug 2012, Yao Qi wrote:
> 
> > I don't have any preference on clearing either the last one bit in
> > address or the last two bits.  Looks like two ways coexist in arm-tdep.c
> > nowadays.  As 'pc_val' is set by MAKE_THUMB_ADDR at the beginning, it is
> > better to revert its change by using UNMAKE_THUMB_ADDR (which only
> > clears the last one bit of address).
> 
> This version uses UNMAKE_THUMB_ADDR as requested.  Tested with no
> regressions with cross to arm-none-linux-gnueabi.
> 
> 2012-08-13  Joseph Myers  <joseph@codesourcery.com>
> 
> 	* arm-tdep.c (thumb_get_next_pc_raw): Mask off low bits for bx pc
> 	and blx pc.
> 
> testsuite:
> 2012-08-13  Joseph Myers  <joseph@codesourcery.com>
> 
> 	* gdb.arch/thumb-bx-pc.S: New file.
> 	* gdb.arch/thumb-bx-pc.exp: New file.

Ping.  This patch 
<http://sourceware.org/ml/gdb-patches/2012-08/msg00368.html> is pending 
review.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Ping Re: Fix ARM stepping over Thumb-mode "bx pc" or "blx pc"

[Yao Qi]

On 08/20/2012 07:09 PM, Joseph S. Myers wrote:
>> >This version uses UNMAKE_THUMB_ADDR as requested.  Tested with no
>> >regressions with cross to arm-none-linux-gnueabi.
>> >
>> >2012-08-13  Joseph Myers<joseph@codesourcery.com>
>> >
>> >	* arm-tdep.c (thumb_get_next_pc_raw): Mask off low bits for bx pc
>> >	and blx pc.
>> >
>> >testsuite:
>> >2012-08-13  Joseph Myers<joseph@codesourcery.com>
>> >
>> >	* gdb.arch/thumb-bx-pc.S: New file.
>> >	* gdb.arch/thumb-bx-pc.exp: New file.
> Ping.  This patch
> <http://sourceware.org/ml/gdb-patches/2012-08/msg00368.html>  is pending
> review.

It is OK to me, but I can't approve it.  Could any GM give a review to 
this patch?

-- 
Yao

Re: Fix ARM stepping over Thumb-mode "bx pc" or "blx pc"

[Pedro Alves]

Hi Joseph,

I'm not an ARM expert, but this looks fine to me.  Go ahead.

On 08/13/2012 05:30 PM, Joseph S. Myers wrote:

> +if [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} $opts] {
> +    untested ${testfile}.exp

See <http://sourceware.org/gdb/wiki/GDBTestcaseCookbook>.

"In untested calls, please spell out the reason the test ends up untested, instead of just writing
 the test name, as with the latter we just end up with the test name duplicated in
 the gdb.sum output. For example: "

Something like:

  untested "Failed to compile $srcfile"

is sufficient.

> +    return -1
> +}
> +
> +if ![runto_main] then {
> +    untested ${testfile}.exp

  untested "could not run to main"

> +    return -1
> +}
> +
> +gdb_test "stepi" "0x\[0-9a-fA-F\]+ in main \\(\\)" "stepi for bx pc"

You can use dejagnu's $hex global instead of "0x\[0-9a-fA-F\]+" .

> +
> +gdb_test "x /i \$pc" \
> +    "0x\[0-9a-fA-F\]+ <main\\+4>:\[ \t\]+mov\[ \t\]+r0,\[ \t\]+#0.*" \
> +    "stepi reached correct instruction"
> 

Ditto.

-- 
Pedro Alves

Re: Fix ARM stepping over Thumb-mode "bx pc" or "blx pc"

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> +if [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} $opts] {
>> +    untested ${testfile}.exp

[...]
Pedro> Something like:
Pedro>   untested "Failed to compile $srcfile"
Pedro> is sufficient.

prepare_for_testing actually calls untested itself, via
build_executable_from_specs, e.g.:

    if  { [gdb_compile $objects "${binfile}" executable $options] != "" } {
        untested $testname
        return -1
    }

So, I've taken to just eliminating it in these cases.

It is an unfriendly-ish call there, but that is easy to fix.

Tom

Re: Ping Re: Fix ARM stepping over Thumb-mode "bx pc" or "blx pc"

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

>> Ping.  This patch
>> <http://sourceware.org/ml/gdb-patches/2012-08/msg00368.html>  is pending
>> review.

Yao> It is OK to me, but I can't approve it.  Could any GM give a review to
Yao> this patch?

Ok.

Thanks for looking at this, Yao.

Tom