From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22238 invoked by alias); 15 Jun 2017 22:21:39 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 22212 invoked by uid 89); 15 Jun 2017 22:21:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy=desktop, honour X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 15 Jun 2017 22:21:35 +0000 Received: by simark.ca (Postfix, from userid 33) id 0ADB71E5A4; Thu, 15 Jun 2017 18:21:38 -0400 (EDT) To: Sergio Durigan Junior Subject: Re: [PATCH] PR tui/21599: GDB crashes if TUI terminal window is made too small X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 15 Jun 2017 22:21:00 -0000 From: Simon Marchi Cc: GDB Patches In-Reply-To: <20170614211912.30843-1-sergiodj@redhat.com> References: <20170614211912.30843-1-sergiodj@redhat.com> Message-ID: <72174fdc7a941330e7e685f2cb907e1c@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.5 X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg00453.txt.bz2 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 > , 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 > > 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