Explicitly call rl_resize_terminal() in TUI's SIGWINCH handler

  (I am looking into syncing our copy of readline to the latest version,
6.3.)

In readline 6.3, the semantics of SIGWINCH handling has changed.
When a SIGWINCH signal is raised, readline's rl_sigwinch_handler() now
does not immediately call rl_resize_terminal().  Instead it sets a flag
that is checked by RL_CHECK_SIGNALS() at a point where readline has
control, and calls rl_resize_terminal() if said flag is set.

This change is item (c) in https://cnswww.cns.cwru.edu/php/chet/readline/CHANGES

  c.  Fixed a bug that caused readline to try and run code to modify its idea
      of the screen size in a signal handler context upon receiving a SIGWINCH.

This change in behavior is important to us because TUI's
tui_sigwinch_handler() relies on the assumption that by the time it's
called, readline will have updated its knowledge of the terminal
dimensions via rl_resize_terminal().  Since this assumption no longer
holds true, TUI's SIGWINCH handling does not work correctly with
readline 6.3.

To fix this issue this patch makes TUI explicitly call
rl_resize_terminal() in tui_async_resize_screen() at the point where
current terminal dimensions are needed.  (We could call it in
tui_sigwinch_handler too, but since readline avoids doing it, we are
probably safer off avoiding to call it in signal handler context as
well.)  After this change, SIGWINCH handling continues to work properly
with both readline 6.2 and 6.3.

Since we no longer need it, we could now explicitly disable readline's
SIGWINCH handler by setting rl_handle_sigwinch to zero early on in the
program startup but I can't seem to find a good spot to place this
assignment (the first call to rl_initialize() occurs in
tui_initialize_readline() so the assignment should occur before then),
and the handler is harmless anyway.
---
 gdb/tui/tui-win.c | 1 +
 1 file changed, 1 insertion(+)
  

On 04/23/2015 12:49 AM, Patrick Palka wrote:
> (I am looking into syncing our copy of readline to the latest version,
> 6.3.)
> 
> In readline 6.3, the semantics of SIGWINCH handling has changed.
> When a SIGWINCH signal is raised, readline's rl_sigwinch_handler() now
> does not immediately call rl_resize_terminal().  Instead it sets a flag
> that is checked by RL_CHECK_SIGNALS() at a point where readline has
> control, and calls rl_resize_terminal() if said flag is set.
> 
> This change is item (c) in https://cnswww.cns.cwru.edu/php/chet/readline/CHANGES
> 
>   c.  Fixed a bug that caused readline to try and run code to modify its idea
>       of the screen size in a signal handler context upon receiving a SIGWINCH.
> 
> This change in behavior is important to us because TUI's
> tui_sigwinch_handler() relies on the assumption that by the time it's
> called, readline will have updated its knowledge of the terminal
> dimensions via rl_resize_terminal().  Since this assumption no longer
> holds true, TUI's SIGWINCH handling does not work correctly with
> readline 6.3.
> 
> To fix this issue this patch makes TUI explicitly call
> rl_resize_terminal() in tui_async_resize_screen() at the point where
> current terminal dimensions are needed.  (We could call it in
> tui_sigwinch_handler too, but since readline avoids doing it, we are
> probably safer off avoiding to call it in signal handler context as
> well.)  After this change, SIGWINCH handling continues to work properly
> with both readline 6.2 and 6.3.

OK with ChangeLog entry.  I wonder if this fixes PR18155?

I think we should call this and update gdb's sense of the screen
size even with the tui disabled though.  I currently see, outside
the TUI:

(gdb) show height
Number of lines gdb thinks are in a page is 45.

*resize*

(gdb) show height
Number of lines gdb thinks are in a page is 45.

* switch TUI on and off *

(gdb) show height
Number of lines gdb thinks are in a page is 27.

> 
> Since we no longer need it, we could now explicitly disable readline's
> SIGWINCH handler by setting rl_handle_sigwinch to zero early on in the
> program startup but I can't seem to find a good spot to place this
> assignment (the first call to rl_initialize() occurs in
> tui_initialize_readline() so the assignment should occur before then),
> and the handler is harmless anyway.

ITYM rl_catch_sigwinch:

/* If non-zero, readline will install a signal handler for SIGWINCH
   that also attempts to call any calling application's SIGWINCH signal
   handler.  Note that the terminal is not cleaned up before the
   application's signal handler is called; use rl_cleanup_after_signal()
   to do that. */
extern int rl_catch_sigwinch;

Maybe in init_page_info, and probably move the init_page_info call
to gdb_init directly for clarity.  (I found that with "rbreak rl_*".)

Thanks,
Pedro Alves
  

On Thu, Apr 23, 2015 at 7:07 AM, Pedro Alves <palves@redhat.com> wrote:
> On 04/23/2015 12:49 AM, Patrick Palka wrote:
>> (I am looking into syncing our copy of readline to the latest version,
>> 6.3.)
>>
>> In readline 6.3, the semantics of SIGWINCH handling has changed.
>> When a SIGWINCH signal is raised, readline's rl_sigwinch_handler() now
>> does not immediately call rl_resize_terminal().  Instead it sets a flag
>> that is checked by RL_CHECK_SIGNALS() at a point where readline has
>> control, and calls rl_resize_terminal() if said flag is set.
>>
>> This change is item (c) in https://cnswww.cns.cwru.edu/php/chet/readline/CHANGES
>>
>>   c.  Fixed a bug that caused readline to try and run code to modify its idea
>>       of the screen size in a signal handler context upon receiving a SIGWINCH.
>>
>> This change in behavior is important to us because TUI's
>> tui_sigwinch_handler() relies on the assumption that by the time it's
>> called, readline will have updated its knowledge of the terminal
>> dimensions via rl_resize_terminal().  Since this assumption no longer
>> holds true, TUI's SIGWINCH handling does not work correctly with
>> readline 6.3.
>>
>> To fix this issue this patch makes TUI explicitly call
>> rl_resize_terminal() in tui_async_resize_screen() at the point where
>> current terminal dimensions are needed.  (We could call it in
>> tui_sigwinch_handler too, but since readline avoids doing it, we are
>> probably safer off avoiding to call it in signal handler context as
>> well.)  After this change, SIGWINCH handling continues to work properly
>> with both readline 6.2 and 6.3.
>
> OK with ChangeLog entry.  I wonder if this fixes PR18155?

Nope, it doesn't.  Strange that resizing in "layout src" or "layout
split" works but not in "layout asm".  I'll take a look at this.

>
> I think we should call this and update gdb's sense of the screen
> size even with the tui disabled though.  I currently see, outside
> the TUI:
>
> (gdb) show height
> Number of lines gdb thinks are in a page is 45.
>
> *resize*
>
> (gdb) show height
> Number of lines gdb thinks are in a page is 45.
>
> * switch TUI on and off *
>
> (gdb) show height
> Number of lines gdb thinks are in a page is 27.

Ah yeah.  That will be easy I think.  I'll take a look at this too.

>
>>
>> Since we no longer need it, we could now explicitly disable readline's
>> SIGWINCH handler by setting rl_handle_sigwinch to zero early on in the
>> program startup but I can't seem to find a good spot to place this
>> assignment (the first call to rl_initialize() occurs in
>> tui_initialize_readline() so the assignment should occur before then),
>> and the handler is harmless anyway.
>
> ITYM rl_catch_sigwinch:
>
> /* If non-zero, readline will install a signal handler for SIGWINCH
>    that also attempts to call any calling application's SIGWINCH signal
>    handler.  Note that the terminal is not cleaned up before the
>    application's signal handler is called; use rl_cleanup_after_signal()
>    to do that. */
> extern int rl_catch_sigwinch;
>
> Maybe in init_page_info, and probably move the init_page_info call
> to gdb_init directly for clarity.  (I found that with "rbreak rl_*".)

Good idea, I'll do this too.

>
> Thanks,
> Pedro Alves
>