[3/3] Replace TUI's select_frame hook (PR tui/13378)

  This version adds a tui_normal_stop observer in place of augmenting the
tui_on_sync_execution_done observer.  And tui_refresh_frame_and_register_information
can now be made a static function.

The observer takes a print_frame parameter that is supposed to inform us
whether the frame should be printed.  This boolean seems to only be true for
when the inferior has exited.  Since tui_refresh_frame_and_register_information
already handles this case by checking has_stack_frames() this patch elects to
ignore this parameter in the observer.

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_before_prompt): New function.
	(tui_normal_stop): New function.
	(tui_before_prompt_observer): New observer.
	(tui_normal_stop_observer): New observer.
	(tui_install_hooks): 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 | 40 +++++++++++++++++++++++++++++++---------
 4 files changed, 31 insertions(+), 15 deletions(-)
  

On 06/30/2015 01:37 PM, Patrick Palka wrote:
> This version adds a tui_normal_stop observer in place of augmenting the
> tui_on_sync_execution_done observer.  And tui_refresh_frame_and_register_information
> can now be made a static function.
> 
> The observer takes a print_frame parameter that is supposed to inform us
> whether the frame should be printed.  This boolean seems to only be true for

s/only be true/only be false/

> when the inferior has exited.  Since tui_refresh_frame_and_register_information
> already handles this case by checking has_stack_frames() this patch elects to
> ignore this parameter in the observer.

This is OK.  I'll take a look at patch 2 soon.

Did you find that we still need deprecated_print_frame_info_listing_hook?

Thanks,
Pedro Alves
  

On Tue, Jun 30, 2015 at 10:08 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2015 01:37 PM, Patrick Palka wrote:
>> This version adds a tui_normal_stop observer in place of augmenting the
>> tui_on_sync_execution_done observer.  And tui_refresh_frame_and_register_information
>> can now be made a static function.
>>
>> The observer takes a print_frame parameter that is supposed to inform us
>> whether the frame should be printed.  This boolean seems to only be true for
>
> s/only be true/only be false/
>
>> when the inferior has exited.  Since tui_refresh_frame_and_register_information
>> already handles this case by checking has_stack_frames() this patch elects to
>> ignore this parameter in the observer.
>
> This is OK.  I'll take a look at patch 2 soon.
>
> Did you find that we still need deprecated_print_frame_info_listing_hook?

It still seems to be "necessary" -- at least, I can't outright remove it.

The only caller of deprecated_print_frame_info_listing_hook is in
print_frame_info and its use looks like this:

 if (deprecated_print_frame_info_listing_hook)
    (*deprecated_print_frame_info_listing_hook) (...);
 else
   { ... other code ... }

If I remove the hook by replacing the above code with

  { ... other code ... }

Then a regression occurs: the TUI decides to make sure that the
currently executing line always sits at the top of the window instead
of only scrolling the screen when the currently executing line is not
invisible.

But if I disable the hook by replacing the body of code in print_frame_info with

  if (deprecated_print_frame_info_listing_hook)
    ;
  else
    { ... other code ... }

Then everything seems to be OK.  So the code in  the else branch is
interfering with TUI somehow.  I will investigate further.

>
> Thanks,
> Pedro Alves
>
  

On Tue, Jun 30, 2015 at 10:54 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Tue, Jun 30, 2015 at 10:08 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 06/30/2015 01:37 PM, Patrick Palka wrote:
>>> This version adds a tui_normal_stop observer in place of augmenting the
>>> tui_on_sync_execution_done observer.  And tui_refresh_frame_and_register_information
>>> can now be made a static function.
>>>
>>> The observer takes a print_frame parameter that is supposed to inform us
>>> whether the frame should be printed.  This boolean seems to only be true for
>>
>> s/only be true/only be false/
>>
>>> when the inferior has exited.  Since tui_refresh_frame_and_register_information
>>> already handles this case by checking has_stack_frames() this patch elects to
>>> ignore this parameter in the observer.
>>
>> This is OK.  I'll take a look at patch 2 soon.
>>
>> Did you find that we still need deprecated_print_frame_info_listing_hook?
>
> It still seems to be "necessary" -- at least, I can't outright remove it.
>
> The only caller of deprecated_print_frame_info_listing_hook is in
> print_frame_info and its use looks like this:
>
>  if (deprecated_print_frame_info_listing_hook)
>     (*deprecated_print_frame_info_listing_hook) (...);
>  else
>    { ... other code ... }
>
> If I remove the hook by replacing the above code with
>
>   { ... other code ... }
>
> Then a regression occurs: the TUI decides to make sure that the
> currently executing line always sits at the top of the window instead
> of only scrolling the screen when the currently executing line is not
> invisible.

I meant to say not _visible_.
  

On Tue, Jun 30, 2015 at 10:54 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Tue, Jun 30, 2015 at 10:08 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 06/30/2015 01:37 PM, Patrick Palka wrote:
>>> This version adds a tui_normal_stop observer in place of augmenting the
>>> tui_on_sync_execution_done observer.  And tui_refresh_frame_and_register_information
>>> can now be made a static function.
>>>
>>> The observer takes a print_frame parameter that is supposed to inform us
>>> whether the frame should be printed.  This boolean seems to only be true for
>>
>> s/only be true/only be false/
>>
>>> when the inferior has exited.  Since tui_refresh_frame_and_register_information
>>> already handles this case by checking has_stack_frames() this patch elects to
>>> ignore this parameter in the observer.
>>
>> This is OK.  I'll take a look at patch 2 soon.
>>
>> Did you find that we still need deprecated_print_frame_info_listing_hook?
>
> It still seems to be "necessary" -- at least, I can't outright remove it.
>
> The only caller of deprecated_print_frame_info_listing_hook is in
> print_frame_info and its use looks like this:
>
>  if (deprecated_print_frame_info_listing_hook)
>     (*deprecated_print_frame_info_listing_hook) (...);
>  else
>    { ... other code ... }
>
> If I remove the hook by replacing the above code with
>
>   { ... other code ... }
>
> Then a regression occurs: the TUI decides to make sure that the
> currently executing line always sits at the top of the window instead
> of only scrolling the screen when the currently executing line is not
> invisible.
>
> But if I disable the hook by replacing the body of code in print_frame_info with
>
>   if (deprecated_print_frame_info_listing_hook)
>     ;
>   else
>     { ... other code ... }
>
> Then everything seems to be OK.  So the code in  the else branch is
> interfering with TUI somehow.  I will investigate further.

The call to "print_source_lines (sal.symtab, sal.line, sal.line + 1,
0);" in the else branch eventually calls "tui_show_source (sal.line);"
which adjusts the source window so that sal.line is the very first
line visible.

I'm not sure how easy this would be to fix properly.  We want to avoid
calling print_source_lines in print_frame_info when the TUI is active.
Of course, I can just guard the code with "if (tui_active)" but that's
not a good fix.  Instead of removing the hook yet, what about making
it (tui_print_frame_info_listing_hook) a no-op in the interim?

BTW, in the CLI, this call to print_source_lines is responsible for
printing the stopped-at source line to stdout, e.g.

(gdb) start
....
Temporary breakpoint 2, main () at 13378.c:9
9         int i = 0;  // THIS LINE
(gdb)
  

On 06/30/2015 04:12 PM, Patrick Palka wrote:
> On Tue, Jun 30, 2015 at 10:54 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:

> The call to "print_source_lines (sal.symtab, sal.line, sal.line + 1,
> 0);" in the else branch eventually calls "tui_show_source (sal.line);"
> which adjusts the source window so that sal.line is the very first
> line visible.
> 
> I'm not sure how easy this would be to fix properly.  We want to avoid
> calling print_source_lines in print_frame_info when the TUI is active.
> Of course, I can just guard the code with "if (tui_active)" but that's
> not a good fix. 

Indeed.

> Instead of removing the hook yet, what about making
> it (tui_print_frame_info_listing_hook) a no-op in the interim?

If that works, fine with me.  Fine with me to leave it be as is too.

> 
> BTW, in the CLI, this call to print_source_lines is responsible for
> printing the stopped-at source line to stdout, e.g.

Yeah.  Maybe if we move the print_stop_event call out of normal_stop
into TUI/CLI normal_stop observers, then we can tailor CLI/TUI/MI to
print what they need.  I actually moved it here:

 https://github.com/palves/gdb/commits/palves/merge-more-async-and-sync
 https://github.com/palves/gdb/commit/c0a88ed037b645fb9f072290000dc11044524639

That's WIP (still ugly) post 7.10 material.

> 
> (gdb) start
> ....
> Temporary breakpoint 2, main () at 13378.c:9
> 9         int i = 0;  // THIS LINE
> (gdb)

Thanks,
Pedro Alves