From: Carl Love <cel@us.ibm.com>
To: Bruno Larsen <blarsen@redhat.com>,
gdb-patches@sourceware.org,
UlrichWeigand <Ulrich.Weigand@de.ibm.com>,
pedro@palves.net
Cc: luis.machado@arm.com, cel@us.ibm.com
Subject: RE: [PATCH v3] Fix reverse stepping multiple contiguous PC ranges over the line table.
Date: Wed, 10 May 2023 10:16:18 -0700 [thread overview]
Message-ID: <956b8c3c9a7bdc3aa6f9a040619ec4778edc9c94.camel@us.ibm.com> (raw)
In-Reply-To: <0a2c4ebd-f01d-4b96-1b13-25d7276056a5@redhat.com>
Bruno:
Thanks for the review. I addressed your comments as mentioned below.
I will post version 4 with the changes.
FYI, I will be out of the office from May 11 thru May 15. So will
reply to any additional comments when I return.
On Wed, 2023-05-10 at 15:47 +0200, Bruno Larsen wrote:
> On 04/05/2023 17:59, Carl Love wrote:
> > Bruno, GDB maintainers:
> >
> > Version 3, added the gcc version check as discussed further from
> > version 2 of the patch. Also updated the tests to check for
> > supporting
> >
<snip>
> > extern struct symtab *find_pc_line_symtab (CORE_ADDR);
> > diff --git a/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> > b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> > new file mode 100644
> > index 00000000000..412ab180943
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.reverse/func-map-to-same-line.c
> > @@ -0,0 +1,36 @@
> > +/* Copyright 2008-2023 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 L
<snip>
> > +
> > +require supports_reverse
> > +
> > +# This test uses the gcc no-column-info command which was added in
> > gcc 7.1.
> > +require get_compiler_info "gcc-7-*"
I put the compiler check in last. When I ran it, I obviously didn't
double check gdb/testsuite/gdb.log to make sure it really worked. I
normally try to make a point of double checking the log file. I have
been burned before thinking it was OK when there were no errors visible
on the command line. The above command fails if you check the log file.
>
> By constructing your regex like this, you are only allowing this test
> to
> be run on gcc 7. Anything later is also not accepted. I would do
> something like (Warning, untested)
>
> require get_compiler_info "gcc"
> require !get_compiler_info "gcc-[1-6]-*"
I couldn't get require to work like that. The get_compiler_info
doesn't seem take "gcc" as an argument.
I changed the test to:
if {![test_compiler_info {gcc-*}]
|| [test_compiler_info {gcc-[1-6]-*}]} {
return
}
With this, I do see the correct number of passes in
gdb/testsuite/gdb.log.
>
> Which requires gcc, but does not allow versions 1 to 6. There is
> probably a way to do it with a single require line, but I'm not the
> best
> with regexes.
>
> > +
> > +proc run_tests {msg} {
> > + global srcfile
> > + global executable
> > +
> > + runto_main
> > + set target_remote [gdb_is_target_remote]
>
> When probing for target remote, GDB will emit pass/fails with
> hardcoded
> names, so the current proc setup gives us some duplicated test names.
>
> I would suggest that, instead of passing a message as a parameter,
> you
> wrapped all function calls in a with_test_prefix scope, like:
>
> with_test_prefix "with-column-info" {
> run_test
> }
OK, I changed from passing in an argument and did the wrapped calls to
run_test instead. Note, this still didn't fix the duplicate test names
for turning on record.
>
> > +
> > + gdb_test_no_output "record" "turn on process record"
> > +
> > + # This regression test verifies the reverse-step and reverse-
> > next commands
> > + # work properly when executing backwards thru a source line
> > containing
> > + # two function calls on the same source line, i.e. func1 ();
> > func2 ();
> > + # This test is compiled so the dwarf info not contain the line
> > table
> > + # information.
> > +
> > + # Test 1, reverse-next command
> > + # Set breakpoint at the line after the function calls.
> > + set bp_start_reverse_test [gdb_get_line_number "START REVERSE
> > TEST" \
> > + $srcfile]
> > + gdb_breakpoint $srcfile:$bp_start_reverse_test temporary
> > +
> > + # Continue to break point for reverse-next test.
> > + # Command definition: reverse-next [count]
> > + # Run backward to the beginning of the previous line
> > executed in the
> > + # current (innermost) stack frame. If the line contains
> > function calls,
> > + # they will be “un-executed” without stopping. Starting from
> > the first
> > + # line of a function, reverse-next will take you back to the
> > caller of
> > + # that function, before the function was called, just as the
> > normal next
> > + # command would take you from the last line of a function
> > back to its
> > + # return to its caller 2 .
> > + gdb_continue_to_breakpoint \
> > + "$msg: test1: stopped at command reverse-next test start
> > location" \
> > + ".*$srcfile:$bp_start_reverse_test\r\n.*"
> > +
> > + # The reverse-next should step all the way back to the
> > beginning of the
> > + # line, i.e. at the beginning of the func1 call.
> > + gdb_test "reverse-next" ".*func1 \\(\\); func2 \\(\\);.*" \
> > + "$msg: test1: reverse-next to line with two functions"
> > +
> > + # We should be stopped at the first instruction of the line. A
> > reverse-step
> > + # should step back and stop at the beginning of the previous
> > line b = 2,
> > + # i.e. not in func1 ().
> > + gdb_test "reverse-stepi" ".*b = 2;.*" \
> > + "$msg: test1: reverse-stepi to previous line b = 2"
> > +
> > +
> > + # Setup for test 2
> > + clean_restart $executable
> > + runto_main
> > +
> > + gdb_test_no_output "record" "turn on process record"
>
> This gives a duplicate test name from setting up for the first test.
> Adding "test 2:(...)" in here solves it.
I couldn't figure out how to get the above syntax to work. So I used
the with_test_prefix instead, i.e.
with_test_prefix "test2" {
gdb_test_no_output "record" "turn on process record"
}
That fixed the duplicate names. I also wrapped the first record with
"test1" for consistency.
>
> > +
> > + # Test 2, reverse-step command
> > + # Set breakpoint at the line after the function calls.
> > + gdb_breakpoint $srcfile:$bp_start_reverse_test temporary
> > +
> >
<snip>
> > +# the same line to do the needed testing.
> > +
> > +# This test can only be run on targets which support DWARF-2 and
> > use gas.
> > +require dwarf2_support
> > +load_lib dwarf.exp
> the library has to be imported before the "require"
OK, switched the order of the lines.
> > +
> > +# The DWARF assembler requires the gcc compiler.
> > +require is_c_compiler_gcc
> > +
> > +# This test suitable only for process that can do reverse
> > execution
> > +requires supports_reverse
>
> s/requires/require
Fixed.
Carl
next prev parent reply other threads:[~2023-05-10 17:16 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-27 20:59 [PATCH] " Carl Love
2023-05-02 14:15 ` Bruno Larsen
2023-05-02 15:40 ` Carl Love
2023-05-02 15:42 ` Bruno Larsen
2023-05-11 15:11 ` Simon Marchi
2023-05-03 9:53 ` Bruno Larsen
2023-05-04 2:55 ` Carl Love
2023-05-04 9:24 ` Bruno Larsen
2023-05-04 14:52 ` Carl Love
2023-05-04 2:55 ` [PATCH v2] " Carl Love
2023-05-04 15:59 ` [PATCH v3] " Carl Love
2023-05-05 14:59 ` Luis Machado
2023-05-05 16:10 ` Carl Love
2023-05-10 13:47 ` Bruno Larsen
2023-05-10 17:16 ` Carl Love [this message]
2023-05-10 17:32 ` [PATCH v4] " Carl Love
2023-05-11 16:01 ` Simon Marchi
2023-05-11 16:23 ` Bruno Larsen
2023-05-11 17:28 ` Simon Marchi
2023-05-16 22:54 ` [PATCH 1/2] " Carl Love
2023-06-19 17:11 ` Simon Marchi
2023-06-22 16:52 ` Carl Love
2023-06-23 17:44 ` Simon Marchi
2023-06-23 19:41 ` Carl Love
2023-06-23 20:04 ` [PATCH 1/2 ver 2] " Carl Love
2023-07-06 15:07 ` Carl Love
2023-05-16 22:54 ` [PATCH 2/2 v5] " Carl Love
2023-05-25 15:08 ` Carl Love
2023-06-08 16:36 ` Carl Love
2023-06-19 17:58 ` Simon Marchi
2023-06-22 20:38 ` Carl Love
2023-06-22 20:39 ` Carl Love
2023-06-23 17:49 ` Simon Marchi
2023-06-23 20:04 ` Carl Love
2023-06-23 20:04 ` [PATCH 2/2 v6] " Carl Love
2023-05-16 22:54 ` [PATCH v4] " Carl Love
2023-05-11 7:52 ` [PATCH v3] " Bruno Larsen
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=956b8c3c9a7bdc3aa6f9a040619ec4778edc9c94.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=luis.machado@arm.com \
--cc=pedro@palves.net \
/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).