This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Fix TUI flicker resulting from frequent frame changes (PR tui/13378)
- From: Patrick Palka <patrick at parcs dot ath dot cx>
- To: Pedro Alves <palves at redhat dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Fri, 26 Jun 2015 10:09:45 -0400
- Subject: Re: [PATCH] Fix TUI flicker resulting from frequent frame changes (PR tui/13378)
- Authentication-results: sourceware.org; auth=none
- References: <1434688566-2549-1-git-send-email-patrick at parcs dot ath dot cx> <558D5562 dot 5090106 at redhat dot com>
On Fri, Jun 26, 2015 at 9:36 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/19/2015 05:36 AM, Patrick Palka wrote:
>> This patch fixes the perceived flicker of the TUI screen (and subsequent
>> slowdown) that most apparent when running an inferior while a
>> conditional breakpoint is active. The cause of the flicker is that each
>> internal event GDB responds to is accompanied by a multitude of calls to
>> select_frame() and each such call forces the TUI screen to be refreshed.
>> We would like to not update the TUI screen after each such frame change.
>>
>> The fix for this issue is pretty straightforward: do not update the TUI
>> screen when select_frame() gets called while synchronous execution of
>> the inferior is enabled. This works because synchronous execution
>> remains enabled during the processing of internal events. And since
>> during synchronous execution the user has no control of the TUI anyway,
>> it does not hurt to avoid updating the screen then.
>>
>> The select_frame hook is still undesirable and should be removed, but
>> in the meantime this fix is seemingly an effective approximation of a
>> more disciplined approach of updating the TUI screen.
>>
>> [ When the inferior is running while sync_execution is disabled, e.g.
>> via "continue&" it looks like GDB lacks access to frame information
>> thorughout -- and the hook never gets called -- so seemingly no
>> worries in that case. ]
>
> That can't be right. Whenever GDB stops for any (thread) event, it'll
> select the current frame. So try putting a conditional breakpoint (whose
> condition fails) on a tight loop, and then continuing, like:
>
> (gdb) b inside_loop if 0
> (gdb) c&
>
> I see lots of TUI flicker doing this. E.g.:
>
> (top-gdb) bt
> #0 tui_refresh_win (win_info=0xdbb3e0 <exec_info>) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-wingeneral.c:38
> #1 0x000000000052fcfa in tui_show_exec_info_content (win_info=0x1a2e0e0) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-winsource.c:569
> #2 0x000000000052fd8a in tui_update_exec_info (win_info=0x1a2e0e0) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-winsource.c:598
> #3 0x000000000052bc62 in tui_show_frame_info (fi=0x1dbc2a0) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-stack.c:429
> #4 0x0000000000526656 in tui_selected_frame_level_changed_hook (level=0) at /home/pedro/gdb/mygit/build/../src/gdb/tui/tui-hooks.c:158
> #5 0x000000000076a5b9 in select_frame (fi=0x1dbc2a0) at /home/pedro/gdb/mygit/build/../src/gdb/frame.c:1580
> #6 0x00000000005ac80d in bpstat_check_breakpoint_conditions (bs=0x3d42a30, ptid=...) at /home/pedro/gdb/mygit/build/../src/gdb/breakpoint.c:5414
> #7 0x00000000005acc05 in bpstat_stop_status (aspace=0x1034a40, bp_addr=4196369, ptid=..., ws=0x7fffffffd210) at /home/pedro/gdb/mygit/build/../src/gdb/breakpoint.c:5616
> #8 0x000000000062e6bc in handle_signal_stop (ecs=0x7fffffffd1f0) at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:4529
> #9 0x000000000062dbce in handle_inferior_event_1 (ecs=0x7fffffffd1f0) at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:4195
> #10 0x000000000062dc70 in handle_inferior_event (ecs=0x7fffffffd1f0) at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:4220
> #11 0x000000000062bfb9 in fetch_inferior_event (client_data=0x0) at /home/pedro/gdb/mygit/build/../src/gdb/infrun.c:3328
I see.. I did not catch that..
>
> How about we instead find some more higher level place
> to refresh? I'm thinking that maybe whenever we display the prompt
> might be a good place (before_prompt observer). With both the prompt
> and normal_stop, we cover every case that needs a refresh, I think.
I can imagine a problem with this. It seems that when the screen gets
refreshed following a frame change, any scrolling that the user did in
the source/asm windows would get undone because the screen gets
re-centered on the currently executing line. You can see this by
doing "frame 0", scrolling the window a bit and doing "frame 0" again:
the scrolling gets undone. So by naively refreshing the source/asm
windows before each prompt, we would undo scrolling for benign
commands such as "print 1 + 2", "bt", I think... This could be fixed
by being smarter about refreshing, by only refreshing the screen in
the before_prompt observer if the frame information/PC has changed.
>
> If we don't want that to always refresh at the prompt, we could still
> have the tui_selected_frame_level_changed_hook hook, but instead
> of having that cause an immediate refresh, it'd just set a
> flag to indicate that the next time we display the prompt, we should
> refresh everything. That way, multiple frame changes until we
> reach the prompt cause only one refresh. And if no frame changes,
> the flag is not set, so no refresh is done at the prompt.
> But if a simpler unconditional refresh at the prompt works, I'd
> go for that.
Setting a flag inside the hook would not be perfect anyway since it
seems that many times select_frame() is called on the already-selected
frame. It seems it would be better to only refresh the screen if the
frame/PC actually changes as mentioned above. This can be checked in
the observer itself -- no need for the hook, right?
>
> Thanks,
> Pedro Alves
>
>
>>
>> [ up/down etc still work properl of course ]
>>
>> [ I am not sure that the change in tui_on_sync_execution_done is
>> necessary/desirable. It seems normal_stop already calls
>> select_frame (get_current_frame ()) after sync_execution is toggled
>> off. ]
>>
>> gdb/ChangeLog:
>>
>> * tui/tui-hooks.c (tui_selected_frame_level_changed_hook): Don't
>> update the screen while synchronous execution is active.
>> * tui/tui-interp.c (tui_on_sync_execution_done): Make sure that
>> TUI's frame information is refreshed.
>> ---
>> gdb/tui/tui-hooks.c | 8 ++++++++
>> gdb/tui/tui-interp.c | 5 +++++
>> 2 files changed, 13 insertions(+)
>>
>> diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
>> index 8d84551..ca8358d 100644
>> --- a/gdb/tui/tui-hooks.c
>> +++ b/gdb/tui/tui-hooks.c
>> @@ -133,6 +133,14 @@ tui_selected_frame_level_changed_hook (int level)
>> if (level < 0)
>> return;
>>
>> + /* Do not respond to frame changes occurring while synchronous execution is
>> + enabled. Updating the screen in response to each such frame change just
>> + results in pointless flicker and slowdown. Once synchronous execution is
>> + done this hook will get called again to ensure that our frame information
>> + is refreshed. */
>> + if (sync_execution)
>> + return;
>> +
>> old_chain = make_cleanup_restore_target_terminal ();
>> target_terminal_ours_for_output ();
>>
>> diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
>> index 1a5639d..2477536 100644
>> --- a/gdb/tui/tui-interp.c
>> +++ b/gdb/tui/tui-interp.c
>> @@ -107,6 +107,11 @@ tui_on_sync_execution_done (void)
>> {
>> if (!interp_quiet_p (tui_interp))
>> display_gdb_prompt (NULL);
>> +
>> + /* Make sure our frame information is refreshed now that synchronous
>> + execution is done. */
>> + if (tui_active)
>> + deprecated_selected_frame_level_changed_hook (0);
>> }
>>
>> /* Observer for the command_error notification. */
>>
>
>