Re: [PATCH] Explicitly call rl_resize_terminal() in TUI's SIGWINCH handler

[Patrick Palka <patrick@parcs.ath.cx>]

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
>