Re: [PATCH] PR tui/21599: GDB crashes if TUI terminal window is made too small

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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