[review,ARM,Thumb] Fix disassembling bug after reloading a symbol file
Commit Message
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447
......................................................................
[ARM, Thumb] Fix disassembling bug after reloading a symbol file
The speed optimization from commit 5f6cac4085c95c5339b9549dc06d4f9184184fa6
made GDB skip reloading all symbols when the same symbol file is reloaded.
As a result, ARM targets only read the mapping symbols the first time we
load a symbol file. When reloaded, the speed optimization above will
cause an early return and gdbarch_record_special_symbol won't be called to
save mapping symbol data, which in turn affects disassembling of thumb
instructions.
First load and correct disassemble output:
Dump of assembler code for function main:
0x0000821c <+0>: bx pc
0x0000821e <+2>: nop
0x00008220 <+4>: mov r0, #0
0x00008224 <+8>: bx lr
Second load and incorrect disassemble output:
Dump of assembler code for function main:
0x0000821c <+0>: bx pc
0x0000821e <+2>: nop
0x00008220 <+4>: movs r0, r0
0x00008222 <+6>: b.n 0x8966
0x00008224 <+8>: vrhadd.u16 d14, d14, d31
This happens because the mapping symbol data is stored in an objfile_key-based
container, and that data isn't preserved across the two symbol loading
operations.
The following patch fixes this by storing the mapping symbol data in a
bfd_key-based container, which doesn't change as long as the bfd is the same.
I've also added a new test to verify the correct disassemble output.
OK?
gdb/ChangeLog:
2019-01-29 Luis Machado <luis.machado@linaro.org>
* arm-tdep.c (arm_per_objfile): Rename to ...
(arm_per_bfd): ... this.
(arm_objfile_data_key): Rename to ...
(arm_bfd_data_key): ... this.
(arm_find_mapping_symbol): Adjust access to new bfd_key-based
data.
(arm_record_special_symbol): Likewise.
gdb/testsuite/ChangeLog:
2019-01-29 Luis Machado <luis.machado@linaro.org>
* gdb.arch/pr25124.S: New file.
* gdb.arch/pr25124.exp: New file.
Signed-off-by: Luis Machado <luis.machado@linaro.org>
Change-Id: I22c3e6ebe9bfedad66d56fe9656994fa1761c485
---
M gdb/arm-tdep.c
A gdb/testsuite/gdb.arch/pr25124.S
A gdb/testsuite/gdb.arch/pr25124.exp
3 files changed, 86 insertions(+), 10 deletions(-)
Comments
Andrew Burgess has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447
......................................................................
Patch Set 1:
(3 comments)
Basically fine, few minor issues I think need addressing before it can be merged.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/1/gdb/arm-tdep.c
File gdb/arm-tdep.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/1/gdb/arm-tdep.c@101
PS1, Line 101:
92 | struct arm_per_bfd
93 | {
94 | explicit arm_per_bfd (size_t num_sections)
95 | : section_maps (new arm_mapping_symbol_vec[num_sections]),
96 | section_maps_sorted (new bool[num_sections] ())
97 | {}
98 |
99 | DISABLE_COPY_AND_ASSIGN (arm_per_bfd);
100 |
101 | /* Information about mapping symbols ($a, $d, $t) in the objfile.
Should this mention of objfile be changed to "bfd" or "bfd object"?
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/1/gdb/testsuite/gdb.arch/pr25124.S
File gdb/testsuite/gdb.arch/pr25124.S:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/1/gdb/testsuite/gdb.arch/pr25124.S@4
PS1, Line 4:
1 | /* Test proper disassembling of ARM thumb instructions when reloading a symbol
2 | file.
3 |
4 | Copyright 2012-2019 Free Software Foundation, Inc.
This year should be 2019 only unless (1) it's a copy of another file that was added in 2012, or (2) this patch was originally submitted in 2012.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/1/gdb/testsuite/gdb.arch/pr25124.exp
File gdb/testsuite/gdb.arch/pr25124.exp:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/1/gdb/testsuite/gdb.arch/pr25124.exp@33
PS1, Line 33:
24 | standard_testfile .S
25 |
26 | if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable debug] != "" } {
27 | untested "failed to compile"
28 | return -1
29 | }
30 |
31 | gdb_test "x /i main+8"
32 | "$hex <main\\+8>:\[ \t\]+bx\[ \t\]+lr"
33 | "disassemble thumb instruction (1st try)"
Maybe I'm going crazy, but doesn't like 31 and 32 need to end in a trailing backslash? The next call to gdb_test does, and then the third below again, no backslash. I don't have an ARM target to hand, but does this test run correctly like this?
Luis Machado has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447
......................................................................
Patch Set 1:
(3 comments)
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/1/gdb/arm-tdep.c
File gdb/arm-tdep.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/1/gdb/arm-tdep.c@101
PS1, Line 101:
92 | struct arm_per_bfd
93 | {
94 | explicit arm_per_bfd (size_t num_sections)
95 | : section_maps (new arm_mapping_symbol_vec[num_sections]),
96 | section_maps_sorted (new bool[num_sections] ())
97 | {}
98 |
99 | DISABLE_COPY_AND_ASSIGN (arm_per_bfd);
100 |
101 | /* Information about mapping symbols ($a, $d, $t) in the objfile.
> Should this mention of objfile be changed to "bfd" or "bfd object"?
I thought about it, but the symbols themselves are contained in the objfile itself, though we're storing additional information inferred from said symbols in the bfd container.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/1/gdb/testsuite/gdb.arch/pr25124.S
File gdb/testsuite/gdb.arch/pr25124.S:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/1/gdb/testsuite/gdb.arch/pr25124.S@4
PS1, Line 4:
1 | /* Test proper disassembling of ARM thumb instructions when reloading a symbol
2 | file.
3 |
4 | Copyright 2012-2019 Free Software Foundation, Inc.
> This year should be 2019 only unless (1) it's a copy of another file that was added in 2012, or (2) […]
This is in fact a copy of gdb.arch/thumb-bx-pc.S, hence why i left it that way.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/1/gdb/testsuite/gdb.arch/pr25124.exp
File gdb/testsuite/gdb.arch/pr25124.exp:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/1/gdb/testsuite/gdb.arch/pr25124.exp@33
PS1, Line 33:
24 | standard_testfile .S
25 |
26 | if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable debug] != "" } {
27 | untested "failed to compile"
28 | return -1
29 | }
30 |
31 | gdb_test "x /i main+8"
32 | "$hex <main\\+8>:\[ \t\]+bx\[ \t\]+lr"
33 | "disassemble thumb instruction (1st try)"
> Maybe I'm going crazy, but doesn't like 31 and 32 need to end in a trailing backslash? The next cal […]
They do. I borked it just before submission, having noticed the overly long line and breaking it into multiple lines. I'll fix it.
As for running it effectively, i haven't due to the lack of access to a box running arm 32-bit. It doesn't seem we have buildbots running those either.
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447
......................................................................
Patch Set 2:
(2 comments)
Thank you for the patch. I had one question that I think should
be answered before landing; plus one nit.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/2//COMMIT_MSG
Commit Message:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/2//COMMIT_MSG@62
PS2, Line 62:
53 | (arm_bfd_data_key): ... this.
54 | (arm_find_mapping_symbol): Adjust access to new bfd_key-based
55 | data.
56 | (arm_record_special_symbol): Likewise.
57 |
58 | gdb/testsuite/ChangeLog:
59 |
60 | 2019-01-29 Luis Machado <luis.machado@linaro.org>
61 |
62 | * gdb.arch/pr25124.S: New file.
If there's a PR open for this, then each ChangeLog should
mention it, like "PR arch/25124". This syntax lets the
commit script log the commit in bugzilla.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/2/gdb/arm-tdep.c
File gdb/arm-tdep.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/2/gdb/arm-tdep.c@117
PS2, Line 117:
108 | symbol value (address). */
109 | std::unique_ptr<arm_mapping_symbol_vec[]> section_maps;
110 |
111 | /* For each corresponding element of section_maps above, is this vector
112 | sorted. */
113 | std::unique_ptr<bool[]> section_maps_sorted;
114 | };
115 |
116 | /* Per-bfd data used for mapping symbols. */
117 | static bfd_key<arm_per_bfd> arm_bfd_data_key;
I wonder if `arm_exidx_data_key` needs this treatment as well?
Luis Machado has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447
......................................................................
Patch Set 2:
(2 comments)
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/2//COMMIT_MSG
Commit Message:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/2//COMMIT_MSG@62
PS2, Line 62:
53 | (arm_bfd_data_key): ... this.
54 | (arm_find_mapping_symbol): Adjust access to new bfd_key-based
55 | data.
56 | (arm_record_special_symbol): Likewise.
57 |
58 | gdb/testsuite/ChangeLog:
59 |
60 | 2019-01-29 Luis Machado <luis.machado@linaro.org>
61 |
62 | * gdb.arch/pr25124.S: New file.
> If there's a PR open for this, then each ChangeLog should […]
Great. I'll add it. Thanks for the catching that.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/2/gdb/arm-tdep.c
File gdb/arm-tdep.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447/2/gdb/arm-tdep.c@117
PS2, Line 117:
108 | symbol value (address). */
109 | std::unique_ptr<arm_mapping_symbol_vec[]> section_maps;
110 |
111 | /* For each corresponding element of section_maps above, is this vector
112 | sorted. */
113 | std::unique_ptr<bool[]> section_maps_sorted;
114 | };
115 |
116 | /* Per-bfd data used for mapping symbols. */
117 | static bfd_key<arm_per_bfd> arm_bfd_data_key;
> I wonder if `arm_exidx_data_key` needs this treatment as well?
I did go through it when doing the change. It seems a bit more complicated, and it is attached to an observer, but i think it makes sense to convert it too.
I'll make a separate patch for it, since, unlike this particular case, the exidx processing doesn't suffer from not getting updated when we reload the objfile.
Luis Machado has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447
......................................................................
Patch Set 5:
I've now updated the testcase, with the help of the tryserver, and made sure it is passing accordingly. It was erroring out before.
Andrew Burgess has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/447
......................................................................
Patch Set 5: Code-Review+2
LGTM.
Tom's question about arm_exidx_data_key is address by you other patch https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/448.
@@ -89,14 +89,14 @@
typedef std::vector<arm_mapping_symbol> arm_mapping_symbol_vec;
-struct arm_per_objfile
+struct arm_per_bfd
{
- explicit arm_per_objfile (size_t num_sections)
+ explicit arm_per_bfd (size_t num_sections)
: section_maps (new arm_mapping_symbol_vec[num_sections]),
section_maps_sorted (new bool[num_sections] ())
{}
- DISABLE_COPY_AND_ASSIGN (arm_per_objfile);
+ DISABLE_COPY_AND_ASSIGN (arm_per_bfd);
/* Information about mapping symbols ($a, $d, $t) in the objfile.
@@ -113,8 +113,8 @@
std::unique_ptr<bool[]> section_maps_sorted;
};
-/* Per-objfile data used for mapping symbols. */
-static objfile_key<arm_per_objfile> arm_objfile_data_key;
+/* Per-bfd data used for mapping symbols. */
+static bfd_key<arm_per_bfd> arm_bfd_data_key;
/* The list of available "set arm ..." and "show arm ..." commands. */
static struct cmd_list_element *setarmcmdlist = NULL;
@@ -350,7 +350,7 @@
sec = find_pc_section (memaddr);
if (sec != NULL)
{
- arm_per_objfile *data = arm_objfile_data_key.get (sec->objfile);
+ arm_per_bfd *data = arm_bfd_data_key.get (sec->objfile->obfd);
if (data != NULL)
{
unsigned int section_idx = sec->the_bfd_section->index;
@@ -8560,17 +8560,17 @@
asymbol *sym)
{
const char *name = bfd_asymbol_name (sym);
- struct arm_per_objfile *data;
+ struct arm_per_bfd *data;
struct arm_mapping_symbol new_map_sym;
gdb_assert (name[0] == '$');
if (name[1] != 'a' && name[1] != 't' && name[1] != 'd')
return;
- data = arm_objfile_data_key.get (objfile);
+ data = arm_bfd_data_key.get (objfile->obfd);
if (data == NULL)
- data = arm_objfile_data_key.emplace (objfile,
- objfile->obfd->section_count);
+ data = arm_bfd_data_key.emplace (objfile->obfd,
+ objfile->obfd->section_count);
arm_mapping_symbol_vec &map
= data->section_maps[bfd_asymbol_section (sym)->index];
new file mode 100644
@@ -0,0 +1,35 @@
+/* Test proper disassembling of ARM thumb instructions when reloading a symbol
+ file.
+
+ Copyright 2012-2019 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
new file mode 100644
@@ -0,0 +1,41 @@
+# Copyright 2019 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 proper disassembling of ARM thumb instructions when reloading a symbol
+# file.
+
+if {![is_aarch32_target]} then {
+ verbose "Skipping ARM tests."
+ return
+}
+
+standard_testfile .S
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable debug] != "" } {
+ untested "failed to compile"
+ return -1
+}
+
+gdb_test "x /i main+8"
+ "$hex <main\\+8>:\[ \t\]+bx\[ \t\]+lr"
+ "disassemble thumb instruction (1st try)"
+
+gdb_test "file $binfile" \
+ "Reading symbols from.*" \
+ "reload symbol file"
+
+gdb_test "x /i main+8"
+ "$hex <main\\+8>:\[ \t\]+bx\[ \t\]+lr"
+ "disassemble thumb instruction (2nd try)"