[PATCH] PR tui/21599: GDB crashes if TUI terminal window is made too small
Sergio Durigan Junior
sergiodj@redhat.com
Thu Jun 15 23:56:00 GMT 2017
On Thursday, June 15 2017, Simon Marchi wrote:
> On 2017-06-14 23:19, Sergio Durigan Junior wrote:
>> This problem happens mostly when using TUI mode inside a tmux pane
>> which is split horizontally. To reproduce:
>>
>> 1) Enter tmux. I am assuming that the modifier sequence for your tmux
>> is C-b (the default).
>>
>> 2) Split the pane horizontally (C-b ").
>>
>> 3) Start GDB in TUI mode (gdb -tui) on the upper pane.
>>
>> 4) Resize the upper pane, making it as small as possible (C-b
>> <upper-arrow>, repeatedly).
>
> For those that use a desktop environment, it's also easy to reproduce
> by reducing the height of the window to 3 rows or less.
Right, thanks for pointing that out.
>> The problem happens because tmux's pane has a screen height that makes
>> GDB miscalculate the minimum screen height that can be set by the
>> terminal. The solution I found was to first check if this minimum
>> height is actually negative, and avoid using it if so. In this case,
>> TUI can just use the MIN_WIN_HEIGHT define and be done with the
>> resizing process.
>
> Hmm I still get a crash with your patch by resizing to <= 3 rows.
Oh? Maybe I need to test the patch by actually resizing the terminal
window to <= 3 rows, instead of using tmux.
> From what I understand, the problem is that the computed height
> (new_height) is negative. We have 3 rows total, and we want to
> allocate at least 4 (MIN_CMD_WIN_HEIGHT + 1) to the command window and
> the separator. That leaves us with -1 rows for the source window,
> which makes no sense. Instead, shouldn't we clamp new_height after
> computing it? See suggestion below.
>
> Btw, I don't really understand the point of having a MIN_WIN_HEIGHT
> (which seems to apply to the source window, for example) and a
> MIN_CMD_WIN_HEIGHT. If you are at the point where you hit the
> minimum, we'll never be able to honour both minimums, on window or the
> other will have to shrink below its minimum.
I confess I don't really understand the point either. This code is kind
of messy and I didn't take a long time looking at it.
> Also, there seems to be the same problem for the 2 windows + command
> window layouts.
>
>> gdb/ChangeLog:
>> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> PR tui/21599
>> * tui-win.c (tui_resize_all): New variable 'min_screenheight'.
>> Use it to avoid resizing the TUI window to an invalid value.
>> ---
>> gdb/tui/tui-win.c | 13 +++++++++++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
>> index f49d7d5..ec594bf 100644
>> --- a/gdb/tui/tui-win.c
>> +++ b/gdb/tui/tui-win.c
>> @@ -753,6 +753,7 @@ tui_resize_all (void)
>> {
>> int height_diff, width_diff;
>> int screenheight, screenwidth;
>> + int min_screenheight;
>>
>> rl_get_screen_size (&screenheight, &screenwidth);
>> width_diff = screenwidth - tui_term_width ();
>> @@ -795,6 +796,7 @@ tui_resize_all (void)
>> erase ();
>> clearok (curscr, TRUE);
>> refresh ();
>> + min_screenheight = screenheight - MIN_CMD_WIN_HEIGHT - 1;
>
> This computation sounds more like a "max_screenheight". It's the
> maximum height our source window can have.
Hm, right. I'll rename the variable accordingly.
>> switch (cur_layout)
>> {
>> case SRC_COMMAND:
>> @@ -805,9 +807,16 @@ tui_resize_all (void)
>> /* Check for invalid heights. */
>> if (height_diff == 0)
>> new_height = first_win->generic.height;
>> + else if (min_screenheight < 0)
>> + {
>> + /* In some cases min_screenheight can be negative.
>> + E.g., when using tmux and resizing the screen to the
>> + minimum allowed. See PR tui/21599. */
>> + new_height = MIN_WIN_HEIGHT;
>> + }
>> else if ((first_win->generic.height + split_diff) >=
>> - (screenheight - MIN_CMD_WIN_HEIGHT - 1))
>> - new_height = screenheight - MIN_CMD_WIN_HEIGHT - 1;
>> + min_screenheight)
>> + new_height = min_screenheight;
>
> ... and here again it sounds like max: if the new screen height is
> greater than the max, you clamp it at the max.
OK.
>
>> else if ((first_win->generic.height + split_diff) <= 0)
>> new_height = MIN_WIN_HEIGHT;
>> else
>
> Instead of an if/else chain, I think this code would be simpler if it
> just computed the new_height unconditionally:
>
> new_height = first_win->generic.height + split_diff;
>
> and then clamped the result to the acceptable range:
>
> int max_win_height = screenheight - MIN_CMD_WIN_HEIGHT - 1;
> new_height = gdb::clamp (new_height, MIN_WIN_HEIGHT, max_win_height);
>
> gdb::clamp would be a backport of C++17's function.
>
> My next question would be: why bother with diffs and not recompute all
> the sizes from scratch from the new absolute terminal size. It can't
> be that long. Maybe there's a good reason, but I did not get that far
> :)
Please disconsider this patch, then. I will see if I understand this
code better to propose a more reasonable approach.
Thanks,
--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
More information about the Gdb-patches
mailing list