[PATCH] Call target_terminal_ours() before refreshing TUI's frame info

[PATCH] Call target_terminal_ours() before refreshing TUI's frame info

[Patrick Palka]

In some cases tui_show_frame_info() may get called while the inferior's
terminal settings are still in effect.  But when we call this function
we absolutely need to have our terminal settings in effect because the
function is responsible for redrawing TUI's windows following a change
in the selected frame or a change in the PC.  If our terminal settings
are not in effect, the screen does not get redrawn properly, causing
temporary display artifacts (which can be fixed via ^L).

This scenario happens most prominently when stepping through a program
in TUI while a watchpoint is in effect.

Here is an example backtrace for when tui_show_frame_info() gets called
while target_terminal_is_inferior() == 1:

  #1  0x00000000004988ee in tui_selected_frame_level_changed_hook (level=0)
  #2  0x0000000000617b99 in select_frame (fi=0x18c9820)
  #3  0x0000000000617c3f in get_selected_frame (message=message@entry=0x0)
  #4  0x00000000004ce534 in update_watchpoint (b=b@entry=0x2d9a760,
      reparse=reparse@entry=0)
  #5  0x00000000004d625e in insert_breakpoints ()
  #6  0x0000000000531cfe in keep_going (ecs=ecs@entry=0x7ffea7884ac0)
  #7  0x00000000005326d7 in process_event_stop_test (ecs=ecs@entry=0x7ffea7884ac0)
  #8  0x000000000053596e in handle_inferior_event_1 (ecs=0x7ffea7884ac0)

The fix is simple: call target_terminal_ours() before calling
tui_show_frame_info() in TUI's frame-changed hook.

gdb/ChangeLog:

	* tui/tui-hooks.c (tui_selected_frame_level_changed_hook): Call
	target_terminal_ours() before calling tui_show_frame_info().
---
 gdb/tui/tui-hooks.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index e53f526..1eb5300 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -132,6 +132,8 @@ tui_selected_frame_level_changed_hook (int level)
   if (level < 0)
     return;
 
+  target_terminal_ours ();
+
   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
-- 
2.4.2.387.gf86f31a.dirty

Re: [PATCH] Call target_terminal_ours() before refreshing TUI's frame info

[Andrew Burgess]

* Patrick Palka <patrick@parcs.ath.cx> [2015-06-01 21:57:41 -0400]:

> Here is an example backtrace for when tui_show_frame_info() gets called
> while target_terminal_is_inferior() == 1:
> 
>   #1  0x00000000004988ee in tui_selected_frame_level_changed_hook (level=0)
>   #2  0x0000000000617b99 in select_frame (fi=0x18c9820)
>   #3  0x0000000000617c3f in get_selected_frame (message=message@entry=0x0)
>   #4  0x00000000004ce534 in update_watchpoint (b=b@entry=0x2d9a760,
>       reparse=reparse@entry=0)
>   #5  0x00000000004d625e in insert_breakpoints ()
>   #6  0x0000000000531cfe in keep_going (ecs=ecs@entry=0x7ffea7884ac0)
>   #7  0x00000000005326d7 in process_event_stop_test (ecs=ecs@entry=0x7ffea7884ac0)
>   #8  0x000000000053596e in handle_inferior_event_1 (ecs=0x7ffea7884ac0)
> 
> The fix is simple: call target_terminal_ours() before calling
> tui_show_frame_info() in TUI's frame-changed hook.

If we assume that somewhere down the call stack the terminal is
supposed to belong to the inferior, then does your patch not leave gdb
in the wrong state, where the terminal now belongs to gdb?

It feels like the terminal should probably have been claimed back by
gdb as soon as the inferior stopped, and should only be passed back to
the inferior just before it is resumed.

As an experiment, I replaced your call to target_terminal_ours with:
  gdb_assert (!target_terminal_is_inferior ());
in tui_selected_frame_level_changed_hook, this triggers frequently,
for example:
  file ~/hello.exe
  tui enable
  start

Thanks,
Andrew

> 
> gdb/ChangeLog:
> 
> 	* tui/tui-hooks.c (tui_selected_frame_level_changed_hook): Call
> 	target_terminal_ours() before calling tui_show_frame_info().
> ---
>  gdb/tui/tui-hooks.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
> index e53f526..1eb5300 100644
> --- a/gdb/tui/tui-hooks.c
> +++ b/gdb/tui/tui-hooks.c
> @@ -132,6 +132,8 @@ tui_selected_frame_level_changed_hook (int level)
>    if (level < 0)
>      return;
>  
> +  target_terminal_ours ();
> +
>    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
> -- 
> 2.4.2.387.gf86f31a.dirty
> 

Re: [PATCH] Call target_terminal_ours() before refreshing TUI's frame info

[Patrick Palka]

On Tue, Jun 2, 2015 at 10:11 AM, Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> * Patrick Palka <patrick@parcs.ath.cx> [2015-06-01 21:57:41 -0400]:
>
>> Here is an example backtrace for when tui_show_frame_info() gets called
>> while target_terminal_is_inferior() == 1:
>>
>>   #1  0x00000000004988ee in tui_selected_frame_level_changed_hook (level=0)
>>   #2  0x0000000000617b99 in select_frame (fi=0x18c9820)
>>   #3  0x0000000000617c3f in get_selected_frame (message=message@entry=0x0)
>>   #4  0x00000000004ce534 in update_watchpoint (b=b@entry=0x2d9a760,
>>       reparse=reparse@entry=0)
>>   #5  0x00000000004d625e in insert_breakpoints ()
>>   #6  0x0000000000531cfe in keep_going (ecs=ecs@entry=0x7ffea7884ac0)
>>   #7  0x00000000005326d7 in process_event_stop_test (ecs=ecs@entry=0x7ffea7884ac0)
>>   #8  0x000000000053596e in handle_inferior_event_1 (ecs=0x7ffea7884ac0)
>>
>> The fix is simple: call target_terminal_ours() before calling
>> tui_show_frame_info() in TUI's frame-changed hook.
>
> If we assume that somewhere down the call stack the terminal is
> supposed to belong to the inferior, then does your patch not leave gdb
> in the wrong state, where the terminal now belongs to gdb?

Good point -- I can confirm that this actually happens with the patch.
do_target_resume() first calls target_terminal_inferior(), and the TUI
hook gets called later in target_resume()...

I should use make_cleanup_restore_target_terminal() instead.

>
> It feels like the terminal should probably have been claimed back by
> gdb as soon as the inferior stopped, and should only be passed back to
> the inferior just before it is resumed.

That makes a lot of sense to me.  I can see no other reason for the
inferior's terminal settings to be in effect while GDB has control if
not because it is going to get resumed shortly.  But in the meantime I
will change the patch to use make_cleanup_restore_target_terminal() in
time for GDB 7.10.

>
> As an experiment, I replaced your call to target_terminal_ours with:
>   gdb_assert (!target_terminal_is_inferior ());
> in tui_selected_frame_level_changed_hook, this triggers frequently,
> for example:
>   file ~/hello.exe
>   tui enable
>   start

It seems that in a few code paths, target_terminal_ours() is called in
anticipation of writing to stdout.  Though the coverage is patchy to
say the least.

Thanks for reviewing.  Will send v2 in a bit.

>
> Thanks,
> Andrew
>
>>
>> gdb/ChangeLog:
>>
>>       * tui/tui-hooks.c (tui_selected_frame_level_changed_hook): Call
>>       target_terminal_ours() before calling tui_show_frame_info().
>> ---
>>  gdb/tui/tui-hooks.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
>> index e53f526..1eb5300 100644
>> --- a/gdb/tui/tui-hooks.c
>> +++ b/gdb/tui/tui-hooks.c
>> @@ -132,6 +132,8 @@ tui_selected_frame_level_changed_hook (int level)
>>    if (level < 0)
>>      return;
>>
>> +  target_terminal_ours ();
>> +
>>    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
>> --
>> 2.4.2.387.gf86f31a.dirty
>>

[PATCH] Call target_terminal_ours() before refreshing TUI's frame info

[Patrick Palka]

In some cases tui_show_frame_info() may get called while the inferior's
terminal settings are still in effect.  But when we call this function
we absolutely need to have our terminal settings in effect because the
function is responsible for redrawing TUI's windows following a change
in the selected frame or a change in the PC.  If our terminal settings
are not in effect, the screen does not get redrawn properly, causing
temporary display artifacts (which can be fixed via ^L).

This scenario happens most prominently when stepping through a program
in TUI while a watchpoint is in effect.

Here is an example backtrace for when tui_show_frame_info() gets called
while target_terminal_is_inferior() == 1:

  #1  0x00000000004988ee in tui_selected_frame_level_changed_hook (level=0)
  #2  0x0000000000617b99 in select_frame (fi=0x18c9820)
  #3  0x0000000000617c3f in get_selected_frame (message=message@entry=0x0)
  #4  0x00000000004ce534 in update_watchpoint (b=b@entry=0x2d9a760,
      reparse=reparse@entry=0)
  #5  0x00000000004d625e in insert_breakpoints ()
  #6  0x0000000000531cfe in keep_going (ecs=ecs@entry=0x7ffea7884ac0)
  #7  0x00000000005326d7 in process_event_stop_test (ecs=ecs@entry=0x7ffea7884ac0)
  #8  0x000000000053596e in handle_inferior_event_1 (ecs=0x7ffea7884ac0)

The fix is simple: call target_terminal_ours() before calling
tui_show_frame_info() in TUI's frame-changed hook, making sure to
restore the original terminal settings afterwards.

gdb/ChangeLog:

	* tui/tui-hooks.c (tui_selected_frame_level_changed_hook): Call
	target_terminal_ours() before calling tui_show_frame_info(), and
	restore the original terminal settings afterwards.
---
 gdb/tui/tui-hooks.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index e53f526..3331d67 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -127,11 +127,15 @@ tui_selected_frame_level_changed_hook (int level)
 {
   struct frame_info *fi;
   CORE_ADDR pc;
+  struct cleanup *old_chain;
 
   /* Negative level means that the selected frame was cleared.  */
   if (level < 0)
     return;
 
+  old_chain = make_cleanup_restore_target_terminal ();
+  target_terminal_ours ();
+
   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
@@ -160,6 +164,8 @@ tui_selected_frame_level_changed_hook (int level)
       tui_check_data_values (fi);
       tui_refreshing_registers = 0;
     }
+
+  do_cleanups (old_chain);
 }
 
 /* Called from print_frame_info to list the line we stopped in.  */
-- 
2.4.2.387.gf86f31a.dirty

Re: [PATCH] Call target_terminal_ours() before refreshing TUI's frame info

[Patrick Palka]

On Tue, Jun 2, 2015 at 11:52 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> In some cases tui_show_frame_info() may get called while the inferior's
> terminal settings are still in effect.  But when we call this function
> we absolutely need to have our terminal settings in effect because the
> function is responsible for redrawing TUI's windows following a change
> in the selected frame or a change in the PC.  If our terminal settings
> are not in effect, the screen does not get redrawn properly, causing
> temporary display artifacts (which can be fixed via ^L).
>
> This scenario happens most prominently when stepping through a program
> in TUI while a watchpoint is in effect.
>
> Here is an example backtrace for when tui_show_frame_info() gets called
> while target_terminal_is_inferior() == 1:
>
>   #1  0x00000000004988ee in tui_selected_frame_level_changed_hook (level=0)
>   #2  0x0000000000617b99 in select_frame (fi=0x18c9820)
>   #3  0x0000000000617c3f in get_selected_frame (message=message@entry=0x0)
>   #4  0x00000000004ce534 in update_watchpoint (b=b@entry=0x2d9a760,
>       reparse=reparse@entry=0)
>   #5  0x00000000004d625e in insert_breakpoints ()
>   #6  0x0000000000531cfe in keep_going (ecs=ecs@entry=0x7ffea7884ac0)
>   #7  0x00000000005326d7 in process_event_stop_test (ecs=ecs@entry=0x7ffea7884ac0)
>   #8  0x000000000053596e in handle_inferior_event_1 (ecs=0x7ffea7884ac0)
>
> The fix is simple: call target_terminal_ours() before calling
> tui_show_frame_info() in TUI's frame-changed hook, making sure to
> restore the original terminal settings afterwards.
>
> gdb/ChangeLog:
>
>         * tui/tui-hooks.c (tui_selected_frame_level_changed_hook): Call
>         target_terminal_ours() before calling tui_show_frame_info(), and
>         restore the original terminal settings afterwards.
> ---
>  gdb/tui/tui-hooks.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
> index e53f526..3331d67 100644
> --- a/gdb/tui/tui-hooks.c
> +++ b/gdb/tui/tui-hooks.c
> @@ -127,11 +127,15 @@ tui_selected_frame_level_changed_hook (int level)
>  {
>    struct frame_info *fi;
>    CORE_ADDR pc;
> +  struct cleanup *old_chain;
>
>    /* Negative level means that the selected frame was cleared.  */
>    if (level < 0)
>      return;
>
> +  old_chain = make_cleanup_restore_target_terminal ();
> +  target_terminal_ours ();
> +
>    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
> @@ -160,6 +164,8 @@ tui_selected_frame_level_changed_hook (int level)
>        tui_check_data_values (fi);
>        tui_refreshing_registers = 0;
>      }
> +
> +  do_cleanups (old_chain);
>  }
>
>  /* Called from print_frame_info to list the line we stopped in.  */
> --
> 2.4.2.387.gf86f31a.dirty
>

Ping.  Could this patch make it into 7.10?  It is pretty harmless.

Re: [PATCH] Call target_terminal_ours() before refreshing TUI's frame info

[Pedro Alves]

On 06/02/2015 04:52 PM, Patrick Palka wrote:
> In some cases tui_show_frame_info() may get called while the inferior's
> terminal settings are still in effect.  But when we call this function
> we absolutely need to have our terminal settings in effect because the
> function is responsible for redrawing TUI's windows following a change
> in the selected frame or a change in the PC.  If our terminal settings
> are not in effect, the screen does not get redrawn properly, causing
> temporary display artifacts (which can be fixed via ^L).
> 
> This scenario happens most prominently when stepping through a program
> in TUI while a watchpoint is in effect.
> 
> Here is an example backtrace for when tui_show_frame_info() gets called
> while target_terminal_is_inferior() == 1:
> 
>   #1  0x00000000004988ee in tui_selected_frame_level_changed_hook (level=0)
>   #2  0x0000000000617b99 in select_frame (fi=0x18c9820)
>   #3  0x0000000000617c3f in get_selected_frame (message=message@entry=0x0)
>   #4  0x00000000004ce534 in update_watchpoint (b=b@entry=0x2d9a760,
>       reparse=reparse@entry=0)
>   #5  0x00000000004d625e in insert_breakpoints ()
>   #6  0x0000000000531cfe in keep_going (ecs=ecs@entry=0x7ffea7884ac0)
>   #7  0x00000000005326d7 in process_event_stop_test (ecs=ecs@entry=0x7ffea7884ac0)
>   #8  0x000000000053596e in handle_inferior_event_1 (ecs=0x7ffea7884ac0)
> 
> The fix is simple: call target_terminal_ours() before calling
> tui_show_frame_info() in TUI's frame-changed hook, making sure to
> restore the original terminal settings afterwards.
> 
> gdb/ChangeLog:
> 
> 	* tui/tui-hooks.c (tui_selected_frame_level_changed_hook): Call
> 	target_terminal_ours() before calling tui_show_frame_info(), and
> 	restore the original terminal settings afterwards.


OK, though please try/use target_terminal_ours_for_output instead.

I think we should get rid of this hook entirely, and refresh
the window at some higher level point.  Refreshing the window for
each internal stop and/or selected frame save/restore results in
lots of pointless flicker...  I'm sure you've noticed.  :-)

Thanks,
Pedro Alves

Re: [PATCH] Call target_terminal_ours() before refreshing TUI's frame info

[Patrick Palka]

On Mon, Jun 15, 2015 at 11:09 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/02/2015 04:52 PM, Patrick Palka wrote:
>> In some cases tui_show_frame_info() may get called while the inferior's
>> terminal settings are still in effect.  But when we call this function
>> we absolutely need to have our terminal settings in effect because the
>> function is responsible for redrawing TUI's windows following a change
>> in the selected frame or a change in the PC.  If our terminal settings
>> are not in effect, the screen does not get redrawn properly, causing
>> temporary display artifacts (which can be fixed via ^L).
>>
>> This scenario happens most prominently when stepping through a program
>> in TUI while a watchpoint is in effect.
>>
>> Here is an example backtrace for when tui_show_frame_info() gets called
>> while target_terminal_is_inferior() == 1:
>>
>>   #1  0x00000000004988ee in tui_selected_frame_level_changed_hook (level=0)
>>   #2  0x0000000000617b99 in select_frame (fi=0x18c9820)
>>   #3  0x0000000000617c3f in get_selected_frame (message=message@entry=0x0)
>>   #4  0x00000000004ce534 in update_watchpoint (b=b@entry=0x2d9a760,
>>       reparse=reparse@entry=0)
>>   #5  0x00000000004d625e in insert_breakpoints ()
>>   #6  0x0000000000531cfe in keep_going (ecs=ecs@entry=0x7ffea7884ac0)
>>   #7  0x00000000005326d7 in process_event_stop_test (ecs=ecs@entry=0x7ffea7884ac0)
>>   #8  0x000000000053596e in handle_inferior_event_1 (ecs=0x7ffea7884ac0)
>>
>> The fix is simple: call target_terminal_ours() before calling
>> tui_show_frame_info() in TUI's frame-changed hook, making sure to
>> restore the original terminal settings afterwards.
>>
>> gdb/ChangeLog:
>>
>>       * tui/tui-hooks.c (tui_selected_frame_level_changed_hook): Call
>>       target_terminal_ours() before calling tui_show_frame_info(), and
>>       restore the original terminal settings afterwards.
>
>
> OK, though please try/use target_terminal_ours_for_output instead.

Okay.

>
> I think we should get rid of this hook entirely, and refresh
> the window at some higher level point.  Refreshing the window for
> each internal stop and/or selected frame save/restore results in
> lots of pointless flicker...  I'm sure you've noticed.  :-)

Good idea.

>
> Thanks,
> Pedro Alves
>