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