[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