* [PATCH v2] Don't add window duplicates to tui_windows [not found] <20210105135549.5704-1-ssbssa.ref@yahoo.de> @ 2021-01-05 13:55 ` Hannes Domani 2021-01-25 16:18 ` Andrew Burgess 0 siblings, 1 reply; 5+ messages in thread From: Hannes Domani @ 2021-01-05 13:55 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Don't add window duplicates to tui_windows 2021-01-05 13:55 ` [PATCH v2] Don't add window duplicates to tui_windows Hannes Domani @ 2021-01-25 16:18 ` Andrew Burgess 2021-01-25 17:31 ` Hannes Domani 2021-02-06 20:24 ` Tom Tromey 0 siblings, 2 replies; 5+ messages in thread From: Andrew Burgess @ 2021-01-25 16:18 UTC (permalink / raw) To: Hannes Domani; +Cc: gdb-patches, Tom Tromey * 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; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Don't add window duplicates to tui_windows 2021-01-25 16:18 ` Andrew Burgess @ 2021-01-25 17:31 ` Hannes Domani 2021-01-26 9:29 ` Andrew Burgess 2021-02-06 20:24 ` Tom Tromey 1 sibling, 1 reply; 5+ messages in thread From: Hannes Domani @ 2021-01-25 17:31 UTC (permalink / raw) To: Andrew Burgess; +Cc: gdb-patches, Tom Tromey 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; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Don't add window duplicates to tui_windows 2021-01-25 17:31 ` Hannes Domani @ 2021-01-26 9:29 ` Andrew Burgess 0 siblings, 0 replies; 5+ messages in thread From: Andrew Burgess @ 2021-01-26 9:29 UTC (permalink / raw) To: Hannes Domani; +Cc: gdb-patches, Tom Tromey * 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; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Don't add window duplicates to tui_windows 2021-01-25 16:18 ` Andrew Burgess 2021-01-25 17:31 ` Hannes Domani @ 2021-02-06 20:24 ` Tom Tromey 1 sibling, 0 replies; 5+ messages in thread From: Tom Tromey @ 2021-02-06 20:24 UTC (permalink / raw) To: Andrew Burgess; +Cc: Hannes Domani, Tom Tromey, gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-02-06 20:25 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20210105135549.5704-1-ssbssa.ref@yahoo.de> 2021-01-05 13:55 ` [PATCH v2] Don't add window duplicates to tui_windows Hannes Domani 2021-01-25 16:18 ` Andrew Burgess 2021-01-25 17:31 ` Hannes Domani 2021-01-26 9:29 ` Andrew Burgess 2021-02-06 20:24 ` Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).