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

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

[Hannes Domani]

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);
 }
 
 /* See tui-layout.h.  */
@@ -653,7 +655,7 @@ tui_layout_split::adjust_size (const char *name, int new_height)
   else
     {
       /* Simply re-apply the updated layout.  */
-      apply (x, y, width, height);
+      apply (x, y, width, height, false);
     }
 
   return HANDLED;
@@ -662,7 +664,8 @@ tui_layout_split::adjust_size (const char *name, int new_height)
 /* See tui-layout.h.  */
 
 void
-tui_layout_split::apply (int x_, int y_, int width_, int height_)
+tui_layout_split::apply (int x_, int y_, int width_, int height_,
+			 bool add_window)
 {
   x = x_;
   y = y_;
@@ -772,9 +775,11 @@ tui_layout_split::apply (int x_, int y_, int width_, int height_)
       else if (info[i].share_box)
 	--size_accum;
       if (m_vertical)
-	m_splits[i].layout->apply (x, y + size_accum, width, info[i].size);
+	m_splits[i].layout->apply (x, y + size_accum, width, info[i].size,
+				   add_window);
       else
-	m_splits[i].layout->apply (x + size_accum, y, info[i].size, height);
+	m_splits[i].layout->apply (x + size_accum, y, info[i].size, height,
+				   add_window);
       size_accum += info[i].size;
     }
 
diff --git a/gdb/tui/tui-layout.h b/gdb/tui/tui-layout.h
index 193f42de420..7eb8ea911d3 100644
--- a/gdb/tui/tui-layout.h
+++ b/gdb/tui/tui-layout.h
@@ -56,7 +56,8 @@ class tui_layout_base
   virtual std::unique_ptr<tui_layout_base> clone () const = 0;
 
   /* Change the size and location of this layout.  */
-  virtual void apply (int x, int y, int width, int height) = 0;
+  virtual void apply (int x, int y, int width, int height,
+		      bool add_window) = 0;
 
   /* Return the minimum and maximum height or width of this layout.
      HEIGHT is true to fetch height, false to fetch width.  */
@@ -117,7 +118,7 @@ class tui_layout_window : public tui_layout_base
 
   std::unique_ptr<tui_layout_base> clone () const override;
 
-  void apply (int x, int y, int width, int height) override;
+  void apply (int x, int y, int width, int height, bool add_window) override;
 
   const char *get_name () const override
   {
@@ -181,7 +182,7 @@ class tui_layout_split : public tui_layout_base
 
   std::unique_ptr<tui_layout_base> clone () const override;
 
-  void apply (int x, int y, int width, int height) override;
+  void apply (int x, int y, int width, int height, bool add_window) override;
 
   tui_adjust_result adjust_size (const char *name, int new_height) override;
 
-- 
2.29.2

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

[Andrew Burgess]

* 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;

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

[Hannes Domani]

 Am Montag, 25. Januar 2021, 17:19:16 MEZ hat Andrew Burgess <andrew.burgess@embecosm.com> Folgendes geschrieben:

> * 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?

I think this is fine, I only have one question below.


> 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 ();

Isn't this clear() redundant?


> +  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;

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

[Andrew Burgess]

* Hannes Domani <ssbssa@yahoo.de> [2021-01-25 17:31:35 +0000]:

>  Am Montag, 25. Januar 2021, 17:19:16 MEZ hat Andrew Burgess <andrew.burgess@embecosm.com> Folgendes geschrieben:
> 
> > * 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?
> 
> I think this is fine, I only have one question below.
> 
> 
> > 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 ();
> 
> Isn't this clear() redundant?

Good catch, I'll drop that from the change.

Thanks,
Andrew



> 
> 
> > +  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;

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

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> I wondered if there was a solution to this problem that would remove
Andrew> the need to update some global state from the ::apply function.

This seems like a good idea.  Thank you for doing this.

Tom