From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 57235 invoked by alias); 15 Jun 2017 23:56:20 -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 57224 invoked by uid 89); 15 Jun 2017 23:56:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 15 Jun 2017 23:56:18 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B0FC285376; Thu, 15 Jun 2017 23:56:21 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B0FC285376 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=sergiodj@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com B0FC285376 Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 84F1281B44; Thu, 15 Jun 2017 23:56:21 +0000 (UTC) From: Sergio Durigan Junior To: Simon Marchi Cc: GDB Patches Subject: Re: [PATCH] PR tui/21599: GDB crashes if TUI terminal window is made too small References: <20170614211912.30843-1-sergiodj@redhat.com> <72174fdc7a941330e7e685f2cb907e1c@polymtl.ca> Date: Thu, 15 Jun 2017 23:56:00 -0000 In-Reply-To: <72174fdc7a941330e7e685f2cb907e1c@polymtl.ca> (Simon Marchi's message of "Fri, 16 Jun 2017 00:21:38 +0200") Message-ID: <87injw6c0r.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg00455.txt.bz2 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 >> , 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 >> >> 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/