Make "list" work again in TUI
Commit Message
PR tui/18932 noted that commmit 0986c7 ("Replace TUI's select_frame
hook") regressed the ability to use the "list" command. Previously
one could use "list" to temporarily change what was displayed in the
source window, but after that patch, the "list" command seems to have
no effect -- "seems to", because what actually happens is that the
list command does change the content, but
tui_refresh_frame_and_register_information immediately changes it
back.
This patch makes the "list" command work again by adding some caching.
Now the TUI tracks the previously displayed frame, PC, and inferior;
and only updates the display if one of these was changed by the
previous command.
gdb/ChangeLog
2018-09-08 Tom Tromey <tom@tromey.com>
PR tui/18932:
* tui/tui-hooks.c (last_frame_id, last_frame_pc, last_inferior):
New globals.
(clear_inferior_cache): New function.
(tui_refresh_frame_and_register_information): Cache frame, PC, and
inferior to decide when to update.
(_initialize_tui_hooks): Attach clear_inferior_cache to observer.
---
gdb/ChangeLog | 10 +++++++
gdb/tui/tui-hooks.c | 80 +++++++++++++++++++++++++++++++++++++++--------------
2 files changed, 70 insertions(+), 20 deletions(-)
Comments
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
Tom> This patch makes the "list" command work again by adding some caching.
Tom> Now the TUI tracks the previously displayed frame, PC, and inferior;
Tom> and only updates the display if one of these was changed by the
Tom> previous command.
I have a question about this one now.
In the bug the original poster said that to get back to the current
location, he would use the "frame" command. This doesn't work with my
patch, and I'm not sure how it would have worked in the past.
One part of how the TUI links to the CLI is very convoluted -- there is
a special case in tui-out to notice how the "list" command emits fields
and newlines and to use that to refresh the window. I'd rather not do
anything this roundabout and fragile...
Other parts of the CLI use #ifdef TUI to decide what action to take when
the TUI is available. This seems like an "old school" approach though I
must say I prefer its directness. I wouldn't mind rewriting "list" to
do this.
Another option might be to have the frame command unconditionally notify
some observer. Then the TUI could listen for this.
Anyway, I'd appreciate comments on which approach CLI/TUI integration
ought to ideally take. I don't know the history here and there aren't
guiding comments that I could find.
thanks,
Tom
On 09/09/2018 07:26 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
>
> Tom> This patch makes the "list" command work again by adding some caching.
> Tom> Now the TUI tracks the previously displayed frame, PC, and inferior;
> Tom> and only updates the display if one of these was changed by the
> Tom> previous command.
>
> I have a question about this one now.
>
> In the bug the original poster said that to get back to the current
> location, he would use the "frame" command. This doesn't work with my
> patch, and I'm not sure how it would have worked in the past.
I used to do that too.
>
> One part of how the TUI links to the CLI is very convoluted -- there is
> a special case in tui-out to notice how the "list" command emits fields
> and newlines and to use that to refresh the window. I'd rather not do
> anything this roundabout and fragile...
Yeah.
>
> Other parts of the CLI use #ifdef TUI to decide what action to take when
> the TUI is available. This seems like an "old school" approach though I
> must say I prefer its directness. I wouldn't mind rewriting "list" to
> do this.
>
> Another option might be to have the frame command unconditionally notify
> some observer. Then the TUI could listen for this.
>
> Anyway, I'd appreciate comments on which approach CLI/TUI integration
> ought to ideally take. I don't know the history here and there aren't
> guiding comments that I could find.
I'm under the impression that the TUI was added along with the
major HP dump/merge back in the day, meaning original design history
is probably lost. I may be wrong.
Offhand, it seems to me that the TUI should update its listed source
whenever the CLI's current source changes. I.e., the TUI's source
window should be updated whenever set_current_source_symtab_and_line is
used to update the current source & line for the CLI's "list". Which
suggests that for "frame" TUI re-centering, an observer notification from
within set_current_source_symtab_and_line would work.
If we always updated the source window from a TUI observer for
that notification, could we make tui_refresh_frame_and_register_information
just never update the source window? I.e., remove the select_source_symtab
call from tui_refresh_frame_and_register_information or something along
those lines. That assumes that if the frame changed, then presumably,
set_current_source_symtab_and_line will have already been called, which
it seems is true. (if it isn't, then "list" won't be updated either, and
we're back to the argument about keeping "list" and the TUI in sync.).
That would eliminate the need for the caching.
Thanks,
Pedro Alves
On 09/12/2018 01:20 PM, Pedro Alves wrote:
> On 09/09/2018 07:26 PM, Tom Tromey wrote:
>>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
>>
>> Tom> This patch makes the "list" command work again by adding some caching.
>> Tom> Now the TUI tracks the previously displayed frame, PC, and inferior;
>> Tom> and only updates the display if one of these was changed by the
>> Tom> previous command.
>>
>> I have a question about this one now.
>>
>> In the bug the original poster said that to get back to the current
>> location, he would use the "frame" command. This doesn't work with my
>> patch, and I'm not sure how it would have worked in the past.
>
> I used to do that too.
>
>>
>> One part of how the TUI links to the CLI is very convoluted -- there is
>> a special case in tui-out to notice how the "list" command emits fields
>> and newlines and to use that to refresh the window. I'd rather not do
>> anything this roundabout and fragile...
>
> Yeah.
>
>>
>> Other parts of the CLI use #ifdef TUI to decide what action to take when
>> the TUI is available. This seems like an "old school" approach though I
>> must say I prefer its directness. I wouldn't mind rewriting "list" to
>> do this.
>>
>> Another option might be to have the frame command unconditionally notify
>> some observer. Then the TUI could listen for this.
>>
>> Anyway, I'd appreciate comments on which approach CLI/TUI integration
>> ought to ideally take. I don't know the history here and there aren't
>> guiding comments that I could find.
>
> I'm under the impression that the TUI was added along with the
> major HP dump/merge back in the day, meaning original design history
> is probably lost. I may be wrong.
>
> Offhand, it seems to me that the TUI should update its listed source
> whenever the CLI's current source changes. I.e., the TUI's source
> window should be updated whenever set_current_source_symtab_and_line is
> used to update the current source & line for the CLI's "list". Which
> suggests that for "frame" TUI re-centering, an observer notification from
> within set_current_source_symtab_and_line would work.
>
> If we always updated the source window from a TUI observer for
> that notification, could we make tui_refresh_frame_and_register_information
> just never update the source window? I.e., remove the select_source_symtab
> call from tui_refresh_frame_and_register_information or something along
> those lines. That assumes that if the frame changed, then presumably,
> set_current_source_symtab_and_line will have already been called, which
> it seems is true. (if it isn't, then "list" won't be updated either, and
> we're back to the argument about keeping "list" and the TUI in sync.).
> That would eliminate the need for the caching.
>
Something to keep in mind, though, is avoiding refreshes/updates
caused by temporary thread stops while the target is running.
E.g., if we have breakpoints with conditions that eval false,
or e.g. a software watchpoint, we don't want the intermediate
stops to cause a flurry of source window refreshes. I'm not
sure whether that would invalidate what I said above, depends on
when set_current_source_symtab_and_line is called, I suppose,
but now I'm thinking that it probably does.
That might suggest instead to start by seeing about trying to remove
the select_source_symtab call from tui_refresh_frame_and_register_information.
Why do we need that? Can we remove it and instead see about making
tui_refresh_frame_and_register_information update the source window
from get_current_source_symtab_and_line, if set_current_source_symtab_and_line
was meanwhile called? I.e., refresh the source window if something changed
"list"'s current source&line.
Thanks,
Pedro Alves
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> Something to keep in mind, though, is avoiding refreshes/updates
Pedro> caused by temporary thread stops while the target is running.
Pedro> E.g., if we have breakpoints with conditions that eval false,
Pedro> or e.g. a software watchpoint, we don't want the intermediate
Pedro> stops to cause a flurry of source window refreshes. I'm not
Pedro> sure whether that would invalidate what I said above, depends on
Pedro> when set_current_source_symtab_and_line is called, I suppose,
Pedro> but now I'm thinking that it probably does.
The troubling call is to set_current_sal_from_frame in normal_stop.
It seems like that one will be called frequently.
However one idea might be to have the TUI observer just set a flag, and
then continue to do the actual work in the prompt hook.
Pedro> That might suggest instead to start by seeing about trying to remove
Pedro> the select_source_symtab call from tui_refresh_frame_and_register_information.
Pedro> Why do we need that? Can we remove it and instead see about making
Pedro> tui_refresh_frame_and_register_information update the source window
Pedro> from get_current_source_symtab_and_line, if set_current_source_symtab_and_line
Pedro> was meanwhile called? I.e., refresh the source window if something changed
Pedro> "list"'s current source&line.
I'm going to give it a try.
Tom
@@ -107,6 +107,26 @@ tui_event_modify_breakpoint (struct breakpoint *b)
tui_update_all_breakpoint_info ();
}
+
+/* Cache the last state of the TUI display, so that we can tell if
+ something has changed. In particular, if the frame changed, or if
+ the PC changed, or if the current inferior changed, then the
+ display will update. Otherwise, it will not. This caching is
+ needed to let "list" affect the source window display without
+ resetting. See PR tui/18932. */
+static struct frame_id last_frame_id = null_frame_id;
+static CORE_ADDR last_frame_pc = 0;
+static inferior *last_inferior = nullptr;
+
+/* Clear the inferior cache when the inferior is destroyed. */
+
+static void
+clear_inferior_cache (inferior *inf)
+{
+ if (last_inferior == inf)
+ last_inferior = nullptr;
+}
+
/* 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.
@@ -117,36 +137,55 @@ static void
tui_refresh_frame_and_register_information (int registers_too_p)
{
struct frame_info *fi;
- CORE_ADDR pc;
- int frame_info_changed_p;
+ CORE_ADDR pc = 0;
+ int frame_info_changed_p = 0;
if (!has_stack_frames ())
- return;
+ {
+ last_frame_id = null_frame_id;
+ last_frame_pc = 0;
+ last_inferior = nullptr;
+ return;
+ }
target_terminal::scoped_restore_terminal_state term_state;
target_terminal::ours_for_output ();
fi = get_selected_frame (NULL);
- /* Ensure that symbols for this frame are read in. Also, determine
- the source language of this frame, and switch to it if
- desired. */
- if (get_frame_pc_if_available (fi, &pc))
+ bool have_pc = get_frame_pc_if_available (fi, &pc);
+
+ struct frame_id fid = get_frame_id (fi);
+ if ((!frame_id_p (last_frame_id)
+ || !frame_id_eq (last_frame_id, fid)
+ || pc != last_frame_pc
+ || current_inferior () != last_inferior))
{
- struct symtab *s;
-
- s = find_pc_line_symtab (pc);
- /* elz: This if here fixes the problem with the pc not being
- displayed in the tui asm layout, with no debug symbols. The
- value of s would be 0 here, and select_source_symtab would
- abort the command by calling the 'error' function. */
- if (s)
- select_source_symtab (s);
+ /* Display the frame position (even if there is no symbols or
+ the PC is not known). */
+
+ last_frame_id = fid;
+ last_frame_pc = pc;
+ last_inferior = current_inferior ();
+
+ /* Ensure that symbols for this frame are read in. Also, determine
+ the source language of this frame, and switch to it if
+ desired. */
+ if (have_pc)
+ {
+ struct symtab *s;
+
+ s = find_pc_line_symtab (pc);
+ /* elz: This if here fixes the problem with the pc not being
+ displayed in the tui asm layout, with no debug symbols. The
+ value of s would be 0 here, and select_source_symtab would
+ abort the command by calling the 'error' function. */
+ if (s)
+ select_source_symtab (s);
+ }
+
+ frame_info_changed_p = tui_show_frame_info (fi);
}
- /* Display the frame position (even if there is no symbols or the PC
- is not known). */
- frame_info_changed_p = tui_show_frame_info (fi);
-
/* Refresh the register window if it's visible. */
if (tui_is_window_visible (DATA_WIN)
&& (frame_info_changed_p || registers_too_p))
@@ -271,4 +310,5 @@ _initialize_tui_hooks (void)
{
/* Install the permanent hooks. */
gdb::observers::new_objfile.attach (tui_new_objfile_hook);
+ gdb::observers::inferior_removed.attach (clear_inferior_cache);
}