public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: "Andrew Burgess (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>
To: Luis Machado <luis.machado@linaro.org>, gdb-patches@sourceware.org
Subject: [review] [ARM, Thumb] Fix disassembling bug after reloading a symbol file
Date: Wed, 30 Oct 2019 14:01:00 -0000	[thread overview]
Message-ID: <20191030140042.3F7AD20AF6@gnutoolchain-gerrit.osci.io> (raw)
In-Reply-To: <gerrit.1572442722000.I22c3e6ebe9bfedad66d56fe9656994fa1761c485@gnutoolchain-gerrit.osci.io>

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?



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I22c3e6ebe9bfedad66d56fe9656994fa1761c485
Gerrit-Change-Number: 447
Gerrit-PatchSet: 1
Gerrit-Owner: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Comment-Date: Wed, 30 Oct 2019 14:00:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

  reply	other threads:[~2019-10-30 14:01 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-30 13:38 Luis Machado (Code Review)
2019-10-30 14:01 ` Andrew Burgess (Code Review) [this message]
2019-10-30 14:11 ` Luis Machado (Code Review)
2019-10-30 14:22 ` [review v2] " Luis Machado (Code Review)
2019-10-30 15:15 ` Tom Tromey (Code Review)
2019-10-30 15:22 ` Luis Machado (Code Review)
2019-10-30 15:59 ` [review v3] " Luis Machado (Code Review)
2019-10-31 16:44 ` [review v5] " Luis Machado (Code Review)
2019-10-31 16:45 ` Luis Machado (Code Review)
2019-11-01  1:09 ` Andrew Burgess (Code Review)
2019-11-01 13:14 ` [pushed] [ARM, thumb] " Sourceware to Gerrit sync (Code Review)
2019-11-01 13:14 ` Sourceware to Gerrit sync (Code Review)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191030140042.3F7AD20AF6@gnutoolchain-gerrit.osci.io \
    --to=gerrit@gnutoolchain-gerrit.osci.io \
    --cc=gdb-patches@sourceware.org \
    --cc=gnutoolchain-gerrit@osci.io \
    --cc=luis.machado@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).