PR tui/21599: GDB crashes if TUI terminal window is made too small

  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).

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.

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(-)
  

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.

> 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.

 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.

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.

>        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.

>  	  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 :)

Simon
  

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,