From: Andrew Burgess Date: Mon, 25 Jan 2021 15:46:58 +0000 (+0000) Subject: gdb/tui: don't add windows to global list from tui_layout:window::apply X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=1cf2399651ef3fe1350ad8276cf00d16ddeb9960;p=binutils-gdb.git 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 3b9615dfe42..d450be6433d 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,14 @@ +2021-02-08 Andrew Burgess + + * 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 * tui/tui-layout.c (tui_apply_current_layout): Restore the delete diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 8fb69c0bacf..de68598dc5f 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,7 @@ +2021-02-08 Andrew Burgess + + * gdb.tui/winheight.exp: Add more tests. + 2021-02-08 Andrew Burgess * 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 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 f01e2f9f3a6..f08d62d681f 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_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 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 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 *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 193f42de420..f89166eae37 100644 --- 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 *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 *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 *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;