[PATCH v2] Don't add window duplicates to tui_windows

Andrew Burgess andrew.burgess@embecosm.com
Mon Jan 25 16:18:41 GMT 2021


* Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> [2021-01-05 14:55:49 +0100]:

> Otherwise, each time a window size is changed with 'winheight', all windows
> are duplicated, and when done multiple times, slows down redrawing.
> 
> gdb/ChangeLog:
> 
> 2021-01-05  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* tui/tui-layout.c (tui_apply_current_layout): Add add_window
> 	argument to apply function.
> 	(tui_layout_window::apply): Likewise.
> 	(tui_layout_split::adjust_size): Likewise.
> 	(tui_layout_split::apply): Likewise.
> 	* tui/tui-layout.h (class tui_layout_base): Likewise.
> 	(class tui_layout_window): Likewise.
> 	(class tui_layout_split): Likewise.
> ---
> v2:
> - Use new add_window argument to decide if window should be added to
>   tui_windows.
> ---
>  gdb/tui/tui-layout.c | 19 ++++++++++++-------
>  gdb/tui/tui-layout.h |  7 ++++---
>  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 ()
>    for (tui_win_info *win_info : saved_tui_windows)
>      win_info->make_visible (false);
>  
> -  applied_layout->apply (0, 0, tui_term_width (), tui_term_height ());
> +  applied_layout->apply (0, 0, tui_term_width (), tui_term_height (), true);
>  
>    /* Keep the list of internal windows up-to-date.  */
>    for (int win_type = SRC_WIN; (win_type < MAX_MAJOR_WINDOWS); win_type++)
> @@ -416,7 +416,8 @@ tui_layout_window::clone () const
>  /* See tui-layout.h.  */
>  
>  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_,
> +			  bool add_window)
>  {
>    x = x_;
>    y = y_;
> @@ -424,7 +425,8 @@ tui_layout_window::apply (int x_, int y_, int width_, int height_)
>    height = height_;
>    gdb_assert (m_window != nullptr);
>    m_window->resize (height, width, x, y);
> -  tui_windows.push_back (m_window);
> +  if (add_window)
> +    tui_windows.push_back (m_window);
>  }

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.  What do you think?

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 <andrew.burgess@embecosm.com>
Date:   Mon Jan 25 15:46:58 2021 +0000

    gdb/tui: don't add windows to global list from tui_layout:window::apply
    
    This commit was inspired by this mailing list patch:
    
      https://sourceware.org/pipermail/gdb-patches/2021-January/174713.html
    
    Currently, calling tui_layout_window::apply will add the window from
    the layout object to the global tui_windows list.
    
    Unfortunately, when the user runs the 'winheight' command, this calls
    tui_adjust_window_height, which calls the tui_layout_base::adjust_size
    function, which can then call tui_layout_base::apply.  The consequence
    of this is that when the user does 'winheight' duplicate copies of a
    window can be added to the global tui_windows list.
    
    The original patch fixed this by changing the apply function to only
    update the global list some of the time.
    
    This patch takes a different approach.  The apply function no longer
    updates the global tui_windows list.  Instead a new virtual function
    is added to tui_layout_base which is used to gather all the currently
    applied windows into a vector.  Finally tui_apply_current_layout is
    updated to make use of this new function to update the tui_windows
    list.
    
    The benefits I see in this approach are, (a) the apply function now no
    longer touches global state, this solves the immediate problem,
    and (b) now that tui_windows is updated directly in the function
    tui_apply_current_layout, we can drop the saved_tui_windows global.
    
    gdb/ChangeLog:
    
            * tui-layout.c (saved_tui_windows): Delete.
            (tui_apply_current_layout): Don't make use of saved_tui_windows,
            call new get_windows member function instead.
            (tui_get_window_by_name): Check in tui_windows.
            (tui_layout_window::apply): Don't add to tui_windows.
            * tui-layout.h (tui_layout_base::get_windows): New member function.
            (tui_layout_window::get_windows): Likewise.
            (tui_layout_split::get_windows): Likewise.
    
    gdb/testsuite/ChangeLog:
    
            * gdb.tui/winheight.exp: Add more tests.

diff --git a/gdb/testsuite/gdb.tui/winheight.exp b/gdb/testsuite/gdb.tui/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.  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.  */
 std::vector<tui_win_info *> tui_windows;
 
-/* When applying a layout, this is the list of all windows that were
-   in the previous layout.  This is used to re-use windows when
-   changing a layout.  */
-static std::vector<tui_win_info *> saved_tui_windows;
-
 /* See tui-layout.h.  */
 
 void
@@ -79,10 +74,7 @@ tui_apply_current_layout ()
 
   extract_display_start_addr (&gdbarch, &addr);
 
-  saved_tui_windows = std::move (tui_windows);
-  tui_windows.clear ();
-
-  for (tui_win_info *win_info : saved_tui_windows)
+  for (tui_win_info *win_info : tui_windows)
     win_info->make_visible (false);
 
   applied_layout->apply (0, 0, tui_term_width (), tui_term_height ());
@@ -96,25 +88,33 @@ tui_apply_current_layout ()
   /* This should always be made visible by a layout.  */
   gdb_assert (TUI_CMD_WIN->is_visible ());
 
+  /* Get the new list of currently visible windows.  */
+  std::vector<tui_win_info *> new_tui_windows;
+  applied_layout->get_windows (&new_tui_windows);
+
   /* Now delete any window that was not re-applied.  */
   tui_win_info *focus = tui_win_with_focus ();
   tui_win_info *locator = tui_locator_win_info_ptr ();
-  for (tui_win_info *win_info : saved_tui_windows)
+  for (tui_win_info *win_info : tui_windows)
     {
       if (!win_info->is_visible ())
 	{
+	  /* If we're about to delete the window with focus then don't.
+	     This will be deleted later once we have selected an
+	     alternative window to focus on.  */
 	  if (focus == win_info)
-	    tui_set_win_focus_to (tui_windows[0]);
+	    tui_set_win_focus_to (new_tui_windows[0]);
 	  if (win_info != locator)
 	    delete win_info;
 	}
     }
 
+  tui_windows.clear ();
+  tui_windows = std::move (new_tui_windows);
+
   if (gdbarch == nullptr && TUI_DISASM_WIN != nullptr)
     tui_get_begin_asm_address (&gdbarch, &addr);
   tui_update_source_windows_with_addr (gdbarch, addr);
-
-  saved_tui_windows.clear ();
 }
 
 /* See tui-layout.  */
@@ -354,7 +354,7 @@ static std::unordered_map<std::string, window_factory> *known_window_types;
 static tui_win_info *
 tui_get_window_by_name (const std::string &name)
 {
-  for (tui_win_info *window : saved_tui_windows)
+  for (tui_win_info *window : tui_windows)
     if (name == window->name ())
       return window;
 
@@ -424,7 +424,6 @@ tui_layout_window::apply (int x_, int y_, int width_, int height_)
   height = height_;
   gdb_assert (m_window != nullptr);
   m_window->resize (height, width, x, y);
-  tui_windows.push_back (m_window);
 }
 
 /* See tui-layout.h.  */
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
      depth of this layout in the hierarchy (zero-based).  */
   virtual void specification (ui_file *output, int depth) = 0;
 
+  /* Add all windows to the WINDOWS vector.  */
+  virtual void get_windows (std::vector<tui_win_info *> *windows) = 0;
+
   /* The most recent space allocation.  */
   int x = 0;
   int y = 0;
@@ -141,6 +144,12 @@ class tui_layout_window : public tui_layout_base
 
   void specification (ui_file *output, int depth) override;
 
+  /* See tui_layout_base::get_windows.  */
+  void get_windows (std::vector<tui_win_info *> *windows) override
+  {
+    windows->push_back (m_window);
+  }
+
 protected:
 
   void get_sizes (bool height, int *min_value, int *max_value) override;
@@ -195,6 +204,13 @@ class tui_layout_split : public tui_layout_base
 
   void specification (ui_file *output, int depth) override;
 
+  /* See tui_layout_base::get_windows.  */
+  void get_windows (std::vector<tui_win_info *> *windows) override
+  {
+    for (auto &item : m_splits)
+      item.layout->get_windows (windows);
+  }
+
 protected:
 
   void get_sizes (bool height, int *min_value, int *max_value) override;


More information about the Gdb-patches mailing list