Re: [RFC v6] PR gdb/13860: make -interpreter-exec console "list" behave more like "list".

[Pedro Alves <palves 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"
>