From: Carl Love <cel@us.ibm.com>
To: Bruno Larsen <blarsen@redhat.com>, cel@us.ibm.com
Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
Ulrich Weigand <Ulrich.Weigand@de.ibm.com>,
Will Schmidt <will_schmidt@vnet.ibm.com>
Subject: RE: Question: [PATCH] Change calculation of frame_id by amd64 epilogue unwinder
Date: Wed, 02 Nov 2022 08:59:14 -0700 [thread overview]
Message-ID: <83ed9fdfcfdb6a578ff67ab5accc00d55b077ac5.camel@us.ibm.com> (raw)
In-Reply-To: <b717128d967cd09ead03d6860cd96187c1ed1b87.camel@us.ibm.com>
On Wed, 2022-11-02 at 08:36 -0700, Carl Love wrote:
> Bruno:
> > The idea of the patch looks fine, I just have a few style nits
> > inlined.
> >
> > > ---
> > > .../gdb.base/unwind-on-each-insn.exp | 25
> > > +++++++++++++++++--
> > > 1 file changed, 23 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> > > b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> > > index 3b48805cff8..c908a4b838e 100644
> > > --- a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> > > +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> > > @@ -41,7 +41,6 @@ if ![runto_main] then {
> > > proc get_sp_and_fba { testname } {
> > > with_test_prefix "get \$sp and frame base $testname" {
> > > set sp [get_hexadecimal_valueof "\$sp" "*UNKNOWN*"]
> > > -
> > > set fba ""
> > > gdb_test_multiple "info frame" "" {
> > > -re -wrap ".*Stack level ${::decimal}, frame at
> > > ($::hex):.*" {
> > > @@ -75,6 +74,23 @@ gdb_continue_to_breakpoint "enter foo"
> > >
> > > # Figure out the range of addresses covered by this function.
> > > set last_addr_in_foo ""
> > > +
> > > +# The disassembly of foo on PowerPC looks like:
> > > +# Dump of assembler code for function foo:
> > > +# => 0x00000000100006dc <+0>: std r31,-8(r1)
> > > +# 0x00000000100006e0 <+4>: stdu r1,-48(r1)
> > > +# 0x00000000100006e4 <+8>: mr r31,r1
> > > +# 0x00000000100006e8 <+12>: nop
> > > +# 0x00000000100006ec <+16>: addi r1,r31,48
> > > +# 0x00000000100006f0 <+20>: ld r31,-8(r1)
> > > +# 0x00000000100006f4 <+24>: blr
> > > +# 0x00000000100006f8 <+28>: .long 0x0
> > > +# 0x00000000100006fc <+32>: .long 0x0
> > > +# 0x0000000010000700 <+36>: .long 0x1000180
> > > +# End of assembler dump.
> > > +#
> > > +# The last instruction in function foo is blr. Ignore the .long
> > > +# entries following the blr instruction.
> > > gdb_test_multiple "disassemble foo" "" {
> > > -re "^disassemble foo\r\n" {
> > > exp_continue
> > > @@ -84,7 +100,11 @@ gdb_test_multiple "disassemble foo" "" {
> > > exp_continue
> > > }
> > >
> > > - -re "^...($hex) \[^\r\n\]+\r\n" {
> > > + -re "^...($hex) \[<>+0-
> > > 9:\s\t\]*\.long\[\s\t\]*\[^\r\n\]*\r\n"
> > > {
> >
> > I wonder if this pattern is unnecessarily strict. Correct me if
> > I'm
> > wrong, but I think that no architecture has an instruction that
> > starts
> > with a . so this pattern could probably be simplified to
> >
> > ^...($hex) \[<>+0-9:\s\t\]*\.[^\r\n\]*\r\n
> > And no other architectures would have a similar problem in the
> > future.
> >
> > I'm not very knowledgeable on assembly so you may know better in
> > this
> > area.
>
> I do not know of any architectures that have a dot at the beginning
> of
> an instruction name. So yes the pattern could be changed as
> suggested.
> That will require changing the above comment as well to make it clear
> we are skipping any "instruction" that starts with a dot, such as
> .long.
Well, I guess I should have tried the change before I sent the email!
Using just a dot in the pattern doesn't work as the pattern matches on
the period in the line "End of assembler dump." resulting in a failure
to find the statement for the end of the dump. Argh! I will leave the
pattern as it was to check for ".long".
Carl
prev parent reply other threads:[~2022-11-02 15:59 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-31 20:16 Carl Love
2022-11-01 10:20 ` Bruno Larsen
2022-11-01 19:51 ` Carl Love
2022-11-02 10:08 ` Bruno Larsen
2022-11-02 15:36 ` Carl Love
2022-11-02 15:59 ` Carl Love [this message]
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=83ed9fdfcfdb6a578ff67ab5accc00d55b077ac5.camel@us.ibm.com \
--to=cel@us.ibm.com \
--cc=Ulrich.Weigand@de.ibm.com \
--cc=blarsen@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=will_schmidt@vnet.ibm.com \
/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).