From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x329.google.com (mail-wm1-x329.google.com [IPv6:2a00:1450:4864:20::329]) by sourceware.org (Postfix) with ESMTPS id AACFF3836C4D for ; Tue, 26 Jan 2021 09:29:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org AACFF3836C4D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wm1-x329.google.com with SMTP id u14so1836250wml.4 for ; Tue, 26 Jan 2021 01:29:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=N6EsN2a2o5W6nvieYjjuQTjQhTSSM6kDpjyKJ6J76Uc=; b=Y8nvpLUkDO012O9QQMw/73sx8S2kX+QeqaIygrVkN4xX0XOR8Ze1HgMP2WEkBrmCzO 3ExoA+NYNt0zftMjczqi5c0cybwN+6cdXC6hiJ/bsw7fAlZ8TNd3fY8GJ7DPW6auDWxJ 2K/cDsm2y0S36N7DvCklw53bjEGR7YgUwn+WofU7NGKt0cRyYp/NGYSlmkAn5boWk1x3 oKsBilHh9QWuM1O4W90VMXDFmFvD4D6kV9th9gx4bOhvlKmT7vcRr7ETuXoyB7vOtBgH u2mxuJirW1AYozKzDysDmDiSBLrtGTbwBdZdf7A//DGWtZWV251vQp1ToetTaudsAWqi HGrw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=N6EsN2a2o5W6nvieYjjuQTjQhTSSM6kDpjyKJ6J76Uc=; b=JOlMBtenLGVXkm/ikSvXZ+2JDrZ3XHNQURSP+LAEiYW+iHjzFWJKPRNBwkE7u4mEtX Hn6f5IZDPFyVYFcJs0eIPVloG2rGyiS9+QBwFwvrdyi8suAILv4ppzhJnkOtm9Vy+Q0a iU1pO+6skhW/yxE+PkHFU47NYUecOWfvCekWYsu4cfEuoeZK5qR7Gp/b3J7bd62svOpj gpfL2hAQOn8w3zSzai6ZU/YSsQUlvvhNm2Gs1lR6SLHkcstPEFocnSwjkISuv+VoCkP2 lwBAwQEO8NvcgY6r4zc9JIf4qOR+mEgmUic1kzou3FcF/DwttoJtP0MnGkUmTYVYeHiG 3WXQ== X-Gm-Message-State: AOAM530Svb1AosYvciOp3tOoftThvx7udDd/L4HFd6vaKA2xpGws311v xQM8+fIVyJ/2ASs5YNLp9mlodA== X-Google-Smtp-Source: ABdhPJymQ/KphXkXNtFaIPLeuMGBEvJwHlFTDekDlfDwwlb+JtcU//afVRzrc+rUaxAtGp8Zjb5VEg== X-Received: by 2002:a1c:e905:: with SMTP id q5mr3850465wmc.84.1611653374773; Tue, 26 Jan 2021 01:29:34 -0800 (PST) Received: from localhost (host86-166-129-180.range86-166.btcentralplus.com. [86.166.129.180]) by smtp.gmail.com with ESMTPSA id g132sm2585390wmg.2.2021.01.26.01.29.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Jan 2021 01:29:33 -0800 (PST) Date: Tue, 26 Jan 2021 09:29:33 +0000 From: Andrew Burgess To: Hannes Domani Cc: "gdb-patches@sourceware.org" , Tom Tromey Subject: Re: [PATCH v2] Don't add window duplicates to tui_windows Message-ID: <20210126092933.GJ265215@embecosm.com> References: <20210105135549.5704-1-ssbssa.ref@yahoo.de> <20210105135549.5704-1-ssbssa@yahoo.de> <20210125161841.GF265215@embecosm.com> <1272741195.4862754.1611595895785@mail.yahoo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: <1272741195.4862754.1611595895785@mail.yahoo.com> X-Operating-System: Linux/5.8.13-100.fc31.x86_64 (x86_64) X-Uptime: 09:29:01 up 48 days, 14:13, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-10.6 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 Jan 2021 09:29:38 -0000 * Hannes Domani [2021-01-25 17:31:35 +0000]: > Am Montag, 25. Januar 2021, 17:19:16 MEZ hat Andrew Burgess Folgendes geschrieben: >=20 > > * Hannes Domani via Gdb-patches [2021-01-0= 5 14:55:49 +0100]: > > > > > Otherwise, each time a window size is changed with 'winheight', all w= indows > > > are duplicated, and when done multiple times, slows down redrawing. > > > > > > gdb/ChangeLog: > > > > > > 2021-01-05=A0 Hannes Domani=A0 > > > > > >=A0=A0=A0=A0 * tui/tui-layout.c (tui_apply_current_layout): Add add_wi= ndow > > >=A0=A0=A0=A0 argument to apply function. > > >=A0=A0=A0=A0 (tui_layout_window::apply): Likewise. > > >=A0=A0=A0=A0 (tui_layout_split::adjust_size): Likewise. > > >=A0=A0=A0=A0 (tui_layout_split::apply): Likewise. > > >=A0=A0=A0=A0 * tui/tui-layout.h (class tui_layout_base): Likewise. > > >=A0=A0=A0=A0 (class tui_layout_window): Likewise. > > >=A0=A0=A0=A0 (class tui_layout_split): Likewise. > > > --- > > > v2: > > > - Use new add_window argument to decide if window should be added to > > >=A0 tui_windows. > > > --- > > >=A0 gdb/tui/tui-layout.c | 19 ++++++++++++------- > > >=A0 gdb/tui/tui-layout.h |=A0 7 ++++--- > > >=A0 2 files changed, 16 insertions(+), 10 deletions(-) > > > > > > diff --git a/gdb/tui/tui-layout.c b/gdb/tui/tui-layout.c > > > index d6e299b00f1..fc1b65d8799 100644 > > > --- a/gdb/tui/tui-layout.c > > > +++ b/gdb/tui/tui-layout.c > > > @@ -85,7 +85,7 @@ tui_apply_current_layout () > > >=A0=A0=A0 for (tui_win_info *win_info : saved_tui_windows) > > >=A0=A0=A0=A0=A0 win_info->make_visible (false); > > > > > > -=A0 applied_layout->apply (0, 0, tui_term_width (), tui_term_height = ()); > > > +=A0 applied_layout->apply (0, 0, tui_term_width (), tui_term_height = (), true); > > > > > >=A0=A0=A0 /* Keep the list of internal windows up-to-date.=A0 */ > > >=A0=A0=A0 for (int win_type =3D SRC_WIN; (win_type < MAX_MAJOR_WINDOWS= ); win_type++) > > > @@ -416,7 +416,8 @@ tui_layout_window::clone () const > > >=A0 /* See tui-layout.h.=A0 */ > > > > > >=A0 void > > > -tui_layout_window::apply (int x_, int y_, int width_, int height_) > > > +tui_layout_window::apply (int x_, int y_, int width_, int height_, > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 bool add_window) > > >=A0 { > > >=A0=A0=A0 x =3D x_; > > >=A0=A0=A0 y =3D y_; > > > @@ -424,7 +425,8 @@ tui_layout_window::apply (int x_, int y_, int wid= th_, int height_) > > >=A0=A0=A0 height =3D height_; > > >=A0=A0=A0 gdb_assert (m_window !=3D nullptr); > > >=A0=A0=A0 m_window->resize (height, width, x, y); > > > -=A0 tui_windows.push_back (m_window); > > > +=A0 if (add_window) > > > +=A0=A0=A0 tui_windows.push_back (m_window); > > >=A0 } > > > > I wondered if there was a solution to this problem that would remove > > the need to update some global state from the ::apply function. > > > > I came up with the patch below.=A0 What do you think? >=20 > I think this is fine, I only have one question below. >=20 >=20 > > This also includes a test that covers this issue, when I run the test > > under valgrind it highlights that GDB tries to free the same object > > multiple times. > > > > Thanks, > > Andrew > > > > > > --- > > > > commit 3d47fcb70e234d08ab2e47c1527bca081846b8ee > > Author: Andrew Burgess > > Date:=A0 Mon Jan 25 15:46:58 2021 +0000 > > > >=A0=A0=A0=A0 gdb/tui: don't add windows to global list from tui_layout:w= indow::apply > >=A0=A0 =A0 > >=A0=A0=A0=A0 This commit was inspired by this mailing list patch: > >=A0=A0 =A0 > >=A0=A0=A0=A0=A0=A0 https://sourceware.org/pipermail/gdb-patches/2021-Jan= uary/174713.html > >=A0=A0 =A0 > >=A0=A0=A0=A0 Currently, calling tui_layout_window::apply will add the wi= ndow from > >=A0=A0=A0=A0 the layout object to the global tui_windows list. > >=A0=A0 =A0 > >=A0=A0=A0=A0 Unfortunately, when the user runs the 'winheight' command, = this calls > >=A0=A0=A0=A0 tui_adjust_window_height, which calls the tui_layout_base::= adjust_size > >=A0=A0=A0=A0 function, which can then call tui_layout_base::apply.=A0 Th= e consequence > >=A0=A0=A0=A0 of this is that when the user does 'winheight' duplicate co= pies of a > >=A0=A0=A0=A0 window can be added to the global tui_windows list. > >=A0=A0 =A0 > >=A0=A0=A0=A0 The original patch fixed this by changing the apply functio= n to only > >=A0=A0=A0=A0 update the global list some of the time. > >=A0=A0 =A0 > >=A0=A0=A0=A0 This patch takes a different approach.=A0 The apply functio= n no longer > >=A0=A0=A0=A0 updates the global tui_windows list.=A0 Instead a new virtu= al function > >=A0=A0=A0=A0 is added to tui_layout_base which is used to gather all the= currently > >=A0=A0=A0=A0 applied windows into a vector.=A0 Finally tui_apply_current= _layout is > >=A0=A0=A0=A0 updated to make use of this new function to update the tui_= windows > >=A0=A0=A0=A0 list. > >=A0=A0 =A0 > >=A0=A0=A0=A0 The benefits I see in this approach are, (a) the apply func= tion now no > >=A0=A0=A0=A0 longer touches global state, this solves the immediate prob= lem, > >=A0=A0=A0=A0 and (b) now that tui_windows is updated directly in the fun= ction > >=A0=A0=A0=A0 tui_apply_current_layout, we can drop the saved_tui_windows= global. > >=A0=A0 =A0 > >=A0=A0=A0=A0 gdb/ChangeLog: > >=A0=A0 =A0 > >=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 * tui-layout.c (saved_tui_windows):= Delete. > >=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 (tui_apply_current_layout): Don't m= ake use of saved_tui_windows, > >=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 call new get_windows member functio= n instead. > >=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 (tui_get_window_by_name): Check in = tui_windows. > >=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 (tui_layout_window::apply): Don't a= dd to tui_windows. > >=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 * tui-layout.h (tui_layout_base::ge= t_windows): New member function. > >=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 (tui_layout_window::get_windows): L= ikewise. > >=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 (tui_layout_split::get_windows): Li= kewise. > >=A0=A0 =A0 > >=A0=A0=A0=A0 gdb/testsuite/ChangeLog: > >=A0=A0 =A0 > >=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 * gdb.tui/winheight.exp: Add more t= ests. > > > > diff --git a/gdb/testsuite/gdb.tui/winheight.exp b/gdb/testsuite/gdb.tu= i/winheight.exp > > index 38fb29c3829..04de35d2f45 100644 > > --- a/gdb/testsuite/gdb.tui/winheight.exp > > +++ b/gdb/testsuite/gdb.tui/winheight.exp > > @@ -36,3 +36,17 @@ Term::check_box "smaller source box" 0 0 80 10 > > > > Term::command "winheight cmd -5" > > Term::check_box "larger source box" 0 0 80 15 > > + > > +Term::command "winheight src -5" > > +Term::check_box "smaller source box again" 0 0 80 10 > > + > > +Term::command "winheight src +5" > > +Term::check_box "larger source box again" 0 0 80 15 > > + > > +# At one point we had a bug where adjusting the winheight would result > > +# in GDB keeping hold of duplicate window pointers, which it might > > +# then try to delete when the layout was changed.=A0 Running this test > > +# under valgrind would expose that bug. > > +Term::command "layout asm" > > +Term::check_box "check for asm window" 0 0 80 15 > > + > > diff --git a/gdb/tui/tui-layout.c b/gdb/tui/tui-layout.c > > index d6e299b00f1..adb99852da2 100644 > > --- a/gdb/tui/tui-layout.c > > +++ b/gdb/tui/tui-layout.c > > @@ -64,11 +64,6 @@ static tui_layout_split *asm_regs_layout; > > /* See tui-data.h.=A0 */ > > std::vector tui_windows; > > > > -/* When applying a layout, this is the list of all windows that were > > -=A0 in the previous layout.=A0 This is used to re-use windows when > > -=A0 changing a layout.=A0 */ > > -static std::vector saved_tui_windows; > > - > > /* See tui-layout.h.=A0 */ > > > > void > > @@ -79,10 +74,7 @@ tui_apply_current_layout () > > > >=A0=A0 extract_display_start_addr (&gdbarch, &addr); > > > > -=A0 saved_tui_windows =3D std::move (tui_windows); > > -=A0 tui_windows.clear (); > > - > > -=A0 for (tui_win_info *win_info : saved_tui_windows) > > +=A0 for (tui_win_info *win_info : tui_windows) > >=A0=A0=A0=A0 win_info->make_visible (false); > > > >=A0=A0 applied_layout->apply (0, 0, tui_term_width (), tui_term_height (= )); > > @@ -96,25 +88,33 @@ tui_apply_current_layout () > >=A0=A0 /* This should always be made visible by a layout.=A0 */ > >=A0=A0 gdb_assert (TUI_CMD_WIN->is_visible ()); > > > > +=A0 /* Get the new list of currently visible windows.=A0 */ > > +=A0 std::vector new_tui_windows; > > +=A0 applied_layout->get_windows (&new_tui_windows); > > + > >=A0=A0 /* Now delete any window that was not re-applied.=A0 */ > >=A0=A0 tui_win_info *focus =3D tui_win_with_focus (); > >=A0=A0 tui_win_info *locator =3D tui_locator_win_info_ptr (); > > -=A0 for (tui_win_info *win_info : saved_tui_windows) > > +=A0 for (tui_win_info *win_info : tui_windows) > >=A0=A0=A0=A0 { > >=A0=A0=A0=A0=A0=A0 if (!win_info->is_visible ()) > >=A0=A0=A0=A0 { > > +=A0=A0=A0=A0=A0 /* If we're about to delete the window with focus then= don't. > > +=A0=A0=A0=A0=A0=A0=A0 This will be deleted later once we have selected= an > > +=A0=A0=A0=A0=A0=A0=A0 alternative window to focus on.=A0 */ > >=A0=A0=A0=A0=A0=A0 if (focus =3D=3D win_info) > > -=A0=A0=A0=A0=A0=A0=A0 tui_set_win_focus_to (tui_windows[0]); > > +=A0=A0=A0=A0=A0=A0=A0 tui_set_win_focus_to (new_tui_windows[0]); > >=A0=A0=A0=A0=A0=A0 if (win_info !=3D locator) > >=A0=A0=A0=A0=A0=A0=A0=A0 delete win_info; > >=A0=A0=A0=A0 } > >=A0=A0=A0=A0 } > > > > +=A0 tui_windows.clear (); >=20 > Isn't this clear() redundant? Good catch, I'll drop that from the change. Thanks, Andrew >=20 >=20 > > +=A0 tui_windows =3D std::move (new_tui_windows); > > + > >=A0=A0 if (gdbarch =3D=3D nullptr && TUI_DISASM_WIN !=3D nullptr) > >=A0=A0=A0=A0 tui_get_begin_asm_address (&gdbarch, &addr); > >=A0=A0 tui_update_source_windows_with_addr (gdbarch, addr); > > - > > -=A0 saved_tui_windows.clear (); > > } > > > > /* See tui-layout.=A0 */ > > @@ -354,7 +354,7 @@ static std::unordered_map *known_window_types; > > static tui_win_info * > > tui_get_window_by_name (const std::string &name) > > { > > -=A0 for (tui_win_info *window : saved_tui_windows) > > +=A0 for (tui_win_info *window : tui_windows) > >=A0=A0=A0=A0 if (name =3D=3D window->name ()) > >=A0=A0=A0=A0=A0=A0 return window; > > > > @@ -424,7 +424,6 @@ tui_layout_window::apply (int x_, int y_, int width= _, int height_) > >=A0=A0 height =3D height_; > >=A0=A0 gdb_assert (m_window !=3D nullptr); > >=A0=A0 m_window->resize (height, width, x, y); > > -=A0 tui_windows.push_back (m_window); > > } > > > > /* See tui-layout.h.=A0 */ > > diff --git a/gdb/tui/tui-layout.h b/gdb/tui/tui-layout.h > > index 193f42de420..f89166eae37 100644 > > --- a/gdb/tui/tui-layout.h > > +++ b/gdb/tui/tui-layout.h > > @@ -91,6 +91,9 @@ class tui_layout_base > >=A0=A0=A0=A0=A0=A0 depth of this layout in the hierarchy (zero-based).= =A0 */ > >=A0=A0 virtual void specification (ui_file *output, int depth) =3D 0; > > > > +=A0 /* Add all windows to the WINDOWS vector.=A0 */ > > +=A0 virtual void get_windows (std::vector *windows) = =3D 0; > > + > >=A0=A0 /* The most recent space allocation.=A0 */ > >=A0=A0 int x =3D 0; > >=A0=A0 int y =3D 0; > > @@ -141,6 +144,12 @@ class tui_layout_window : public tui_layout_base > > > >=A0=A0 void specification (ui_file *output, int depth) override; > > > > +=A0 /* See tui_layout_base::get_windows.=A0 */ > > +=A0 void get_windows (std::vector *windows) override > > +=A0 { > > +=A0=A0=A0 windows->push_back (m_window); > > +=A0 } > > + > > protected: > > > >=A0=A0 void get_sizes (bool height, int *min_value, int *max_value) over= ride; > > @@ -195,6 +204,13 @@ class tui_layout_split : public tui_layout_base > > > >=A0=A0 void specification (ui_file *output, int depth) override; > > > > +=A0 /* See tui_layout_base::get_windows.=A0 */ > > +=A0 void get_windows (std::vector *windows) override > > +=A0 { > > +=A0=A0=A0 for (auto &item : m_splits) > > +=A0=A0=A0=A0=A0 item.layout->get_windows (windows); > > +=A0 } > > + > > protected: > > > >=A0=A0 void get_sizes (bool height, int *min_value, int *max_value) over= ride;