This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC v6] PR gdb/13860: make -interpreter-exec console "list" behave more like "list".
- From: Pedro Alves <palves at redhat dot com>
- To: gdb-patches at sourceware dot org
- Cc: Tom Tromey <tromey at redhat dot com>
- Date: Wed, 21 May 2014 23:16:40 +0100
- Subject: Re: [RFC v6] PR gdb/13860: make -interpreter-exec console "list" behave more like "list".
- Authentication-results: sourceware.org; auth=none
- References: <1393957974-4521-1-git-send-email-tromey at redhat dot com> <1393957974-4521-3-git-send-email-tromey at redhat dot com> <53286E76 dot 9080808 at redhat dot com>
On 03/18/2014 04:04 PM, Pedro Alves wrote:
> Looking again at this, I think there's a deeper issue here. This "list"
> is the first source listing after a breakpoint stop, and "set listsize 1".
> The breakpoint is at line 62. Why would we list line 57 (and only
> that line) ? The issue is with the line centering done too soon. We should
> be a little more lazy here, like in the patch below.
>
> Let me know what you think.
Tom told me off list he's OK with this version.
I've pushed it in.
Pedro Alves
>
> ---------
> [PATCH] PR gdb/13860: make -interpreter-exec console "list" behave
> more like "list".
>
> I noticed that "list" behaves differently in CLI vs MI. Particularly:
>
> $ ./gdb -nx -q ./testsuite/gdb.mi/mi-cli
> Reading symbols from /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.mi/mi-cli...done.
> (gdb) start
> Temporary breakpoint 1 at 0x40054d: file ../../../src/gdb/testsuite/gdb.mi/basics.c, line 62.
> Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.mi/mi-cli
>
> Temporary breakpoint 1, main () at ../../../src/gdb/testsuite/gdb.mi/basics.c:62
> 62 callee1 (2, "A string argument.", 3.5);
> (gdb) list
> 57 {
> 58 }
> 59
> 60 main ()
> 61 {
> 62 callee1 (2, "A string argument.", 3.5);
> 63 callee1 (2, "A string argument.", 3.5);
> 64
> 65 do_nothing (); /* Hello, World! */
> 66
> (gdb)
>
> Note the list started at line 57. IOW, the program stopped at line
> 62, and GDB centered the list on that.
>
> compare with:
>
> $ ./gdb -nx -q ./testsuite/gdb.mi/mi-cli -i=mi
> =thread-group-added,id="i1"
> ~"Reading symbols from /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.mi/mi-cli..."
> ~"done.\n"
> (gdb)
> start
> &"start\n"
> ...
> ~"\nTemporary breakpoint "
> ~"1, main () at ../../../src/gdb/testsuite/gdb.mi/basics.c:62\n"
> ~"62\t callee1 (2, \"A string argument.\", 3.5);\n"
> *stopped,reason="breakpoint-hit",disp="del",bkptno="1",frame={addr="0x000000000040054d",func="main",args=[],file="../../../src/gdb/testsuite/gdb.mi/basics.c",fullname="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/basics.c",line="62"},thread-id="1",stopped-threads="all",core="0"
> =breakpoint-deleted,id="1"
> (gdb)
> -interpreter-exec console list
> ~"62\t callee1 (2, \"A string argument.\", 3.5);\n"
> ~"63\t callee1 (2, \"A string argument.\", 3.5);\n"
> ~"64\t\n"
> ~"65\t do_nothing (); /* Hello, World! */\n"
> ~"66\t\n"
> ~"67\t callme (1);\n"
> ~"68\t callme (2);\n"
> ~"69\t\n"
> ~"70\t return 0;\n"
> ~"71\t}\n"
> ^done
> (gdb)
>
> Here the list starts at line 62, where the program was stopped.
>
> This happens because print_stack_frame, called from both normal_stop
> and mi_on_normal_stop, is the function responsible for setting the
> current sal from the selected frame, overrides the PRINT_WHAT
> argument, and only after that does it decide whether to center the
> current sal line or not, based on the overriden value, and it will
> always decide false.
>
> (The print_stack_frame call in mi_on_normal_stop is a little different
> from the call in normal_stop, in that it is an unconditional
> SRC_AND_LOC call. A future patch will make those uniform.)
>
> A previous version of this patch made MI uniform with CLI here, by
> making print_stack_frame also center when MI is active. That changed
> the output of a "list" command in mi-cli.exp, to expect line 57
> instead of 62, as per the example above.
>
> However, looking deeper, that list in question is the first "list"
> after the program stops, and right after the stop, before the "list",
> the test did "set listsize 1". Let's try the same thing with the CLI:
>
> (gdb) start
> 62 callee1 (2, "A string argument.", 3.5);
> (gdb) set listsize 1
> (gdb) list
> 57 {
>
> Huh, that's unexpected. Why the 57? It's because print_stack_frame,
> called in reaction to the breakpoint stop, expecting the next "list"
> to show 10 lines (the listsize at the time) around line 62, sets the
> lines listed range to 57-67 (62 +/- 5). If the user changes the
> listsize before "list", why would we still show that range? Looks
> bogus to me.
>
> So the fix for this whole issue should be delay trying to center the
> listing to until actually listing, so that the correct listsize can be
> taken into account. This makes MI and CLI uniform too, as it deletes
> the center code from print_stack_frame.
>
> A series of tests are added to list.exp to cover this. mi-cli.exp was
> after all correct all along, but it now gains an additional test that
> lists lines with listsize 10, to ensure the centering is consistent
> with CLI's.
>
> One related Python test changed related output -- it's a test that
> prints the line number after stopping for a breakpoint, similar to the
> new list.exp tests. Previously we'd print the stop line minus 5 (due
> to the premature centering), now we print the stop line. I think
> that's a good change.
>
> Tested on x86_64 Fedora 17.
>
> gdb/
> 2014-03-18 Pedro Alves <palves@redhat.com>
>
> * cli/cli-cmds.c (list_command): Handle the first "list" after the
> current source line having changed.
> * frame.h (set_current_sal_from_frame): Remove 'center' parameter.
> * infrun.c (normal_stop): Adjust call to
> set_current_sal_from_frame.
> * source.c (clear_lines_listed_range): New function.
> (set_current_source_symtab_and_line, identify_source_line): Clear
> the lines listed range.
> (line_info): Handle the first "info line" after the current source
> line having changed.
> * stack.c (print_stack_frame): Remove center handling.
> (set_current_sal_from_frame): Remove 'center' parameter. Don't
> center sal.line.
>
> gdb/testsuite/
> 2014-03-18 Pedro Alves <palves@redhat.com>
>
> * gdb.base/list.exp (build_pattern, test_list): New procedures.
> Use them to test variations of "list" after reaching a breakpoint.
> * gdb.mi/mi-cli.exp (line_main_callme_2): New global.
> Test "list" with listsize 10 after reaching a breakpoint.
> * gdb.python/python.exp (decode_line current location line
> number): Adjust expected line number.
> ---
> gdb/cli/cli-cmds.c | 21 +++++++++
> gdb/frame.h | 5 +-
> gdb/infrun.c | 2 +-
> gdb/source.c | 26 ++++++++--
> gdb/source.h | 7 +--
> gdb/stack.c | 14 ++----
> gdb/testsuite/gdb.base/list.exp | 94 +++++++++++++++++++++++++++++++++++++
> gdb/testsuite/gdb.mi/mi-cli.exp | 20 ++++++++
> gdb/testsuite/gdb.python/python.exp | 5 +-
> 9 files changed, 171 insertions(+), 23 deletions(-)
>
> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
> index bfcd975..5c6129d 100644
> --- a/gdb/cli/cli-cmds.c
> +++ b/gdb/cli/cli-cmds.c
> @@ -874,6 +874,27 @@ list_command (char *arg, int from_tty)
> {
> set_default_source_symtab_and_line ();
> cursal = get_current_source_symtab_and_line ();
> +
> + /* If this is the first "list" since we've set the current
> + source line, center the listing around that line. */
> + if (get_first_line_listed () == 0)
> + {
> + int first;
> +
> + first = max (cursal.line - get_lines_to_list () / 2, 1);
> +
> + /* A small special case --- if listing backwards, and we
> + should list only one line, list the preceding line,
> + instead of the exact line we've just shown after e.g.,
> + stopping for a breakpoint. */
> + if (arg != NULL && arg[0] == '-'
> + && get_lines_to_list () == 1 && first > 1)
> + first -= 1;
> +
> + print_source_lines (cursal.symtab, first,
> + first + get_lines_to_list (), 0);
> + return;
> + }
> }
>
> /* "l" or "l +" lists next ten lines. */
> diff --git a/gdb/frame.h b/gdb/frame.h
> index e451a93..6a75f0d 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -388,10 +388,9 @@ extern void find_frame_sal (struct frame_info *frame,
> struct symtab_and_line *sal);
>
> /* Set the current source and line to the location given by frame
> - FRAME, if possible. When CENTER is true, adjust so the relevant
> - line is in the center of the next 'list'. */
> + FRAME, if possible. */
>
> -void set_current_sal_from_frame (struct frame_info *, int);
> +void set_current_sal_from_frame (struct frame_info *);
>
> /* Return the frame base (what ever that is) (DEPRECATED).
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index b7c0969..3d3ba55 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -6091,7 +6091,7 @@ normal_stop (void)
> display the frame below, but the current SAL will be incorrect
> during a user hook-stop function. */
> if (has_stack_frames () && !stop_stack_dummy)
> - set_current_sal_from_frame (get_current_frame (), 1);
> + set_current_sal_from_frame (get_current_frame ());
>
> /* Let the user/frontend see the threads as stopped. */
> do_cleanups (old_chain);
> diff --git a/gdb/source.c b/gdb/source.c
> index c112765..c77a4f4 100644
> --- a/gdb/source.c
> +++ b/gdb/source.c
> @@ -133,7 +133,9 @@ show_filename_display_string (struct ui_file *file, int from_tty,
>
> static int last_line_listed;
>
> -/* First line number listed by last listing command. */
> +/* First line number listed by last listing command. If 0, then no
> + source lines have yet been listed since the last time the current
> + source line was changed. */
>
> static int first_line_listed;
>
> @@ -153,6 +155,16 @@ get_first_line_listed (void)
> return first_line_listed;
> }
>
> +/* Clear line listed range. This makes the next "list" center the
> + printed source lines around the current source line. */
> +
> +static void
> +clear_lines_listed_range (void)
> +{
> + first_line_listed = 0;
> + last_line_listed = 0;
> +}
> +
> /* Return the default number of lines to print with commands like the
> cli "list". The caller of print_source_lines must use this to
> calculate the end line and use it in the call to print_source_lines
> @@ -220,6 +232,9 @@ set_current_source_symtab_and_line (const struct symtab_and_line *sal)
> current_source_symtab = sal->symtab;
> current_source_line = sal->line;
>
> + /* Force the next "list" to center around the current line. */
> + clear_lines_listed_range ();
> +
> return cursal;
> }
>
> @@ -1294,9 +1309,8 @@ identify_source_line (struct symtab *s, int line, int mid_statement,
> mid_statement, get_objfile_arch (s->objfile), pc);
>
> current_source_line = line;
> - first_line_listed = line;
> - last_line_listed = line;
> current_source_symtab = s;
> + clear_lines_listed_range ();
> return 1;
> }
>
> @@ -1488,7 +1502,11 @@ line_info (char *arg, int from_tty)
> {
> sal.symtab = current_source_symtab;
> sal.pspace = current_program_space;
> - sal.line = last_line_listed;
> + if (last_line_listed != 0)
> + sal.line = last_line_listed;
> + else
> + sal.line = current_source_line;
> +
> sals.nelts = 1;
> sals.sals = (struct symtab_and_line *)
> xmalloc (sizeof (struct symtab_and_line));
> diff --git a/gdb/source.h b/gdb/source.h
> index 61826ea..ad74df9 100644
> --- a/gdb/source.h
> +++ b/gdb/source.h
> @@ -62,9 +62,10 @@ extern const char *symtab_to_filename_for_display (struct symtab *symtab);
> lines. */
> extern void find_source_lines (struct symtab *s, int desc);
>
> -/* Return the first line listed by print_source_lines.
> - Used by command interpreters to request listing from
> - a previous point. */
> +/* Return the first line listed by print_source_lines. Used by
> + command interpreters to request listing from a previous point. If
> + 0, then no source lines have yet been listed since the last time
> + the current source line was changed. */
> extern int get_first_line_listed (void);
>
> /* Return the default number of lines to print with commands like the
> diff --git a/gdb/stack.c b/gdb/stack.c
> index da7d977..0c6923d 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -166,12 +166,10 @@ print_stack_frame (struct frame_info *frame, int print_level,
>
> TRY_CATCH (e, RETURN_MASK_ERROR)
> {
> - int center = (print_what == SRC_LINE || print_what == SRC_AND_LOC);
> -
> print_frame_info (frame, print_level, print_what, 1 /* print_args */,
> set_current_sal);
> if (set_current_sal)
> - set_current_sal_from_frame (frame, center);
> + set_current_sal_from_frame (frame);
> }
> }
>
> @@ -716,17 +714,13 @@ print_frame_args (struct symbol *func, struct frame_info *frame,
> line is in the center of the next 'list'. */
>
> void
> -set_current_sal_from_frame (struct frame_info *frame, int center)
> +set_current_sal_from_frame (struct frame_info *frame)
> {
> struct symtab_and_line sal;
>
> find_frame_sal (frame, &sal);
> - if (sal.symtab)
> - {
> - if (center)
> - sal.line = max (sal.line - get_lines_to_list () / 2, 1);
> - set_current_source_symtab_and_line (&sal);
> - }
> + if (sal.symtab != NULL)
> + set_current_source_symtab_and_line (&sal);
> }
>
> /* If ON, GDB will display disassembly of the next source line when
> diff --git a/gdb/testsuite/gdb.base/list.exp b/gdb/testsuite/gdb.base/list.exp
> index 12b2c94..67eef12 100644
> --- a/gdb/testsuite/gdb.base/list.exp
> +++ b/gdb/testsuite/gdb.base/list.exp
> @@ -529,4 +529,98 @@ if [ set_listsize 10 ] then {
> test_only_end
> }
>
> +# Follows tests that require execution.
> +
> +# Build source listing pattern based on a line range spec string. The
> +# range can be specificed as "START-END" indicating all lines in range
> +# (inclusive); or just "LINE", indicating just that line.
> +
> +proc build_pattern { range_spec } {
> + global line_re
> +
> + set range_list [split $range_spec -]
> + set range_list_len [llength $range_list]
> +
> + set range_start [lindex $range_list 0]
> + if { $range_list_len > 2 || $range_list_len < 1} {
> + error "invalid range spec string: $range_spec"
> + } elseif { $range_list_len == 2 } {
> + set range_end [lindex $range_list 1]
> + } else {
> + set range_end $range_start
> + }
> +
> + for {set i $range_start} {$i <= $range_end} {incr i} {
> + append pattern "\r\n$i\[ \t\]\[^\r\n\]*"
> + }
> +
> + verbose -log "pattern $pattern"
> + return $pattern
> +}
> +
> +# Test "list" command invocations right after stopping for an event.
> +# COMMAND is the actual list command, including arguments. LISTSIZE1
> +# and LISTSIZE2 are the listsizes set just before and after running
> +# the program to the stop point. COMMAND is issued twice. The first
> +# time, the lines specificed by LINERANGE1 are expected; the second
> +# time, the lines specified by LINERANGE2 are expected.
> +
> +proc test_list {command listsize1 listsize2 linerange1 linerange2} {
> + with_test_prefix "$command after stop: $listsize1, $listsize2" {
> + global binfile
> +
> + clean_restart $binfile
> + if ![runto_main] then {
> + fail "Can't run to main"
> + return
> + }
> +
> + # Test changing the listsize both before nexting, and after
> + # stopping, but before listing. Only the second listsize
> + # change should affect which lines are listed.
> + gdb_test_no_output "set listsize $listsize1"
> + gdb_test "next" "foo \\(.*"
> + gdb_test_no_output "set listsize $listsize2"
> +
> + set pattern1 [build_pattern $linerange1]
> + set pattern2 [build_pattern $linerange2]
> + gdb_test "$command" "${pattern1}" "$command #1"
> + gdb_test "$command" "${pattern2}" "$command #2"
> + }
> +}
> +
> +
> +# The first "list" should center the listing around line 8, the stop
> +# line.
> +test_list "list" 1 10 "3-12" "13-22"
> +
> +# Likewise.
> +test_list "list" 10 10 "3-12" "13-22"
> +
> +# Likewise, but show only one line. IOW, the first list should show
> +# line 8. Note how the listsize is 10 at the time of the stop, but
> +# before any listing had been requested. That should not affect the
> +# line range that is first listed.
> +test_list "list" 10 1 "8" "9"
> +
> +# Likewise, but show two lines.
> +test_list "list" 10 2 "7-8" "9-10"
> +
> +# Three lines.
> +test_list "list" 10 3 "7-9" "10-12"
> +
> +# Now test backwards. Just like "list", the first "list -" should
> +# center the listing around the stop line.
> +test_list "list -" 10 10 "3-12" "2"
> +
> +# Likewise, but test showing 3 lines at a time.
> +test_list "list -" 10 3 "7-9" "4-6"
> +
> +# 2 lines at a time.
> +test_list "list -" 10 2 "7-8" "5-6"
> +
> +# Test listing one line one. This case is a little special, and
> +# starts showing the previous line immediately.
> +test_list "list -" 10 1 "7" "6"
> +
> remote_exec build "rm -f list0.h"
> diff --git a/gdb/testsuite/gdb.mi/mi-cli.exp b/gdb/testsuite/gdb.mi/mi-cli.exp
> index 5657be6..566d64d 100644
> --- a/gdb/testsuite/gdb.mi/mi-cli.exp
> +++ b/gdb/testsuite/gdb.mi/mi-cli.exp
> @@ -65,6 +65,7 @@ set line_main_head [gdb_get_line_number "main ("]
> set line_main_body [expr $line_main_head + 2]
> set line_main_hello [gdb_get_line_number "Hello, World!"]
> set line_main_return [expr $line_main_hello + 2]
> +set line_main_callme_2 [expr $line_main_return + 1]
> set line_callee4_head [gdb_get_line_number "callee4 ("]
> set line_callee4_body [expr $line_callee4_head + 2]
> set line_callee4_next [expr $line_callee4_body + 1]
> @@ -184,6 +185,25 @@ mi_gdb_test "-interpreter-exec console \"list\"" \
> "\~\"$line_main_return\[\\\\t ]*callme \\(1\\);\\\\n\".*\\^done" \
> "-interpreter-exec console \"list\" at basics.c:\$line_main_return"
>
> +mi_gdb_test "600-break-insert -t basics.c:$line_main_callme_2" \
> + {600\^done,bkpt=.number="4",type="breakpoint".*\}} \
> + "-break-insert -t basics.c:\$line_main_callme_2"
> +
> +mi_execute_to "exec-continue" "breakpoint-hit" "main" "" ".*basics.c" \
> + $line_main_callme_2 { "" "disp=\"del\"" } \
> + "-exec-continue to line \$line_main_callme_2"
> +
> +# Restore the listsize back to the default.
> +mi_gdb_test "-interpreter-exec console \"set listsize 10\"" \
> + ".*=cmd-param-changed,param=\"listsize\",value=\"10\".*\\^done" \
> + "-interpreter-exec console \"set listsize 10\""
> +
> +# "list" should show 10 lines centered on where the program stopped.
> +set first_list_line [expr $line_main_callme_2 - 5]
> +mi_gdb_test "-interpreter-exec console \"list\"" \
> + ".*\~\"$first_list_line.*\\^done" \
> + "-interpreter-exec console \"list\" at basics.c:\$line_main_return"
> +
> mi_gdb_test "-interpreter-exec console \"help set args\"" \
> {\~"Set argument list to give program being debugged when it is started\.\\nFollow this command with any number of args, to be passed to the program\.".*\^done} \
> "-interpreter-exec console \"help set args\""
> diff --git a/gdb/testsuite/gdb.python/python.exp b/gdb/testsuite/gdb.python/python.exp
> index a470427..2b7d78e 100644
> --- a/gdb/testsuite/gdb.python/python.exp
> +++ b/gdb/testsuite/gdb.python/python.exp
> @@ -168,7 +168,8 @@ if ![runto_main] then {
> return 0
> }
>
> -runto [gdb_get_line_number "Break to end."]
> +set lineno [gdb_get_line_number "Break to end."]
> +runto $lineno
>
> # Test gdb.decode_line.
> gdb_test "python gdb.decode_line(\"main.c:43\")" \
> @@ -179,7 +180,7 @@ gdb_test "python print (len(symtab))" "2" "Test decode_line current location"
> gdb_test "python print (symtab\[0\])" "None" "Test decode_line expression parse"
> gdb_test "python print (len(symtab\[1\]))" "1" "Test decode_line current location"
> gdb_test "python print (symtab\[1\]\[0\].symtab)" ".*gdb.python/python.c.*" "Test decode_line current locationn filename"
> -gdb_test "python print (symtab\[1\]\[0\].line)" "22" "Test decode_line current location line number"
> +gdb_test "python print (symtab\[1\]\[0\].line)" "$lineno" "Test decode_line current location line number"
>
> gdb_py_test_silent_cmd "python symtab = gdb.decode_line(\"python.c:26 if foo\")" "test decode_line python.c:26" 1
> gdb_test "python print (len(symtab))" "2" "Test decode_line python.c:26 length"
>