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/ChangeLog b/gdb/ChangeLog
index 3b9615dfe4214ade0cc637b45904e2be7247175d..d450be6433dc4b3344a71662644c7e91e22d1806 100644 (file)
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2021-02-08  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+       * 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.
+
 2021-02-08  Andrew Burgess  <andrew.burgess@embecosm.com>
 
        * tui/tui-layout.c (tui_apply_current_layout): Restore the delete

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 8fb69c0bacf627153f99c440576c4eaac7d4895e..de68598dc5ffa7991ec98e6e026cbc1d22b9eba2 100644 (file)
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2021-02-08  Andrew Burgess  <andrew.burgess@embecosm.com>
+
+       * gdb.tui/winheight.exp: Add more tests.
+
 2021-02-08  Andrew Burgess  <andrew.burgess@embecosm.com>
 
        * gdb.python/py-framefilter.exp: Update expected results.

diff --git a/gdb/testsuite/gdb.tui/winheight.exp b/gdb/testsuite/gdb.tui/winheight.exp
index 38fb29c382958c27e742cf716325451b08e8f95b..04de35d2f45c244f61af0721fdaea9a7f87d93a8 100644 (file)
--- 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 f01e2f9f3a6a6dcc28b5704d6a0a1d656da07f7b..f08d62d681fb1a8486cadc57d6e08e6ab9b15971 100644 (file)
--- 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,23 +88,28 @@ 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 ();
-  for (tui_win_info *win_info : saved_tui_windows)
+  for (tui_win_info *win_info : tui_windows)
     {
       if (!win_info->is_visible ())
        {
          if (focus == win_info)
-           tui_set_win_focus_to (tui_windows[0]);
+           tui_set_win_focus_to (new_tui_windows[0]);
          delete win_info;
        }
     }
 
+  /* Replace the global list of active windows.  */
+  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.  */
@@ -343,7 +340,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;
 
@@ -415,7 +412,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 193f42de4205f2fe39229575a99e4061b28a9113..f89166eae3789621fc20ea79f6612e6b1d0be20a 100644 (file)
--- a/gdb/tui/tui-layout.h
+++ b/gdb/tui/tui-layout.h
@@ -91,6 +91,9 @@ public:
      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 @@ public:
 
   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 @@ public:
 
   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;