[RFC][PATCH] Call tui_before_prompt in do_scroll_vertical

[RFC][PATCH] Call tui_before_prompt in do_scroll_vertical

[Hannes Domani via gdb-patches]

Without this call scrolling in the src window does not work at all.

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).

gdb/ChangeLog:

2019-12-21  Hannes Domani  <ssbssa@yahoo.de>

	* tui/tui-hooks.c (tui_before_prompt): Remove static.
	* tui/tui-source.c (tui_before_prompt): Add prototype.
	(tui_source_window::do_scroll_vertical): Add tui_before_prompt call.
---
 gdb/tui/tui-hooks.c  | 2 +-
 gdb/tui/tui-source.c | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index 8576bb8fcc..8b9e70316f 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -179,7 +179,7 @@ tui_inferior_exit (struct inferior *inf)
 
 /* Observer for the before_prompt notification.  */
 
-static void
+void
 tui_before_prompt (const char *current_gdb_prompt)
 {
   tui_refresh_frame_and_register_information ();
diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
index 0728263b8c..dfe5721edf 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -39,6 +39,9 @@
 #include "tui/tui-source.h"
 #include "gdb_curses.h"
 
+void
+tui_before_prompt (const char *current_gdb_prompt);
+
 /* Function to display source in the source window.  */
 bool
 tui_source_window::set_contents (struct gdbarch *arch,
@@ -158,6 +161,8 @@ tui_source_window::do_scroll_vertical (int num_to_scroll)
 	l.u.line_no = 1;
 
       print_source_lines (s, l.u.line_no, l.u.line_no + 1, 0);
+
+      tui_before_prompt ("");
     }
 }
 
-- 
2.24.1

Re: [RFC][PATCH] Call tui_before_prompt in do_scroll_vertical

[Andrew Burgess]

* Hannes Domani via gdb-patches <gdb-patches@sourceware.org> [2019-12-21 16:33:25 +0100]:

> Without this call scrolling in the src window does not work at all.

Thanks for spotting this and starting work on a fix.

When fixing regressions like this its always useful to track down the
commit that introduced the failure and add a link to it.  This is not
about blaming others, but just adds a really useful historical log.

In this case, it was this commit that caused the breakage in scrolling:

  commit b4b49dcbff6b437fa8b4e2fc0c3f27b457f11310 (refs/bisect/bad)
  Date:   Wed Nov 13 16:47:58 2019 -0700

      Don't call tui_show_source from tui_ui_out

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

Tom might suggest a better solution quicker, but if not I'll try to
review this code properly soon.

However, the questions/observations I have are:

 - It doesn't feel like calling a before prompt hook is the right way
   to go, and even if this is basically the right solution, we should
   split the important part of the hook into a separate function with
   a suitable name and call that instead.

 - I also don't understand why calling this hook in
   tui_souce_window::do_scroll_vertical changes what happens when we
   switch to tui_disasm_window.  Stepping through from the 'layout
   asm' seems to show that we do correctly change to the asm window,
   and its only when we later call display_gdb_prompt (from
   event-top.c) that we overwrite the asm window with the source for
   some reason.  I'd like to understand what's going on there more as
   it feels like if we could fix that as almost a separate issue then
   we should go back to calling tui_update_source_windows_with_line as
   you tried first, which feels like a better solution.

 - On coding style within GDB, we declare functions in header files,
   and define them in source files.  Also declarations are marked
   extern and include return type and function name on one line, so it
   would be:

   extern void tui_before_prompt (const char *current_gdb_prompt);

   But like I said, I'll need convincing that this is the right
   approach to fix this.

Thanks,
Andrew

> 
> gdb/ChangeLog:
> 
> 2019-12-21  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* tui/tui-hooks.c (tui_before_prompt): Remove static.
> 	* tui/tui-source.c (tui_before_prompt): Add prototype.
> 	(tui_source_window::do_scroll_vertical): Add tui_before_prompt call.
> ---
>  gdb/tui/tui-hooks.c  | 2 +-
>  gdb/tui/tui-source.c | 5 +++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
> index 8576bb8fcc..8b9e70316f 100644
> --- a/gdb/tui/tui-hooks.c
> +++ b/gdb/tui/tui-hooks.c
> @@ -179,7 +179,7 @@ tui_inferior_exit (struct inferior *inf)
>  
>  /* Observer for the before_prompt notification.  */
>  
> -static void
> +void
>  tui_before_prompt (const char *current_gdb_prompt)
>  {
>    tui_refresh_frame_and_register_information ();
> diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
> index 0728263b8c..dfe5721edf 100644
> --- a/gdb/tui/tui-source.c
> +++ b/gdb/tui/tui-source.c
> @@ -39,6 +39,9 @@
>  #include "tui/tui-source.h"
>  #include "gdb_curses.h"
>  
> +void
> +tui_before_prompt (const char *current_gdb_prompt);
> +
>  /* Function to display source in the source window.  */
>  bool
>  tui_source_window::set_contents (struct gdbarch *arch,
> @@ -158,6 +161,8 @@ tui_source_window::do_scroll_vertical (int num_to_scroll)
>  	l.u.line_no = 1;
>  
>        print_source_lines (s, l.u.line_no, l.u.line_no + 1, 0);
> +
> +      tui_before_prompt ("");
>      }
>  }
>  
> -- 
> 2.24.1
> 

Re: [RFC][PATCH] Call tui_before_prompt in do_scroll_vertical

[Hannes Domani via gdb-patches]

 Am Sonntag, 22. Dezember 2019, 01:57:56 MEZ hat Andrew Burgess <andrew.burgess@embecosm.com> Folgendes geschrieben:

> * Hannes Domani via gdb-patches <gdb-patches@sourceware.org> [2019-12-21 16:33:25 +0100]:
>
> > Without this call scrolling in the src window does not work at all.
>
> Thanks for spotting this and starting work on a fix.
>
> When fixing regressions like this its always useful to track down the
> commit that introduced the failure and add a link to it.  This is not
> about blaming others, but just adds a really useful historical log.
>
> In this case, it was this commit that caused the breakage in scrolling:
>
>   commit b4b49dcbff6b437fa8b4e2fc0c3f27b457f11310 (refs/bisect/bad)
>   Date:  Wed Nov 13 16:47:58 2019 -0700
>
>       Don't call tui_show_source from tui_ui_out

I have never tried bisecting before, and I don't know how it works on Windows,
but I'll try to do that the next time.

> >
> > 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 was able to reproduce the layout split -> layout asm issue you
> describe, but, I'm not convinced that the solution below is the right
> way to go.
>
> Tom might suggest a better solution quicker, but if not I'll try to
> review this code properly soon.
>
> However, the questions/observations I have are:
>
> - It doesn't feel like calling a before prompt hook is the right way
>   to go, and even if this is basically the right solution, we should
>   split the important part of the hook into a separate function with
>   a suitable name and call that instead.

I wasn't 100% sure that this is the proper solution either,
that's why I added [RFC].

> - I also don't understand why calling this hook in
>   tui_souce_window::do_scroll_vertical changes what happens when we
>   switch to tui_disasm_window.  Stepping through from the 'layout
>   asm' seems to show that we do correctly change to the asm window,
>   and its only when we later call display_gdb_prompt (from
>   event-top.c) that we overwrite the asm window with the source for
>   some reason.  I'd like to understand what's going on there more as
>   it feels like if we could fix that as almost a separate issue then
>   we should go back to calling tui_update_source_windows_with_line as
>   you tried first, which feels like a better solution.

That's what I tried to describe with this:

> > 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),

The variable from_source_symtab was set somewhere in print_source_lines,
and because it was not reset, the next time tui_before_prompt is called,
the source window is automatically added again in
tui_refresh_frame_and_register_information.

> - On coding style within GDB, we declare functions in header files,
>   and define them in source files.  Also declarations are marked
>   extern and include return type and function name on one line, so it
>   would be:
>
>   extern void tui_before_prompt (const char *current_gdb_prompt);
>
>   But like I said, I'll need convincing that this is the right
>   approach to fix this.

Again, I agree with you that this is probably not the correct fix,
that's why I added [RFC] and didn't bother with the function prototype
in the correct header file.

If that's not the proper use of [RFC], I apologize.


Regards
Hannes Domani


>
> Thanks,
> Andrew
>
>
> >
> > gdb/ChangeLog:
> >
> > 2019-12-21  Hannes Domani  <ssbssa@yahoo.de>
> >
> >     * tui/tui-hooks.c (tui_before_prompt): Remove static.
> >     * tui/tui-source.c (tui_before_prompt): Add prototype.
> >     (tui_source_window::do_scroll_vertical): Add tui_before_prompt call.
> > ---
> >  gdb/tui/tui-hooks.c  | 2 +-
> >  gdb/tui/tui-source.c | 5 +++++
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
> > index 8576bb8fcc..8b9e70316f 100644
> > --- a/gdb/tui/tui-hooks.c
> > +++ b/gdb/tui/tui-hooks.c
> > @@ -179,7 +179,7 @@ tui_inferior_exit (struct inferior *inf)
> >
> >  /* Observer for the before_prompt notification.  */
> >
> > -static void
> > +void
> >  tui_before_prompt (const char *current_gdb_prompt)
> >  {
> >    tui_refresh_frame_and_register_information ();
> > diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
> > index 0728263b8c..dfe5721edf 100644
> > --- a/gdb/tui/tui-source.c
> > +++ b/gdb/tui/tui-source.c
> > @@ -39,6 +39,9 @@
> >  #include "tui/tui-source.h"
> >  #include "gdb_curses.h"
> >
> > +void
> > +tui_before_prompt (const char *current_gdb_prompt);
> > +
> >  /* Function to display source in the source window.  */
> >  bool
> >  tui_source_window::set_contents (struct gdbarch *arch,
> > @@ -158,6 +161,8 @@ tui_source_window::do_scroll_vertical (int num_to_scroll)
> >      l.u.line_no = 1;
> >
> >        print_source_lines (s, l.u.line_no, l.u.line_no + 1, 0);
> > +
> > +      tui_before_prompt ("");
> >      }
> >  }
> >
> > --
> > 2.24.1
> >
>

Re: [RFC][PATCH] Call tui_before_prompt in do_scroll_vertical

[Tom Tromey]

>>>>> "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

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);
     }
 }
 

Re: [RFC][PATCH] Call tui_before_prompt in do_scroll_vertical

[Hannes Domani via gdb-patches]

 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

Re: [RFC][PATCH] Call tui_before_prompt in do_scroll_vertical

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2019-12-22 17:03:45 -0700]:

> >>>>> "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.

I started playing with similar solutions.  I also started looking at
the role tui_refresh_frame_and_register_information plays in keeping
the source/asm windows up to date.  My conclusion was it does nothing
but add more complication.

Tom, the patch below applies on top of yours, it removes all of the
source/asm updating from tui_refresh_frame_and_register_information.
There's some direct action taken in the symtab changed observer
hook.

I also now call update_source_window_as_is on all source windows after
the vertical scroll - this partly fixes the asm/source scroll
synchronisation issue Hannes pointed out.  Calling the *_as_is version
has (I think) an added benefit - if you scroll horizontally, and then
scroll up/down, the horizontal scroll is maintained.  This feels much
nicer to me, and is what caused me to remove the code from
tui_refresh_frame_and_register_information.

I said partly fixes the scroll synchronisation issue as scrolling the
asm window doesn't (and never has I think) caused the source window to
scroll - it kind of feels like it should though... this probably just
requires us to add something similar to the asm windows vert scroll
code.

There is one issue with this patch, but I've run out of time right now
to look at this any further, if you fire up gdb on gdb then do:

  start
  tui enable
  # will be in main now.
  break gdb_main
  continue

The breakpoint line will be the first line in the console, rather than
being in the centre of the window.

I don't know if this is a worth while direction to go in or not, what
do you think?

Thanks,
Andrew


---

diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index 8576bb8fccd..a6fbd88e51b 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -110,18 +110,13 @@ tui_event_modify_breakpoint (struct breakpoint *b)
 
 static bool from_stack;
 
-/* This is set to true if the next window refresh should come from the
-   current source symtab.  */
-
-static bool from_source_symtab;
-
 /* Refresh TUI's frame and register information.  This is a hook intended to be
    used to update the screen after potential frame and register changes.  */
 
 static void
 tui_refresh_frame_and_register_information ()
 {
-  if (!from_stack && !from_source_symtab)
+  if (!from_stack)
     return;
 
   target_terminal::scoped_restore_terminal_state term_state;
@@ -144,6 +139,7 @@ tui_refresh_frame_and_register_information ()
 	  tui_refreshing_registers = 0;
 	}
     }
+#if 0
   else if (!from_stack)
     {
       /* Make sure that the source window is displayed.  */
@@ -152,6 +148,7 @@ tui_refresh_frame_and_register_information ()
       struct symtab_and_line sal = get_current_source_symtab_and_line ();
       tui_update_source_windows_with_line (sal);
     }
+#endif
 }
 
 /* Dummy callback for deprecated_print_frame_info_listing_hook which is called
@@ -184,7 +181,6 @@ tui_before_prompt (const char *current_gdb_prompt)
 {
   tui_refresh_frame_and_register_information ();
   from_stack = false;
-  from_source_symtab = false;
 }
 
 /* Observer for the normal_stop notification.  */
@@ -208,7 +204,11 @@ tui_context_changed (user_selected_what ignore)
 static void
 tui_symtab_changed ()
 {
-  from_source_symtab = true;
+  /* Make sure that the source window is displayed.  */
+  tui_add_win_to_layout (SRC_WIN);
+
+  struct symtab_and_line sal = get_current_source_symtab_and_line ();
+  tui_update_source_windows_with_line (sal);
 }
 
 /* Token associated with observers registered while TUI hooks are
diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
index bcba9e176f1..4846ddec0fc 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -145,6 +145,7 @@ tui_source_window::do_scroll_vertical (int num_to_scroll)
 	  struct frame_info *fi = get_selected_frame (NULL);
 	  s = find_pc_line_symtab (get_frame_pc (fi));
 	  arch = get_frame_arch (fi);
+	  cursal.symtab = s;
 	}
       else
 	s = cursal.symtab;
@@ -158,7 +159,10 @@ tui_source_window::do_scroll_vertical (int num_to_scroll)
 	line_no = 1;
 
       cursal.line = line_no;
-      update_source_window (arch, cursal);
+      find_line_pc (cursal.symtab, cursal.line, &cursal.pc);
+
+      for (struct tui_source_window_base *win_info : tui_source_windows ())
+	win_info->update_source_window_as_is (arch, cursal);
     }
 }
 

[PATCH 2/6] gdb/testsuite/tui: Split enter_tui into two procs

[Andrew Burgess]

Split Term::enter_tui into two procedures, a core which does the
setup, but doesn't actually enable tui mode, and the old enter_tui
that calls the new core, and then enables tui mode.

This is going to be useful in a later commit.

gdb/testsuite/ChangeLog:

	* lib/tuiterm.exp (Term::prepare_for_tui): New proc.
	(Term::enter_tui): Use Term::prepare_for_tui.

Change-Id: I501dfb2ddaa4a4e7246a5ad319ab428e4f42b3af
---
 gdb/testsuite/ChangeLog       |  5 +++++
 gdb/testsuite/lib/tuiterm.exp | 16 +++++++++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index 36e034a3639..9ac599b6f2d 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -462,15 +462,25 @@ namespace eval Term {
 	}
     }
 
-    # Start the TUI.  Returns 1 on success, 0 if TUI tests should be
-    # skipped.
-    proc enter_tui {} {
+    # Setup ready for starting the tui, but don't actually start it.
+    # Returns 1 on success, 0 if TUI tests should be skipped.
+    proc prepare_for_tui {} {
 	if {[skip_tui_tests]} {
 	    return 0
 	}
 
 	gdb_test_no_output "set tui border-kind ascii"
 	gdb_test_no_output "maint set tui-resize-message on"
+	return 1
+    }
+
+    # Start the TUI.  Returns 1 on success, 0 if TUI tests should be
+    # skipped.
+    proc enter_tui {} {
+	if {![prepare_for_tui]} {
+	    return 0
+	}
+
 	command "tui enable"
 	return 1
     }
-- 
2.14.5

[PATCH 1/6] gdb/testsuite/tui: Always dump_screen when asked

[Andrew Burgess]

The Term::dump_screen routine currently dumps the screen using calls
to 'verbose', this means it will only dump the screen when the
testsuite is running in verbose mode.

However, the Term::dump_screen is most often called when a test fails,
in this case I think it is useful to have the screen dumped even when
we're not in verbose mode.

This commit changes the calls to 'verbose' to be 'verbose -log' so we
always get the screen dump.

gdb/testsuite/ChangeLog:

	* lib/tuiterm.exp (Term::dump_screen): Always dump the screen when
	called.

Change-Id: I5f0a7f5ac2ece04d6fe6e9c5a28ea2a0dda38955
---
 gdb/testsuite/ChangeLog       | 5 +++++
 gdb/testsuite/lib/tuiterm.exp | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index 6f3d41f1ccb..36e034a3639 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -595,10 +595,10 @@ namespace eval Term {
     proc dump_screen {} {
 	variable _rows
 	variable _cols
-	verbose "Screen Dump ($_cols x $_rows):"
+	verbose -log "Screen Dump ($_cols x $_rows):"
 	for {set y 0} {$y < $_rows} {incr y} {
 	    set fmt [format %5d $y]
-	    verbose "$fmt [get_line $y]"
+	    verbose -log "$fmt [get_line $y]"
 	}
     }
 
-- 
2.14.5

[PATCH 4/6] gdb/tui: Fix 'layout asm' before the inferior has started

[Andrew Burgess]

Currently if a user starts the tui with 'layout asm' then they will be
presented with the 'src' layout.

What happens is:

  1. Layout command enables TUI, selecting the SRC layout by default.

  2. As part of tui_enable we call tui_display_main, which calls
     tui_get_begin_asm_address, which calls
     set_default_source_symtab_and_line.  This changes core GDBs
     current symtab and line, which triggers a call to the symtab
     changed hook tui_symtab_changed, which sets the flag
     from_source_symtab.

  3. Back in the layout command, the layout is changed from SRC to
     ASM.  After this the layout command completes and we return to
     core GDB which prints the prompt, however...

  4. The before prompt hook is called which sees the
     from_source_symtab flag is set and forces the SRC window to be
     displayed.  This switches us back to SRC view.

The solution I propose here is to delay installing the hooks into core
GDB until after we have finished setting up the tui and selecting the
default frame to view.  In this way we effectively ignore the first
symtab changed event triggered when making main the default symtab.

gdb/ChangeLog:

	* tui/tui.c (tui_enable): Register tui hooks after calling
	tui_display_main.

gdb/testsuite/ChangeLog:

	* gdb.tui/tui-layout-asm.exp: New file.

Change-Id: I858ab81a17ffb4aa72deb3f36c3755228a9c9d9a
---
 gdb/ChangeLog                            |  5 +++++
 gdb/testsuite/ChangeLog                  |  4 ++++
 gdb/testsuite/gdb.tui/tui-layout-asm.exp | 34 ++++++++++++++++++++++++++++++++
 gdb/tui/tui.c                            | 10 ++++++----
 4 files changed, 49 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.tui/tui-layout-asm.exp

diff --git a/gdb/testsuite/gdb.tui/tui-layout-asm.exp b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
new file mode 100644
index 00000000000..cec2735764e
--- /dev/null
+++ b/gdb/testsuite/gdb.tui/tui-layout-asm.exp
@@ -0,0 +1,34 @@
+# Copyright 2020 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 License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Ensure that 'layout asm' before starting the inferior puts us in the
+# asm layout and displays the disassembly for main.
+
+load_lib "tuiterm.exp"
+
+standard_testfile tui-layout.c
+
+if {[build_executable "failed to prepare" ${testfile} ${srcfile}] == -1} {
+    return -1
+}
+
+Term::clean_restart 24 80 $testfile
+if {![Term::prepare_for_tui]} {
+    unsupported "TUI not supported"
+}
+
+# This puts us into TUI mode, and should display the ASM window.
+Term::command "layout asm"
+Term::check_box_contents "check asm box contents" 0 0 80 15 "<main>"
diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c
index 7ff5825ece2..99d30a55a15 100644
--- a/gdb/tui/tui.c
+++ b/gdb/tui/tui.c
@@ -489,10 +489,6 @@ tui_enable (void)
      clearok (stdscr, TRUE);
    }
 
-  /* Install the TUI specific hooks.  */
-  tui_install_hooks ();
-  rl_startup_hook = tui_rl_startup_hook;
-
   if (tui_update_variables ())
     tui_rehighlight_all ();
 
@@ -513,6 +509,12 @@ tui_enable (void)
   else
     tui_display_main ();
 
+  /* Install the TUI specific hooks.  This must be done after the call to
+     tui_display_main so that we don't detect the symtab changed event it
+     can cause.  */
+  tui_install_hooks ();
+  rl_startup_hook = tui_rl_startup_hook;
+
   /* Restore TUI keymap.  */
   tui_set_key_mode (tui_current_key_mode);
 
-- 
2.14.5

[PATCH 0/6] Vertical scrolling and another small bug fix

[Andrew Burgess]

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

-- 
2.14.5

[PATCH 6/6] gdb/tui: Link source and assembler scrolling .... again

[Andrew Burgess]

Until recently when the source window was scrolled the assembler
window would scroll in sync - keeping the disassembly for the current
line in view.

This was broken in commit:

  commit b4b49dcbff6b437fa8b4e2fc0c3f27b457f11310
  Date:   Wed Nov 13 16:47:58 2019 -0700

      Don't call tui_show_source from tui_ui_out

This commit restores the synchronised scrolling and also maintains the
horizontal scroll within the source view when it is vertically
scrolled, something that was broken before.

This commit does not mean that scrolling the assembler view scrolls
the source view.  The connection this way never existed, though maybe
it should, but I'll leave adding this feature for a separate commit.

gdb/ChangeLog:

	* tui/tui-source.c (tui_source_window::do_scroll_vertical): Update
	all source windows, and maintain horizontal scroll status while
	doing so.

gdb/testsuite/ChangeLog:

	* gdb.tui/basic.exp: Add more scrolling tests.

Change-Id: I250114a3bc670040a6a759d41905776771b2f818
---
 gdb/ChangeLog                   |  6 ++++++
 gdb/testsuite/ChangeLog         |  4 ++++
 gdb/testsuite/gdb.tui/basic.exp | 27 +++++++++++++++++++++++++++
 gdb/tui/tui-source.c            |  4 +++-
 4 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.tui/basic.exp b/gdb/testsuite/gdb.tui/basic.exp
index be822f8a915..34e60384c4e 100644
--- a/gdb/testsuite/gdb.tui/basic.exp
+++ b/gdb/testsuite/gdb.tui/basic.exp
@@ -48,6 +48,33 @@ if {[Term::wait_for [string_to_regexp $line]] \
     fail "scroll up"
 }
 
+# Check the horizontal scrolling.  First confirm that 'main ()' is
+# where we expect it to be.  This relies on the current way we
+# position source code on the screen, which might change in the
+# future.  The important part of this test is detecting the left/right
+# scrolling, not which line main is actually on.
+set line_num 6
+set line [Term::get_line $line_num]
+gdb_assert {[regexp -- "19\[\\t \]+main \\(\\)" $line]} \
+    "check main is where we expect on the screen"
+set regexp "19\[\\t \]+ain \\(\\)"
+# Send a right arrow.
+send_gdb "\033\[C"
+if {[Term::wait_for $regexp]} {
+    pass "scroll right"
+} else {
+    fail "scroll right"
+}
+set line [Term::get_line $line_num]
+# Send a down arrow.
+send_gdb "\033\[B"
+if {[Term::wait_for $regexp] \
+	&& [Term::get_line [expr {$line_num - 1}]] == $line} {
+    pass "scroll down"
+} else {
+    fail "scroll down"
+}
+
 Term::check_box "source box" 0 0 80 15
 
 Term::command "layout asm"
diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
index 13f2dc7cfe1..912eaa45440 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -158,7 +158,9 @@ tui_source_window::do_scroll_vertical (int num_to_scroll)
 	line_no = 1;
 
       cursal.line = line_no;
-      update_source_window (arch, cursal);
+      find_line_pc (cursal.symtab, cursal.line, &cursal.pc);
+      for (struct tui_source_window_base *win_info : tui_source_windows ())
+	win_info->update_source_window_as_is (arch, cursal);
     }
 }
 
-- 
2.14.5

[PATCH 3/6] gdb/testsuite/tui: Introduce check_box_contents

[Andrew Burgess]

A new test procedure for matching the contents of one screen box
against a regexp.  This can be used to match the contents of one TUI
window against a regexp without any of the borders, or other windows
being included in the matched output (as is currently the case with
check_contents).

This will be used in a later commit.

gdb/testsuite/ChangeLog:

	* lib/tuiterm.exp (Term::check_box_contents): New proc.

Change-Id: Icf795bf38dd9295e282a34eecc318a9cdbc73926
---
 gdb/testsuite/ChangeLog       |  4 ++++
 gdb/testsuite/lib/tuiterm.exp | 31 +++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index 9ac599b6f2d..0307745d879 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -600,6 +600,37 @@ namespace eval Term {
 	}
     }
 
+    # Check the contents of a box on the screen.  This is a little
+    # like check_contents, but doens't check the whole screen
+    # contents, only the contents of a single box.  This procedure
+    # includes (effectively) a call to check_box to ensure there is a
+    # box where expected, if there is then the contents of the box are
+    # matched against REGEXP.
+    proc check_box_contents {test_name x y width height regexp} {
+	variable _chars
+
+	set why [_check_box $x $y $width $height]
+	if {$why != ""} {
+	    dump_screen
+	    fail "$test_name (box check: $why)"
+	    return
+	}
+
+	# Now grab the contents of the box, join each line together
+	# with a newline character and match against REGEXP.
+	set result ""
+	for {set yy [expr {$y + 1}]} {$yy < [expr {$y + $height - 1}]} {incr yy} {
+	    for {set xx [expr {$x + 1}]} {$xx < [expr {$x + $width - 1}]} {incr xx} {
+		append result [lindex $_chars($xx,$yy) 0]
+	    }
+	    append result "\n"
+	}
+
+	if {![gdb_assert {[regexp -- $regexp $result]} $test_name]} {
+	    dump_screen
+	}
+    }
+
     # A debugging function to dump the current screen, with line
     # numbers.
     proc dump_screen {} {
-- 
2.14.5

[PATCH 5/6] gdb: Fix scrolling in TUI

[Andrew Burgess]

From: Tom Tromey <tom@tromey.com>

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
---
 gdb/ChangeLog                   |  6 ++++++
 gdb/testsuite/ChangeLog         |  8 ++++++++
 gdb/testsuite/gdb.tui/basic.exp | 13 +++++++++++++
 gdb/testsuite/lib/tuiterm.exp   | 16 ++++++++++------
 gdb/tui/tui-source.c            | 23 +++++++++++++----------
 5 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/gdb/testsuite/gdb.tui/basic.exp b/gdb/testsuite/gdb.tui/basic.exp
index c3a3fdd4f5e..be822f8a915 100644
--- a/gdb/testsuite/gdb.tui/basic.exp
+++ b/gdb/testsuite/gdb.tui/basic.exp
@@ -35,6 +35,19 @@ 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 [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 [string_to_regexp $line]] \
+	&& [Term::get_line 1] != $line\
+	&& [Term::get_line 2] == $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 0307745d879..7adaf1b71ab 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
@@ -490,7 +494,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
@@ -682,14 +686,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 1503cd4c636..13f2dc7cfe1 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -136,26 +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 ())
-	l.u.line_no = start_line_or_addr.u.line_no;
-      if (l.u.line_no <= 0)
-	l.u.line_no = 1;
+	  && line_no > offsets->size ())
+	line_no = start_line_or_addr.u.line_no;
+      if (line_no <= 0)
+	line_no = 1;
 
-      print_source_lines (s, l.u.line_no, l.u.line_no + 1, 0);
+      cursal.line = line_no;
+      update_source_window (arch, cursal);
     }
 }
 
-- 
2.14.5

Re: [PATCH 1/6] gdb/testsuite/tui: Always dump_screen when asked

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> The Term::dump_screen routine currently dumps the screen using calls
Andrew> to 'verbose', this means it will only dump the screen when the
Andrew> testsuite is running in verbose mode.

Andrew> However, the Term::dump_screen is most often called when a test fails,
Andrew> in this case I think it is useful to have the screen dumped even when
Andrew> we're not in verbose mode.

Andrew> This commit changes the calls to 'verbose' to be 'verbose -log' so we
Andrew> always get the screen dump.

Andrew> gdb/testsuite/ChangeLog:

Andrew> 	* lib/tuiterm.exp (Term::dump_screen): Always dump the screen when
Andrew> 	called.

Seems reasonable to me.  Thanks.

Tom

Re: [PATCH 2/6] gdb/testsuite/tui: Split enter_tui into two procs

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Split Term::enter_tui into two procedures, a core which does the
Andrew> setup, but doesn't actually enable tui mode, and the old enter_tui
Andrew> that calls the new core, and then enables tui mode.

Andrew> This is going to be useful in a later commit.

Andrew> gdb/testsuite/ChangeLog:

Andrew> 	* lib/tuiterm.exp (Term::prepare_for_tui): New proc.
Andrew> 	(Term::enter_tui): Use Term::prepare_for_tui.

Thanks, this looks good.

Tom

Re: [PATCH 3/6] gdb/testsuite/tui: Introduce check_box_contents

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> A new test procedure for matching the contents of one screen box
Andrew> against a regexp.  This can be used to match the contents of one TUI
Andrew> window against a regexp without any of the borders, or other windows
Andrew> being included in the matched output (as is currently the case with
Andrew> check_contents).

Andrew> This will be used in a later commit.

Andrew> gdb/testsuite/ChangeLog:

Andrew> 	* lib/tuiterm.exp (Term::check_box_contents): New proc.

Thanks, this looks good.

Tom

Re: [PATCH 4/6] gdb/tui: Fix 'layout asm' before the inferior has started

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> gdb/ChangeLog:

Andrew> 	* tui/tui.c (tui_enable): Register tui hooks after calling
Andrew> 	tui_display_main.

Tricky but with the comment, it seems fine to me.
Thanks for finding & fixing this.

Tom

Re: [PATCH 5/6] gdb: Fix scrolling in TUI

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> gdb/ChangeLog
Andrew> 2019-12-22  Tom Tromey  <tom@tromey.com>

Andrew> 	PR tui/18932:
Andrew> 	* tui/tui-source.c (tui_source_window::do_scroll_vertical): Call
Andrew> 	update_source_window, not print_source_lines.

Obviously this one is fine by me :-)

Tom

Re: [PATCH 6/6] gdb/tui: Link source and assembler scrolling .... again

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> gdb/ChangeLog:

Andrew> 	* tui/tui-source.c (tui_source_window::do_scroll_vertical): Update
Andrew> 	all source windows, and maintain horizontal scroll status while
Andrew> 	doing so.

Andrew> gdb/testsuite/ChangeLog:

Andrew> 	* gdb.tui/basic.exp: Add more scrolling tests.

Looks good, thanks.  And, special thanks for the test case.

Tom

Re: [PATCH 0/6] Vertical scrolling and another small bug fix

[Tom Tromey]

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

Re: [PATCH 0/6] Vertical scrolling and another small bug fix

[Andrew Burgess]

* 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