[RFC] Call tui_before_prompt in do_scroll_vertical
Commit Message
>>>>> "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
Comments
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
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
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
* 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
@@ -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.
@@ -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.
@@ -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"
@@ -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
}
}
@@ -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);
}
}