Patchwork [RFC] Call tui_before_prompt in do_scroll_vertical

login
register
mail settings
Submitter Tom Tromey
Date Dec. 23, 2019, 12:03 a.m.
Message ID <874kxrc3ha.fsf@tromey.com>
Download mbox | patch
Permalink /patch/37069/
State New
Headers show

Comments

Tom Tromey - Dec. 23, 2019, 12:03 a.m.
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

>> First I tried it with tui_update_source_windows_with_line, but this didn't
>> reset from_source_symtab (which was set deep in print_source_lines),
>> which resulted in some weird behavior when switching from "layout split"
>> to "layout asm" after scrolling down in the src window (the asm window
>> was then overwritten by the src window).

I wonder if we really need to change the current source symtab.
If so, I guess the TUI could just call
set_current_source_symtab_and_line directly.

The patch below removes the call to print_source_lines -- this approach
just seems too roundabout for my taste.

Andrew> I was able to reproduce the layout split -> layout asm issue you
Andrew> describe, but, I'm not convinced that the solution below is the right
Andrew> way to go.

With my patch I can't reproduce this, but I don't know whether I'm doing
it properly.

Hannes, could you possibly try this patch?

This could maybe be made even a bit smaller, but it's hard to be sure.
Despite all the cleanups, I find this area still pretty confusing.

Tom

commit 5a30635b352a9c0ff896c5044296087a3eb42073
Author: Tom Tromey <tom@tromey.com>
Date:   Sun Dec 22 16:52:56 2019 -0700

    Fix scrolling in TUI
    
    Hannes Domani pointed out that my previous patch to fix the "list"
    command in the TUI instead broke vertical scrolling.  While looking at
    this, I found that do_scroll_vertical calls print_source_lines, which
    seems like a very roundabout way to change the source window.  This
    patch removes this oddity and fixes the bug at the same time.
    
    I've added a new test case.  This is somewhat tricky, because the
    obvious approach of sending a dummy command after the scroll did not
    work -- due to how the TUI works, sennding a command causes the scroll
    to take effect.
    
    gdb/ChangeLog
    2019-12-22  Tom Tromey  <tom@tromey.com>
    
            PR tui/18932:
            * tui/tui-source.c (tui_source_window::do_scroll_vertical): Call
            update_source_window, not print_source_lines.
    
    gdb/testsuite/ChangeLog
    2019-12-22  Tom Tromey  <tom@tromey.com>
    
            PR tui/18932:
            * lib/tuiterm.exp (Term::wait_for): Rename from _accept.  Return a
            meangingful value.
            (Term::command, Term::resize): Update.
            * gdb.tui/basic.exp: Add scrolling test.
    
    Change-Id: I9636a7c8a8cade37431c6165ee996a9d556ef1c8
Doug Evans via gdb-patches - Dec. 23, 2019, 12:19 a.m.
Am Montag, 23. Dezember 2019, 01:03:49 MEZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
>
> >> First I tried it with tui_update_source_windows_with_line, but this didn't
> >> reset from_source_symtab (which was set deep in print_source_lines),
> >> which resulted in some weird behavior when switching from "layout split"
> >> to "layout asm" after scrolling down in the src window (the asm window
> >> was then overwritten by the src window).
>
> I wonder if we really need to change the current source symtab.
> If so, I guess the TUI could just call
> set_current_source_symtab_and_line directly.
>
> The patch below removes the call to print_source_lines -- this approach
> just seems too roundabout for my taste.
>
> Andrew> I was able to reproduce the layout split -> layout asm issue you
> Andrew> describe, but, I'm not convinced that the solution below is the right
> Andrew> way to go.
>
> With my patch I can't reproduce this, but I don't know whether I'm doing
> it properly.
>
> Hannes, could you possibly try this patch?
>
> This could maybe be made even a bit smaller, but it's hard to be sure.
> Despite all the cleanups, I find this area still pretty confusing.

This fixes the scrolling, and I no longer have any problem when switching
from layout split -> layout asm after scrolling the src window.

But now, when in layout split, the asm window doesn't scroll along with
the src window any more.
I found this to be a very convenient feature, because this showed
you which asm lines belonged to which src line (as good as the optimized
code allowed at least).


Regards
Hannes Domani
Andrew Burgess - Jan. 7, 2020, 11:52 a.m.
Here's a series that fixes the vertical scrolling issue.  It
incorporates your proposed fix (with a small testsuite change), and
then builds upon it.

This is a different (simpler) approach than I originally suggested.

I'm still not really happy with how the tui keeps track and displays
the current symtab_and_line; I think there's many places that GDB
should do a better job of placing the "current" location in the centre
of the screen, where currently it has a tendency to jam the current
location right at the top.

However, I think leaving the ability to vertically scroll broken isn't
good, and I think this series is a step in the right direction
(maybe?).

Thoughts / comments welcome.

Thanks,
Andrew


--

Andrew Burgess (5):
  gdb/testsuite/tui: Always dump_screen when asked
  gdb/testsuite/tui: Split enter_tui into two procs
  gdb/testsuite/tui: Introduce check_box_contents
  gdb/tui: Fix 'layout asm' before the inferior has started
  gdb/tui: Link source and assembler scrolling .... again

Tom Tromey (1):
  gdb: Fix scrolling in TUI

 gdb/ChangeLog                            | 17 ++++++++
 gdb/testsuite/ChangeLog                  | 30 ++++++++++++++
 gdb/testsuite/gdb.tui/basic.exp          | 40 +++++++++++++++++++
 gdb/testsuite/gdb.tui/tui-layout-asm.exp | 34 ++++++++++++++++
 gdb/testsuite/lib/tuiterm.exp            | 67 ++++++++++++++++++++++++++------
 gdb/tui/tui-source.c                     | 27 +++++++------
 gdb/tui/tui.c                            | 10 +++--
 7 files changed, 199 insertions(+), 26 deletions(-)
 create mode 100644 gdb/testsuite/gdb.tui/tui-layout-asm.exp
Tom Tromey - Jan. 7, 2020, 7:38 p.m.
Andrew> This is a different (simpler) approach than I originally suggested.

I meant to reply to that one.  I thought the changes in tui-hooks.c
looked reasonable.  If you want to resurrect that, I'm in favor of it.

Andrew> I'm still not really happy with how the tui keeps track and displays
Andrew> the current symtab_and_line

Me too.

Andrew> However, I think leaving the ability to vertically scroll broken isn't
Andrew> good, and I think this series is a step in the right direction
Andrew> (maybe?).

Agreed.  It all looks good to me.

Tom
Andrew Burgess - Jan. 9, 2020, 11:28 p.m.
* Tom Tromey <tom@tromey.com> [2020-01-07 12:38:48 -0700]:

> Andrew> This is a different (simpler) approach than I originally suggested.
> 
> I meant to reply to that one.  I thought the changes in tui-hooks.c
> looked reasonable.  If you want to resurrect that, I'm in favor of
> it.

The reason I abandoned the old approach was that it moved the
redisplay from the pre-prompt hook into the symtab-changed hook.  This
has two problems:

First, calling 'tui_add_win_to_layout (SRC_WIN);' in the symtab
changed hook is not OK, consider stopping after a 'continue' command,
the symtab will change, but we don't want the SRC window to reappear.
So, we should only redisplay SRC if the symtab changes AND the current
frame doesn't change.  The pre-prompt hook achieves this due to its
ordering of the checks.

Second, even if we could solve the above problem, there are cases
where the current symtab changes multiple times due to a single
command (I forget what, but I was probably testing with list at the
time).  If we redraw the contents of the SRC window on every symtab
changes event this is a bit wasteful - though probably not critical.

I've been playing with the idea of having the TUI maintain its own
idea of current source location.  When the global symtab change event
fires we'd copy the global location into the TUI location, but, when
the TUI scrolls we'd update the TUI only location.  Both the SRC and
ASM windows would draw their contents based on the TUI's location
value.

Anyway, I made a little progress, but still don't have it working, so
I don't know if there's a reason why is wouldn't work...

Thanks,
Andrew



> 
> Andrew> I'm still not really happy with how the tui keeps track and displays
> Andrew> the current symtab_and_line
> 
> Me too.
> 
> Andrew> However, I think leaving the ability to vertically scroll broken isn't
> Andrew> good, and I think this series is a step in the right direction
> Andrew> (maybe?).
> 
> Agreed.  It all looks good to me.
> 
> Tom

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0f35deff350..b441c9d5031 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@ 
+2019-12-22  Tom Tromey  <tom@tromey.com>
+
+	PR tui/18932:
+	* tui/tui-source.c (tui_source_window::do_scroll_vertical): Call
+	update_source_window, not print_source_lines.
+
 2019-12-20  Tom Tromey  <tom@tromey.com>
 
 	* tui/tui.c (tui_show_source): Remove.
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 4476549be6b..985a68bc126 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,11 @@ 
+2019-12-22  Tom Tromey  <tom@tromey.com>
+
+	PR tui/18932:
+	* lib/tuiterm.exp (Term::wait_for): Rename from _accept.  Return a
+	meangingful value.
+	(Term::command, Term::resize): Update.
+	* gdb.tui/basic.exp: Add scrolling test.
+
 2019-12-20  Tom Tromey  <tom@tromey.com>
 
 	* gdb.tui/list-before.exp: New file.
diff --git a/gdb/testsuite/gdb.tui/basic.exp b/gdb/testsuite/gdb.tui/basic.exp
index f9d57d1cf6d..ca88dad1cd4 100644
--- a/gdb/testsuite/gdb.tui/basic.exp
+++ b/gdb/testsuite/gdb.tui/basic.exp
@@ -35,6 +35,17 @@  gdb_assert {![string match "No Source Available" $text]} \
 Term::command "list main"
 Term::check_contents "list main" "21 *return 0"
 
+# Get the first source line.
+set line [string_to_regexp [Term::get_line 1]]
+# Send an up arrow.
+send_gdb "\033\[A"
+# Wait for a redraw and check that the first line changed.
+if {[Term::wait_for $line] && [Term::get_line 1] != $line} {
+    pass "scroll up"
+} else {
+    fail "scroll up"
+}
+
 Term::check_box "source box" 0 0 80 15
 
 Term::command "layout asm"
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index 81247d5d9a4..7761c63d6aa 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -388,8 +388,10 @@  namespace eval Term {
 	_clear_lines 0 $_rows
     }
 
-    # Accept some output from gdb and update the screen.
-    proc _accept {wait_for} {
+    # Accept some output from gdb and update the screen.  WAIT_FOR is
+    # a regexp matching the line to wait for.  Return 0 on timeout, 1
+    # on success.
+    proc wait_for {wait_for} {
 	global expect_out
 	global gdb_prompt
 	variable _cur_x
@@ -424,7 +426,7 @@  namespace eval Term {
 		timeout {
 		    # Assume a timeout means we somehow missed the
 		    # expected result, and carry on.
-		    return
+		    return 0
 		}
 	    }
 
@@ -443,6 +445,8 @@  namespace eval Term {
 		set wait_for $prompt_wait_for
 	    }
 	}
+
+	return 1
     }
 
     # Like ::clean_restart, but ensures that gdb starts in an
@@ -480,7 +484,7 @@  namespace eval Term {
     # be supplied by this function.
     proc command {cmd} {
 	send_gdb "$cmd\n"
-	_accept [string_to_regexp $cmd]
+	wait_for [string_to_regexp $cmd]
     }
 
     # Return the text of screen line N, without attributes.  Lines are
@@ -641,14 +645,14 @@  namespace eval Term {
 	# Due to the strange column resizing behavior, and because we
 	# don't care about this intermediate resize, we don't check
 	# the size here.
-	_accept "@@ resize done $_resize_count"
+	wait_for "@@ resize done $_resize_count"
 	incr _resize_count
 	# Somehow the number of columns transmitted to gdb is one less
 	# than what we request from expect.  We hide this weird
 	# details from the caller.
 	_do_resize $_rows $cols
 	stty columns [expr {$_cols + 1}] < $gdb_spawn_name
-	_accept "@@ resize done $_resize_count, size = ${_cols}x${rows}"
+	wait_for "@@ resize done $_resize_count, size = ${_cols}x${rows}"
 	incr _resize_count
     }
 }
diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
index 0728263b8c5..bcba9e176f1 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -136,28 +136,29 @@  tui_source_window::do_scroll_vertical (int num_to_scroll)
 {
   if (!content.empty ())
     {
-      struct tui_line_or_address l;
       struct symtab *s;
       struct symtab_and_line cursal = get_current_source_symtab_and_line ();
+      struct gdbarch *arch = gdbarch;
 
       if (cursal.symtab == NULL)
-	s = find_pc_line_symtab (get_frame_pc (get_selected_frame (NULL)));
+	{
+	  struct frame_info *fi = get_selected_frame (NULL);
+	  s = find_pc_line_symtab (get_frame_pc (fi));
+	  arch = get_frame_arch (fi);
+	}
       else
 	s = cursal.symtab;
 
-      l.loa = LOA_LINE;
-      l.u.line_no = start_line_or_addr.u.line_no
-	+ num_to_scroll;
+      int line_no = start_line_or_addr.u.line_no + num_to_scroll;
       const std::vector<off_t> *offsets;
       if (g_source_cache.get_line_charpos (s, &offsets)
-	  && l.u.line_no > offsets->size ())
-	/* line = s->nlines - win_info->content_size + 1; */
-	/* elz: fix for dts 23398.  */
-	l.u.line_no = start_line_or_addr.u.line_no;
-      if (l.u.line_no <= 0)
-	l.u.line_no = 1;
-
-      print_source_lines (s, l.u.line_no, l.u.line_no + 1, 0);
+	  && line_no > offsets->size ())
+	line_no = start_line_or_addr.u.line_no;
+      if (line_no <= 0)
+	line_no = 1;
+
+      cursal.line = line_no;
+      update_source_window (arch, cursal);
     }
 }