From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26458 invoked by alias); 21 May 2014 22:16:48 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 26426 invoked by uid 89); 21 May 2014 22:16:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 21 May 2014 22:16:43 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s4LMGg4T011277 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 21 May 2014 18:16:42 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s4LMGeKq002799; Wed, 21 May 2014 18:16:41 -0400 Message-ID: <537D25C8.4070905@redhat.com> Date: Wed, 21 May 2014 22:16:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: gdb-patches@sourceware.org CC: Tom Tromey Subject: Re: [RFC v6] PR gdb/13860: make -interpreter-exec console "list" behave more like "list". References: <1393957974-4521-1-git-send-email-tromey@redhat.com> <1393957974-4521-3-git-send-email-tromey@redhat.com> <53286E76.9080808@redhat.com> In-Reply-To: <53286E76.9080808@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-05/txt/msg00524.txt.bz2 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 > > * 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 > > * 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" >