[3/3] Replace TUI's select_frame hook (PR tui/13378)
Commit Message
[ I elected to go with making the print_frame_info_listing hook a no-op, since
it does not seem to regress anything.
I noticed that "layout regs" was somewhat broken now that
tui_refresh_frame_and_register_information is called twice following a normal
stop: once in tui_normal_stop and then in tui_before_prompt. The second call
removes any highlights done to the individual registers during the first
call, because the function notices that the current snapshot of register
values is the same as the one taken during the first call. So effectively
register changes are no longer highlighted in "layout regs" since the
highlights immediately get removed.
I don't think we should refresh register information at all in
tui_before_prompt since this observer is intended to only update frame
information following a call to "up", "down", "frame", etc. Only after the
inferior has run for a bit could registers have changed. So this patch adds
the parameter registers_too_p to tui_refresh_frame_and_register_information
to indicate wheher we should update registers too, and updates
tui_before_prompt and tui_normal_stop accordingly. ]
The select_frame hook is used by TUI to update TUI's frame and register
information following changes to the selected frame. The problem with
this hook is that it gets called after every single frame change, even
if the frame change is only temporary or internal. This is the primary
cause of flickering and slowdown when running the inferior under TUI
with conditional breakpoints set. Internal GDB events are the source of
many calls to select_frame and these internal events are triggered
frequently, especially when a few conditional breakpoints are set.
This patch removes the select_frame hook altogether and instead makes
the frame and register information get updated in two key places (using
observers): after an inferior stops, and right before displaying a
prompt. The latter hook covers the case when frame information must be
updated following a call to "up", "down" or "frame", and the former
covers the case when frame and register information must be updated
after a call to "continue", "step", etc. or after the inferior stops in
async execution mode. Together these hooks should cover all the cases
when frame information ought to be refreshed (and when the relevant
windows ought to be subsequently updated).
The print_frame_info_listing hook is also effectively obsolete now, but
it still must be set while the TUI is active because its caller
print_frame_info will otherwise assume that the CLI is active, and will
print the frame informaion accordingly. So this patch also sets the
print_frame_info_listing hook to a dummy callback, in lieu of outright
removing it yet.
Effectively, with this patch, frame/PC changes that do not immediately
precede an inferior-stop event or a prompt display event no longer cause
TUI's frame and register information to be updated.
And as a result of this change and of the previous change to
tui_show_frame_info, the TUI is much more disciplined about updating the
screen, and so the flicker as described in the PR is totally gone.
gdb/ChangeLog:
* frame.c (select_frame): Remove reference to
deprecated_selected_frame_level_changed_hook.
* frame.h (deprecated_selected_frame_level_changed_hook): Remove
declaration.
* stack.c (deprecated_selected_frame_level_changed_hook):
Likewise.
* tui/tui-hooks.c (tui_selected_frame_level_changed_hook):
Rename to ...
(tui_refresh_frame_and_register_information): ... this. Bail
out if there is no stack.
(tui_print_frame_info_listing_hook): Rename to ...
(tui_dummy_print_frame_info_listing_hook): ... this.
(tui_before_prompt): New function.
(tui_normal_stop): New function.
(tui_before_prompt_observer): New observer.
(tui_normal_stop_observer): New observer.
(tui_install_hooks): Set
deprecated_print_frame_info_listing_hook to
tui_dummy_print_frame_info_listing_hook. Register
tui_before_prompt_observer to call tui_before_prompt and
tui_normal_stop_observer to call tui_normal_stop. Remove
reference to deprecated_selected_frame_level_changed_hook.
(tui_remove_hooks): Detach and unset tui_before_prompt_observer
and tui_normal_stop_observer. Remove reference to
deprecated_selected_frame_level_changed_hook.
---
gdb/frame.c | 2 --
gdb/frame.h | 2 --
gdb/stack.c | 2 --
gdb/tui/tui-hooks.c | 70 +++++++++++++++++++++++++++++++++++++++--------------
4 files changed, 52 insertions(+), 24 deletions(-)
Comments
On 06/30/2015 05:40 PM, Patrick Palka wrote:
> [ I elected to go with making the print_frame_info_listing hook a no-op, since
> it does not seem to regress anything.
>
> I noticed that "layout regs" was somewhat broken now that
> tui_refresh_frame_and_register_information is called twice following a normal
> stop: once in tui_normal_stop and then in tui_before_prompt. The second call
> removes any highlights done to the individual registers during the first
> call, because the function notices that the current snapshot of register
> values is the same as the one taken during the first call. So effectively
> register changes are no longer highlighted in "layout regs" since the
> highlights immediately get removed.
>
> I don't think we should refresh register information at all in
> tui_before_prompt since this observer is intended to only update frame
> information following a call to "up", "down", "frame", etc. Only after the
> inferior has run for a bit could registers have changed. So this patch adds
> the parameter registers_too_p to tui_refresh_frame_and_register_information
> to indicate wheher we should update registers too, and updates
> tui_before_prompt and tui_normal_stop accordingly. ]
Hmm, what about when the user changes registers with "print $rax = 1" etc.?
Do we end up with stale contents?
Thanks,
Pedro Alves
On Tue, Jun 30, 2015 at 1:07 PM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2015 05:40 PM, Patrick Palka wrote:
>> [ I elected to go with making the print_frame_info_listing hook a no-op, since
>> it does not seem to regress anything.
>>
>> I noticed that "layout regs" was somewhat broken now that
>> tui_refresh_frame_and_register_information is called twice following a normal
>> stop: once in tui_normal_stop and then in tui_before_prompt. The second call
>> removes any highlights done to the individual registers during the first
>> call, because the function notices that the current snapshot of register
>> values is the same as the one taken during the first call. So effectively
>> register changes are no longer highlighted in "layout regs" since the
>> highlights immediately get removed.
>>
>> I don't think we should refresh register information at all in
>> tui_before_prompt since this observer is intended to only update frame
>> information following a call to "up", "down", "frame", etc. Only after the
>> inferior has run for a bit could registers have changed. So this patch adds
>> the parameter registers_too_p to tui_refresh_frame_and_register_information
>> to indicate wheher we should update registers too, and updates
>> tui_before_prompt and tui_normal_stop accordingly. ]
>
> Hmm, what about when the user changes registers with "print $rax = 1" etc.?
> Do we end up with stale contents?
Apparently not, thanks to our deprecated_register_changed_hook called
from value_assign. So many hooks!
On 06/30/2015 06:10 PM, Patrick Palka wrote:
>> Hmm, what about when the user changes registers with "print $rax = 1" etc.?
>> Do we end up with stale contents?
>
> Apparently not, thanks to our deprecated_register_changed_hook called
> from value_assign. So many hooks!
Phew! And luckily there's an equivalent registers_changed observer
we could use instead too.
Patch is OK, just please mention tui_register_changed_hook ...
> +/* Observer for the before_prompt notification. */
> +
> +static void
> +tui_before_prompt (const char *current_gdb_prompt)
> +{
> + /* This refresh is intended to catch changes to the selected frame following
> + a call to "up", "down" or "frame". As such we don't necessarily want to
> + refresh registers here as they could not have changed. Registers will be
> + refreshed after a normal stop. */
... here too.
Awesome. Glad that this is finally fixed.
Thanks,
Pedro Alves
On Tue, Jun 30, 2015 at 1:32 PM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2015 06:10 PM, Patrick Palka wrote:
>
>>> Hmm, what about when the user changes registers with "print $rax = 1" etc.?
>>> Do we end up with stale contents?
>>
>> Apparently not, thanks to our deprecated_register_changed_hook called
>> from value_assign. So many hooks!
>
> Phew! And luckily there's an equivalent registers_changed observer
> we could use instead too.
How convenient.
>
> Patch is OK, just please mention tui_register_changed_hook ...
>
>> +/* Observer for the before_prompt notification. */
>> +
>> +static void
>> +tui_before_prompt (const char *current_gdb_prompt)
>> +{
>> + /* This refresh is intended to catch changes to the selected frame following
>> + a call to "up", "down" or "frame". As such we don't necessarily want to
>> + refresh registers here as they could not have changed. Registers will be
>> + refreshed after a normal stop. */
>
> ... here too.
>
> Awesome. Glad that this is finally fixed.
Me too. The results are quite nice. TUI is silky smooth.
>
> Thanks,
> Pedro Alves
>
On Tue, Jun 30, 2015 at 1:07 PM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2015 05:40 PM, Patrick Palka wrote:
>> [ I elected to go with making the print_frame_info_listing hook a no-op, since
>> it does not seem to regress anything.
>>
>> I noticed that "layout regs" was somewhat broken now that
>> tui_refresh_frame_and_register_information is called twice following a normal
>> stop: once in tui_normal_stop and then in tui_before_prompt. The second call
>> removes any highlights done to the individual registers during the first
>> call, because the function notices that the current snapshot of register
>> values is the same as the one taken during the first call. So effectively
>> register changes are no longer highlighted in "layout regs" since the
>> highlights immediately get removed.
>>
>> I don't think we should refresh register information at all in
>> tui_before_prompt since this observer is intended to only update frame
>> information following a call to "up", "down", "frame", etc. Only after the
>> inferior has run for a bit could registers have changed. So this patch adds
>> the parameter registers_too_p to tui_refresh_frame_and_register_information
>> to indicate wheher we should update registers too, and updates
>> tui_before_prompt and tui_normal_stop accordingly. ]
>
> Hmm, what about when the user changes registers with "print $rax = 1" etc.?
> Do we end up with stale contents?
Unfortunately registers _do_ change following a call to "up", "down"
or "frame", because the snapshot of register values is per-frame. So
this patch introduced a regression -- it made it so that TUI does not
update register information following such a call even though earlier
versions do. I sent a patch to fix this.
@@ -1576,8 +1576,6 @@ select_frame (struct frame_info *fi)
selected_frame = fi;
/* NOTE: cagney/2002-05-04: FI can be NULL. This occurs when the
frame is being invalidated. */
- if (deprecated_selected_frame_level_changed_hook)
- deprecated_selected_frame_level_changed_hook (frame_relative_level (fi));
/* FIXME: kseitz/2002-08-28: It would be nice to call
selected_frame_level_changed_event() right here, but due to limitations
@@ -763,8 +763,6 @@ extern void args_info (char *, int);
extern void locals_info (char *, int);
-extern void (*deprecated_selected_frame_level_changed_hook) (int);
-
extern void return_command (char *, int);
/* Set FRAME's unwinder temporarily, so that we can call a sniffer.
@@ -52,8 +52,6 @@
#include "symfile.h"
#include "extension.h"
-void (*deprecated_selected_frame_level_changed_hook) (int);
-
/* The possible choices of "set print frame-arguments", and the value
of this setting. */
@@ -119,18 +119,19 @@ tui_about_to_proceed (void)
tui_target_has_run = 1;
}
-/* The selected frame has changed. This is happens after a target
- stop or when the user explicitly changes the frame
- (up/down/thread/...). */
+/* 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.
+
+ REGISTERS_TOO_P controls whether to refresh our register information. */
+
static void
-tui_selected_frame_level_changed_hook (int level)
+tui_refresh_frame_and_register_information (int registers_too_p)
{
struct frame_info *fi;
CORE_ADDR pc;
struct cleanup *old_chain;
- /* Negative level means that the selected frame was cleared. */
- if (level < 0)
+ if (!has_stack_frames ())
return;
old_chain = make_cleanup_restore_target_terminal ();
@@ -158,7 +159,7 @@ tui_selected_frame_level_changed_hook (int level)
tui_show_frame_info (fi);
/* Refresh the register window if it's visible. */
- if (tui_is_window_visible (DATA_WIN))
+ if (tui_is_window_visible (DATA_WIN) && registers_too_p)
{
tui_refreshing_registers = 1;
tui_check_data_values (fi);
@@ -168,15 +169,15 @@ tui_selected_frame_level_changed_hook (int level)
do_cleanups (old_chain);
}
-/* Called from print_frame_info to list the line we stopped in. */
+/* Dummy callback for deprecated_print_frame_info_listing_hook which is called
+ from print_frame_info. */
+
static void
-tui_print_frame_info_listing_hook (struct symtab *s,
- int line,
- int stopline,
- int noerror)
+tui_dummy_print_frame_info_listing_hook (struct symtab *s,
+ int line,
+ int stopline,
+ int noerror)
{
- select_source_symtab (s);
- tui_show_frame_info (get_selected_frame (NULL));
}
/* Perform all necessary cleanups regarding our module's inferior data
@@ -191,21 +192,47 @@ tui_inferior_exit (struct inferior *inf)
tui_display_main ();
}
+/* Observer for the before_prompt notification. */
+
+static void
+tui_before_prompt (const char *current_gdb_prompt)
+{
+ /* This refresh is intended to catch changes to the selected frame following
+ a call to "up", "down" or "frame". As such we don't necessarily want to
+ refresh registers here as they could not have changed. Registers will be
+ refreshed after a normal stop. */
+ tui_refresh_frame_and_register_information (/*registers_too_p=*/0);
+}
+
+/* Observer for the normal_stop notification. */
+
+static void
+tui_normal_stop (struct bpstats *bs, int print_frame)
+{
+ /* This refresh is intended to catch changes to the selected frame and to
+ registers following a normal stop. */
+ tui_refresh_frame_and_register_information (/*registers_too_p=*/1);
+}
+
/* Observers created when installing TUI hooks. */
static struct observer *tui_bp_created_observer;
static struct observer *tui_bp_deleted_observer;
static struct observer *tui_bp_modified_observer;
static struct observer *tui_inferior_exit_observer;
static struct observer *tui_about_to_proceed_observer;
+static struct observer *tui_before_prompt_observer;
+static struct observer *tui_normal_stop_observer;
/* Install the TUI specific hooks. */
void
tui_install_hooks (void)
{
- deprecated_selected_frame_level_changed_hook
- = tui_selected_frame_level_changed_hook;
+ /* If this hook is not set to something then print_frame_info will
+ assume that the CLI, not the TUI, is active, and will print the frame info
+ for us in such a way that we are not prepared to handle. This hook is
+ otherwise effectively obsolete. */
deprecated_print_frame_info_listing_hook
- = tui_print_frame_info_listing_hook;
+ = tui_dummy_print_frame_info_listing_hook;
/* Install the event hooks. */
tui_bp_created_observer
@@ -218,6 +245,10 @@ tui_install_hooks (void)
= observer_attach_inferior_exit (tui_inferior_exit);
tui_about_to_proceed_observer
= observer_attach_about_to_proceed (tui_about_to_proceed);
+ tui_before_prompt_observer
+ = observer_attach_before_prompt (tui_before_prompt);
+ tui_normal_stop_observer
+ = observer_attach_normal_stop (tui_normal_stop);
deprecated_register_changed_hook = tui_register_changed_hook;
}
@@ -226,7 +257,6 @@ tui_install_hooks (void)
void
tui_remove_hooks (void)
{
- deprecated_selected_frame_level_changed_hook = 0;
deprecated_print_frame_info_listing_hook = 0;
deprecated_query_hook = 0;
deprecated_register_changed_hook = 0;
@@ -242,6 +272,10 @@ tui_remove_hooks (void)
tui_inferior_exit_observer = NULL;
observer_detach_about_to_proceed (tui_about_to_proceed_observer);
tui_about_to_proceed_observer = NULL;
+ observer_detach_before_prompt (tui_before_prompt_observer);
+ tui_before_prompt_observer = NULL;
+ observer_detach_normal_stop (tui_normal_stop_observer);
+ tui_normal_stop_observer = NULL;
}
void _initialize_tui_hooks (void);