[RFC] Call tui_before_prompt in do_scroll_vertical

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

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

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