[PATCH v2 00/14] Cleanups for the TUi register window

[PATCH v2 00/14] Cleanups for the TUi register window

[Tom Tromey]

This series is a grab-bag of cleanups to the TUI register window.

I've tried to untangle the code somewhat, with the goal being removing
the extra rerender overload.

This series also fixes the 'exit' bug and removes the hacky and
unnecessary recursion flag from tui-hooks.c.

More cleanups here are possible -- the register window is pretty old
code and pretty ugly.  In particular:

* the layout code is still pretty bad

* it doesn't really make sense for check_register_values to accept a
  frame, because the window can only really ever use the selected
  frame anyway

* there's no horizontal scrolling, but for vector registers this might
  be nice (there's a bug about this)

---
Changes in v2:
- Addressed review comments
- Link to v1: https://inbox.sourceware.org/gdb-patches/20231217-tui-regs-cleanup-v1-0-67bd0ea1e8be@tromey.com

---
Tom Tromey (14):
      Use pop_back in tui_register_format
      Minor C++ cleanups in tui-regs.c
      Simplify tui_data_window::show_register_group
      Rename tui_data_item_window -> tui_register_info
      Change tui_register_info::visible to a method
      Move scrollok call in register window
      Simplify update_register_data
      Remove the TUI register window rerender overload
      Simplify tui_data_win::erase_data_content
      Remove tui_refreshing_registers
      Remove redundant check from tui_refresh_frame_and_register_information
      Return void from tui_show_frame_info
      Rename show_registers -> set_register_group
      Update TUI register window when the inferior exits

 gdb/testsuite/gdb.tui/regs.exp |   8 ++
 gdb/tui/tui-hooks.c            |  36 +++----
 gdb/tui/tui-regs.c             | 229 +++++++++++++++--------------------------
 gdb/tui/tui-regs.h             |  59 ++++++-----
 gdb/tui/tui-status.c           |  16 ++-
 gdb/tui/tui-status.h           |   2 +-
 6 files changed, 148 insertions(+), 202 deletions(-)
---
base-commit: 7ae24327467750c445733e40d840e502795dbdf3
change-id: 20231217-tui-regs-cleanup-36d8f390a65a

Best regards,
-- 
Tom Tromey <tom@tromey.com>

[PATCH v2 01/14] Use pop_back in tui_register_format

[Tom Tromey]

tui_register_format can use string::pop_back now.

Tested-By: Tom de Vries <tdevries@suse.de>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
---
 gdb/tui/tui-regs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 4a6696d99e7..590e9e81317 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -96,7 +96,7 @@ tui_register_format (frame_info_ptr frame, int regnum)
   /* Remove the possible \n.  */
   std::string str = stream.release ();
   if (!str.empty () && str.back () == '\n')
-    str.resize (str.size () - 1);
+    str.pop_back ();
 
   return str;
 }

-- 
2.43.0

[PATCH v2 02/14] Minor C++ cleanups in tui-regs.c

[Tom Tromey]

This changes a couple of spots to use nullptr rather than 0, and
changes an int to a bool.

Tested-By: Tom de Vries <tdevries@suse.de>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
---
 gdb/tui/tui-regs.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 590e9e81317..f1c3e2ed9d4 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -173,7 +173,7 @@ tui_data_window::first_reg_element_no_inline (int line_no) const
 void
 tui_data_window::show_registers (const reggroup *group)
 {
-  if (group == 0)
+  if (group == nullptr)
     group = general_reggroup;
 
   if (target_has_registers () && target_has_stack () && target_has_memory ())
@@ -188,7 +188,7 @@ tui_data_window::show_registers (const reggroup *group)
     }
   else
     {
-      m_current_group = 0;
+      m_current_group = nullptr;
       m_regs_content.clear ();
     }
 
@@ -472,9 +472,7 @@ tui_data_window::check_register_values (frame_info_ptr frame)
     {
       for (auto &&data_item_win : m_regs_content)
 	{
-	  int was_hilighted;
-
-	  was_hilighted = data_item_win.highlight;
+	  bool was_hilighted = data_item_win.highlight;
 
 	  tui_get_register (frame, &data_item_win,
 			    data_item_win.regno,

-- 
2.43.0

[PATCH v2 03/14] Simplify tui_data_window::show_register_group

[Tom Tromey]

This simplifies tui_data_window::show_register_group, renaming it as
well.  The old method had two loops to iterate over all the registers
for the arch, but in the new scheme, the vector is set up when
switching groups, and then updates simply iterate over the vector.

tui_data_item_window is made self-updating, which also clarifies the
code somewhat.

Tested-By: Tom de Vries <tdevries@suse.de>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
---
 gdb/tui/tui-regs.c | 113 ++++++++++++++++++++---------------------------------
 gdb/tui/tui-regs.h |  21 +++++++---
 2 files changed, 57 insertions(+), 77 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index f1c3e2ed9d4..f7911b53bfb 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -101,24 +101,20 @@ tui_register_format (frame_info_ptr frame, int regnum)
   return str;
 }
 
-/* Get the register value from the given frame and format it for the
-   display.  When changedp is set, check if the new register value has
-   changed with respect to the previous call.  */
-static void
-tui_get_register (frame_info_ptr frame,
-		  struct tui_data_item_window *data, 
-		  int regnum, bool *changedp)
+/* Compute the register value from the given frame and format it for
+   the display.  update 'content' and set 'highlight' if the contents
+   changed.  */
+void
+tui_data_item_window::update (const frame_info_ptr &frame)
 {
-  if (changedp)
-    *changedp = false;
   if (target_has_registers ())
     {
-      std::string new_content = tui_register_format (frame, regnum);
+      std::string new_content = tui_register_format (frame, m_regno);
 
-      if (changedp != NULL && data->content != new_content)
-	*changedp = true;
+      if (content != new_content)
+	highlight = true;
 
-      data->content = std::move (new_content);
+      content = std::move (new_content);
     }
 }
 
@@ -178,13 +174,11 @@ tui_data_window::show_registers (const reggroup *group)
 
   if (target_has_registers () && target_has_stack () && target_has_memory ())
     {
-      show_register_group (group, get_selected_frame (NULL),
-			   group == m_current_group);
+      update_register_data (group, get_selected_frame (nullptr));
 
       /* Clear all notation of changed values.  */
       for (auto &&data_item_win : m_regs_content)
 	data_item_win.highlight = false;
-      m_current_group = group;
     }
   else
     {
@@ -197,67 +191,46 @@ tui_data_window::show_registers (const reggroup *group)
 
 
 /* Set the data window to display the registers of the register group
-   using the given frame.  Values are refreshed only when
-   refresh_values_only is true.  */
+   using the given frame.  */
 
 void
-tui_data_window::show_register_group (const reggroup *group,
-				      frame_info_ptr frame, 
-				      bool refresh_values_only)
+tui_data_window::update_register_data (const reggroup *group,
+				       frame_info_ptr frame)
 {
-  struct gdbarch *gdbarch = get_frame_arch (frame);
-  int nr_regs;
-  int regnum, pos;
-
-  /* Make a new title showing which group we display.  */
-  this->set_title (string_printf ("Register group: %s", group->name ()));
-
-  /* See how many registers must be displayed.  */
-  nr_regs = 0;
-  for (regnum = 0; regnum < gdbarch_num_cooked_regs (gdbarch); regnum++)
+  if (group != m_current_group)
     {
-      const char *name;
-
-      /* Must be in the group.  */
-      if (!gdbarch_register_reggroup_p (gdbarch, regnum, group))
-	continue;
-
-      /* If the register name is empty, it is undefined for this
-	 processor, so don't display anything.  */
-      name = gdbarch_register_name (gdbarch, regnum);
-      if (*name == '\0')
-	continue;
-
-      nr_regs++;
-    }
+      m_current_group = group;
 
-  m_regs_content.resize (nr_regs);
+      /* Make a new title showing which group we display.  */
+      this->set_title (string_printf ("Register group: %s", group->name ()));
 
-  /* Now set the register names and values.  */
-  pos = 0;
-  for (regnum = 0; regnum < gdbarch_num_cooked_regs (gdbarch); regnum++)
-    {
-      struct tui_data_item_window *data_item_win;
-      const char *name;
+      /* Create the registers.  */
+      m_regs_content.clear ();
 
-      /* Must be in the group.  */
-      if (!gdbarch_register_reggroup_p (gdbarch, regnum, group))
-	continue;
+      struct gdbarch *gdbarch = get_frame_arch (frame);
+      for (int regnum = 0;
+	   regnum < gdbarch_num_cooked_regs (gdbarch);
+	   regnum++)
+	{
+	  /* Must be in the group.  */
+	  if (!gdbarch_register_reggroup_p (gdbarch, regnum, group))
+	    continue;
 
-      /* If the register name is empty, it is undefined for this
-	 processor, so don't display anything.  */
-      name = gdbarch_register_name (gdbarch, regnum);
-      if (*name == '\0')
-	continue;
+	  /* If the register name is empty, it is undefined for this
+	     processor, so don't display anything.  */
+	  const char *name = gdbarch_register_name (gdbarch, regnum);
+	  if (*name == '\0')
+	    continue;
 
-      data_item_win = &m_regs_content[pos];
-      if (!refresh_values_only)
-	{
-	  data_item_win->regno = regnum;
-	  data_item_win->highlight = false;
+	  m_regs_content.emplace_back (regnum, frame);
 	}
-      tui_get_register (frame, data_item_win, regnum, 0);
-      pos++;
+    }
+  else
+    {
+      /* The group did not change, so we can simply update each
+	 item. */
+      for (tui_data_item_window &reg : m_regs_content)
+	reg.update (frame);
     }
 }
 
@@ -470,13 +443,11 @@ tui_data_window::check_register_values (frame_info_ptr frame)
     show_registers (m_current_group);
   else
     {
-      for (auto &&data_item_win : m_regs_content)
+      for (tui_data_item_window &data_item_win : m_regs_content)
 	{
 	  bool was_hilighted = data_item_win.highlight;
 
-	  tui_get_register (frame, &data_item_win,
-			    data_item_win.regno,
-			    &data_item_win.highlight);
+	  data_item_win.update (frame);
 
 	  /* Register windows whose y == 0 are outside the visible area.  */
 	  if ((data_item_win.highlight || was_hilighted)
diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
index bb4a0cfdb60..c1f89a5c1a6 100644
--- a/gdb/tui/tui-regs.h
+++ b/gdb/tui/tui-regs.h
@@ -29,22 +29,32 @@
 
 struct tui_data_item_window
 {
-  tui_data_item_window () = default;
+  tui_data_item_window (int regno, const frame_info_ptr &frame)
+    : m_regno (regno)
+  {
+    update (frame);
+    highlight = false;
+  }
 
   DISABLE_COPY_AND_ASSIGN (tui_data_item_window);
 
   tui_data_item_window (tui_data_item_window &&) = default;
 
+  void update (const frame_info_ptr &frame);
+
   void rerender (WINDOW *handle, int field_width);
 
   /* Location.  */
   int x = 0;
   int y = 0;
-  /* The register number.  */
-  int regno = -1;
   bool highlight = false;
   bool visible = false;
   std::string content;
+
+private:
+
+  /* The register number.  */
+  const int m_regno;
 };
 
 /* The TUI registers window.  */
@@ -104,9 +114,8 @@ struct tui_data_window : public tui_win_info
      display off the end of the register display.  */
   void display_reg_element_at_line (int start_element_no, int start_line_no);
 
-  void show_register_group (const reggroup *group,
-			    frame_info_ptr frame,
-			    bool refresh_values_only);
+  void update_register_data (const reggroup *group,
+			     frame_info_ptr frame);
 
   /* Answer the number of the last line in the regs display.  If there
      are no registers (-1) is returned.  */

-- 
2.43.0

[PATCH v2 04/14] Rename tui_data_item_window -> tui_register_info

[Tom Tromey]

tui_data_item_window used to hold a curses window, but we removed that
ages ago.  Now it just holds information about a single register.
This patch renames the class to make it more clearly reflect its
meaning.

Tested-By: Tom de Vries <tdevries@suse.de>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
---
 gdb/tui/tui-regs.c | 13 +++++--------
 gdb/tui/tui-regs.h | 14 +++++++-------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index f7911b53bfb..bacb23ab298 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -105,15 +105,12 @@ tui_register_format (frame_info_ptr frame, int regnum)
    the display.  update 'content' and set 'highlight' if the contents
    changed.  */
 void
-tui_data_item_window::update (const frame_info_ptr &frame)
+tui_register_info::update (const frame_info_ptr &frame)
 {
   if (target_has_registers ())
     {
       std::string new_content = tui_register_format (frame, m_regno);
-
-      if (content != new_content)
-	highlight = true;
-
+      highlight = content != new_content;
       content = std::move (new_content);
     }
 }
@@ -229,7 +226,7 @@ tui_data_window::update_register_data (const reggroup *group,
     {
       /* The group did not change, so we can simply update each
 	 item. */
-      for (tui_data_item_window &reg : m_regs_content)
+      for (tui_register_info &reg : m_regs_content)
 	reg.update (frame);
     }
 }
@@ -443,7 +440,7 @@ tui_data_window::check_register_values (frame_info_ptr frame)
     show_registers (m_current_group);
   else
     {
-      for (tui_data_item_window &data_item_win : m_regs_content)
+      for (tui_register_info &data_item_win : m_regs_content)
 	{
 	  bool was_hilighted = data_item_win.highlight;
 
@@ -462,7 +459,7 @@ tui_data_window::check_register_values (frame_info_ptr frame)
 /* Display a register in a window.  If hilite is TRUE, then the value
    will be displayed in reverse video.  */
 void
-tui_data_item_window::rerender (WINDOW *handle, int field_width)
+tui_register_info::rerender (WINDOW *handle, int field_width)
 {
   /* In case the regs window is not boxed, we'll write the last char in the
      last line here, causing a scroll, so prevent that.  */
diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
index c1f89a5c1a6..e7125e457f5 100644
--- a/gdb/tui/tui-regs.h
+++ b/gdb/tui/tui-regs.h
@@ -25,20 +25,20 @@
 #include "tui/tui-data.h"
 #include "reggroups.h"
 
-/* A data item window.  */
+/* Information about the display of a single register.  */
 
-struct tui_data_item_window
+struct tui_register_info
 {
-  tui_data_item_window (int regno, const frame_info_ptr &frame)
+  tui_register_info (int regno, const frame_info_ptr &frame)
     : m_regno (regno)
   {
     update (frame);
     highlight = false;
   }
 
-  DISABLE_COPY_AND_ASSIGN (tui_data_item_window);
+  DISABLE_COPY_AND_ASSIGN (tui_register_info);
 
-  tui_data_item_window (tui_data_item_window &&) = default;
+  tui_register_info (tui_register_info &&) = default;
 
   void update (const frame_info_ptr &frame);
 
@@ -136,8 +136,8 @@ struct tui_data_window : public tui_win_info
 
   void erase_data_content (const char *prompt);
 
-  /* Windows that are used to display registers.  */
-  std::vector<tui_data_item_window> m_regs_content;
+  /* Information about each register in the current register group.  */
+  std::vector<tui_register_info> m_regs_content;
   int m_regs_column_count = 0;
   const reggroup *m_current_group = nullptr;
 

-- 
2.43.0

[PATCH v2 05/14] Change tui_register_info::visible to a method

[Tom Tromey]

tui_register_info::visible is redundant with the fact that y==0 means
that the register is not visible.  This patch changes this member in
favor of having a single indication of the register's visibility -- a
method with the same name.  This change makes it clear that
delete_data_content_windows is not needed, so this is removed as well.
---
 gdb/tui/tui-regs.c | 17 ++---------------
 gdb/tui/tui-regs.h |  8 +++-----
 2 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index bacb23ab298..12301552c34 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -268,7 +268,6 @@ tui_data_window::display_registers_from (int start_element_no)
 	  /* Create the window if necessary.  */
 	  m_regs_content[i].x = box_width () + (m_item_width * j);
 	  m_regs_content[i].y = cur_y;
-	  m_regs_content[i].visible = true;
 	  m_regs_content[i].rerender (handle.get (), m_item_width);
 	  i++;		/* Next register.  */
 	}
@@ -346,23 +345,13 @@ tui_data_window::first_data_item_displayed ()
 {
   for (int i = 0; i < m_regs_content.size (); i++)
     {
-      if (m_regs_content[i].visible)
+      if (m_regs_content[i].visible ())
 	return i;
     }
 
   return -1;
 }
 
-/* See tui-regs.h.  */
-
-void
-tui_data_window::delete_data_content_windows ()
-{
-  for (auto &win : m_regs_content)
-    win.visible = false;
-}
-
-
 void
 tui_data_window::erase_data_content (const char *prompt)
 {
@@ -400,7 +389,6 @@ tui_data_window::rerender (bool toplevel)
   else
     {
       erase_data_content (NULL);
-      delete_data_content_windows ();
       display_registers_from (0);
     }
 }
@@ -425,7 +413,6 @@ tui_data_window::do_scroll_vertical (int num_to_scroll)
     {
       first_line += num_to_scroll;
       erase_data_content (NULL);
-      delete_data_content_windows ();
       display_registers_from_line (first_line);
     }
 }
@@ -448,7 +435,7 @@ tui_data_window::check_register_values (frame_info_ptr frame)
 
 	  /* Register windows whose y == 0 are outside the visible area.  */
 	  if ((data_item_win.highlight || was_hilighted)
-	      && data_item_win.y > 0)
+	      && data_item_win.visible ())
 	    data_item_win.rerender (handle.get (), m_item_width);
 	}
     }
diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
index e7125e457f5..1b0eaa2033b 100644
--- a/gdb/tui/tui-regs.h
+++ b/gdb/tui/tui-regs.h
@@ -44,11 +44,13 @@ struct tui_register_info
 
   void rerender (WINDOW *handle, int field_width);
 
+  bool visible () const
+  { return y > 0; }
+
   /* Location.  */
   int x = 0;
   int y = 0;
   bool highlight = false;
-  bool visible = false;
   std::string content;
 
 private:
@@ -130,10 +132,6 @@ struct tui_data_window : public tui_win_info
      past the register area (-1) is returned.  */
   int first_reg_element_no_inline (int line_no) const;
 
-  /* Delete all the item windows in the data window.  This is usually
-     done when the data window is scrolled.  */
-  void delete_data_content_windows ();
-
   void erase_data_content (const char *prompt);
 
   /* Information about each register in the current register group.  */

-- 
2.43.0

[PATCH v2 06/14] Move scrollok call in register window

[Tom Tromey]

The register window calls scrollok each time a register is written to
the window.  However, we only need to call this once, at the start of
display.  (We could actually call it just once when the window is
made, but that would involve making another method virtual or adding a
new member -- both which I think are worse than this approach.)

Tested-By: Tom de Vries <tdevries@suse.de>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
---
 gdb/tui/tui-regs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 12301552c34..eeea03f87c1 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -236,6 +236,10 @@ tui_data_window::update_register_data (const reggroup *group,
 void
 tui_data_window::display_registers_from (int start_element_no)
 {
+  /* In case the regs window is not boxed, we'll write the last char in the
+     last line here, causing a scroll, so prevent that.  */
+  scrollok (handle.get (), FALSE);
+
   int max_len = 0;
   for (auto &&data_item_win : m_regs_content)
     {
@@ -448,10 +452,6 @@ tui_data_window::check_register_values (frame_info_ptr frame)
 void
 tui_register_info::rerender (WINDOW *handle, int field_width)
 {
-  /* In case the regs window is not boxed, we'll write the last char in the
-     last line here, causing a scroll, so prevent that.  */
-  scrollok (handle, FALSE);
-
   if (highlight)
     /* We ignore the return value, casting it to void in order to avoid
        a compiler warning.  The warning itself was introduced by a patch

-- 
2.43.0

[PATCH v2 07/14] Simplify update_register_data

[Tom Tromey]

This changes update_register_data to always update the register data.
The idea here is that this is really only called when either the
desired register group changes, or when gdb transitions from not
having a frame to having a frame.

show_registers is also simplified slightly to account for this.

Tested-By: Tom de Vries <tdevries@suse.de>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
---
 gdb/tui/tui-regs.c | 57 ++++++++++++++++++++----------------------------------
 1 file changed, 21 insertions(+), 36 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index eeea03f87c1..7af572c9da4 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -170,15 +170,10 @@ tui_data_window::show_registers (const reggroup *group)
     group = general_reggroup;
 
   if (target_has_registers () && target_has_stack () && target_has_memory ())
-    {
-      update_register_data (group, get_selected_frame (nullptr));
-
-      /* Clear all notation of changed values.  */
-      for (auto &&data_item_win : m_regs_content)
-	data_item_win.highlight = false;
-    }
+    update_register_data (group, get_selected_frame (nullptr));
   else
     {
+      set_title (_("Registers"));
       m_current_group = nullptr;
       m_regs_content.clear ();
     }
@@ -194,40 +189,30 @@ void
 tui_data_window::update_register_data (const reggroup *group,
 				       frame_info_ptr frame)
 {
-  if (group != m_current_group)
-    {
-      m_current_group = group;
+  m_current_group = group;
 
-      /* Make a new title showing which group we display.  */
-      this->set_title (string_printf ("Register group: %s", group->name ()));
+  /* Make a new title showing which group we display.  */
+  this->set_title (string_printf ("Register group: %s", group->name ()));
 
-      /* Create the registers.  */
-      m_regs_content.clear ();
+  /* Create the registers.  */
+  m_regs_content.clear ();
 
-      struct gdbarch *gdbarch = get_frame_arch (frame);
-      for (int regnum = 0;
-	   regnum < gdbarch_num_cooked_regs (gdbarch);
-	   regnum++)
-	{
-	  /* Must be in the group.  */
-	  if (!gdbarch_register_reggroup_p (gdbarch, regnum, group))
-	    continue;
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+  for (int regnum = 0;
+       regnum < gdbarch_num_cooked_regs (gdbarch);
+       regnum++)
+    {
+      /* Must be in the group.  */
+      if (!gdbarch_register_reggroup_p (gdbarch, regnum, group))
+	continue;
 
-	  /* If the register name is empty, it is undefined for this
-	     processor, so don't display anything.  */
-	  const char *name = gdbarch_register_name (gdbarch, regnum);
-	  if (*name == '\0')
-	    continue;
+      /* If the register name is empty, it is undefined for this
+	 processor, so don't display anything.  */
+      const char *name = gdbarch_register_name (gdbarch, regnum);
+      if (*name == '\0')
+	continue;
 
-	  m_regs_content.emplace_back (regnum, frame);
-	}
-    }
-  else
-    {
-      /* The group did not change, so we can simply update each
-	 item. */
-      for (tui_register_info &reg : m_regs_content)
-	reg.update (frame);
+      m_regs_content.emplace_back (regnum, frame);
     }
 }
 

-- 
2.43.0

[PATCH v2 08/14] Remove the TUI register window rerender overload

[Tom Tromey]

After these restructurings, it should be clear that the rerender
overload can be removed from the TUI register window.  This is done by
moving a bit more logic from show_registers into update_register_data.
After this, update_register_data simply updates the internal state,
and rerender will write to the screen.  All the actual rendering work
is done, ultimately, by display_registers_from.  This split between
updating the mode and rendering makes it clear that the recursive case
can't happen any longer.

Tested-By: Tom de Vries <tdevries@suse.de>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
---
 gdb/tui/tui-regs.c | 51 ++++++++++++++++++++++-----------------------------
 gdb/tui/tui-regs.h | 14 ++++++--------
 2 files changed, 28 insertions(+), 37 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 7af572c9da4..52cf6b7efdf 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -165,30 +165,32 @@ tui_data_window::first_reg_element_no_inline (int line_no) const
    and refresh the window.  */
 void
 tui_data_window::show_registers (const reggroup *group)
+{
+  update_register_data (group);
+  rerender ();
+}
+
+/* Set the data window to display the registers of the register group
+   using the given frame.  */
+
+void
+tui_data_window::update_register_data (const reggroup *group)
 {
   if (group == nullptr)
     group = general_reggroup;
 
-  if (target_has_registers () && target_has_stack () && target_has_memory ())
-    update_register_data (group, get_selected_frame (nullptr));
-  else
+  if (!target_has_registers ()
+      || !target_has_stack ()
+      || !target_has_memory ())
     {
       set_title (_("Registers"));
       m_current_group = nullptr;
       m_regs_content.clear ();
+      return;
     }
 
-  rerender (false);
-}
-
+  frame_info_ptr frame = get_selected_frame (nullptr);
 
-/* Set the data window to display the registers of the register group
-   using the given frame.  */
-
-void
-tui_data_window::update_register_data (const reggroup *group,
-				       frame_info_ptr frame)
-{
   m_current_group = group;
 
   /* Make a new title showing which group we display.  */
@@ -221,6 +223,9 @@ tui_data_window::update_register_data (const reggroup *group,
 void
 tui_data_window::display_registers_from (int start_element_no)
 {
+  werase (handle.get ());
+  check_and_display_highlight_if_needed ();
+
   /* In case the regs window is not boxed, we'll write the last char in the
      last line here, causing a scroll, so prevent that.  */
   scrollok (handle.get (), FALSE);
@@ -357,29 +362,18 @@ tui_data_window::erase_data_content (const char *prompt)
 	x_pos = half_width - strlen (prompt);
       display_string (height / 2, x_pos, prompt);
     }
-  tui_wrefresh (handle.get ());
 }
 
 /* See tui-regs.h.  */
 
 void
-tui_data_window::rerender (bool toplevel)
+tui_data_window::rerender ()
 {
   if (m_regs_content.empty ())
-    {
-      if (toplevel && has_stack_frames ())
-	{
-	  frame_info_ptr fi = get_selected_frame (NULL);
-	  check_register_values (fi);
-	}
-      else
-	erase_data_content (_("[ Register Values Unavailable ]"));
-    }
+    erase_data_content (_("[ Register Values Unavailable ]"));
   else
-    {
-      erase_data_content (NULL);
-      display_registers_from (0);
-    }
+    display_registers_from (0);
+  tui_wrefresh (handle.get ());
 }
 
 
@@ -401,7 +395,6 @@ tui_data_window::do_scroll_vertical (int num_to_scroll)
   if (first_line >= 0)
     {
       first_line += num_to_scroll;
-      erase_data_content (NULL);
       display_registers_from_line (first_line);
     }
 }
diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
index 1b0eaa2033b..7f1c30ca5d6 100644
--- a/gdb/tui/tui-regs.h
+++ b/gdb/tui/tui-regs.h
@@ -62,7 +62,10 @@ struct tui_register_info
 /* The TUI registers window.  */
 struct tui_data_window : public tui_win_info
 {
-  tui_data_window () = default;
+  tui_data_window ()
+  {
+    update_register_data (nullptr);
+  }
 
   DISABLE_COPY_AND_ASSIGN (tui_data_window);
 
@@ -87,11 +90,7 @@ struct tui_data_window : public tui_win_info
   {
   }
 
-  void rerender (bool toplevel);
-  void rerender () override
-  {
-    rerender (true);
-  }
+  void rerender () override;
 
 private:
 
@@ -116,8 +115,7 @@ struct tui_data_window : public tui_win_info
      display off the end of the register display.  */
   void display_reg_element_at_line (int start_element_no, int start_line_no);
 
-  void update_register_data (const reggroup *group,
-			     frame_info_ptr frame);
+  void update_register_data (const reggroup *group);
 
   /* Answer the number of the last line in the regs display.  If there
      are no registers (-1) is returned.  */

-- 
2.43.0

[PATCH v2 09/14] Simplify tui_data_win::erase_data_content

[Tom Tromey]

There's only a single call to tui_data_win::erase_data_content now, so
remove the parameter and make it just render the "empty window" text.

Tested-By: Tom de Vries <tdevries@suse.de>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
---
 gdb/tui/tui-regs.c | 22 ++++++++++------------
 gdb/tui/tui-regs.h |  2 +-
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 52cf6b7efdf..10d9bc6ef7d 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -347,21 +347,19 @@ tui_data_window::first_data_item_displayed ()
 }
 
 void
-tui_data_window::erase_data_content (const char *prompt)
+tui_data_window::erase_data_content ()
 {
   werase (handle.get ());
   check_and_display_highlight_if_needed ();
-  if (prompt != NULL)
-    {
-      int half_width = (width - box_size ()) / 2;
-      int x_pos;
 
-      if (strlen (prompt) >= half_width)
-	x_pos = 1;
-      else
-	x_pos = half_width - strlen (prompt);
-      display_string (height / 2, x_pos, prompt);
-    }
+  const char *prompt = _("[ Register Values Unavailable ]");
+  int half_width = (width - box_size ()) / 2;
+  int x_pos;
+  if (strlen (prompt) >= half_width)
+    x_pos = 1;
+  else
+    x_pos = half_width - strlen (prompt);
+  display_string (height / 2, x_pos, prompt);
 }
 
 /* See tui-regs.h.  */
@@ -370,7 +368,7 @@ void
 tui_data_window::rerender ()
 {
   if (m_regs_content.empty ())
-    erase_data_content (_("[ Register Values Unavailable ]"));
+    erase_data_content ();
   else
     display_registers_from (0);
   tui_wrefresh (handle.get ());
diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
index 7f1c30ca5d6..880f360bec6 100644
--- a/gdb/tui/tui-regs.h
+++ b/gdb/tui/tui-regs.h
@@ -130,7 +130,7 @@ struct tui_data_window : public tui_win_info
      past the register area (-1) is returned.  */
   int first_reg_element_no_inline (int line_no) const;
 
-  void erase_data_content (const char *prompt);
+  void erase_data_content ();
 
   /* Information about each register in the current register group.  */
   std::vector<tui_register_info> m_regs_content;

-- 
2.43.0

[PATCH v2 10/14] Remove tui_refreshing_registers

[Tom Tromey]

The comment by tui_refreshing_registers mentions a hook that no longer
exists.  However, maybe the comment is wrong.

The code paths touching tui_refreshing_registers can only be called in two places:

1. From the before_prompt observer.  This is only called when a prompt
   is about to be displayed.

2. From the register_changed observer.  This is only called when
   value_assign changes a register value.

From this it seems clear that the recursion case here cannot in fact
occur.  This patch removes the variable.

Tested-By: Tom de Vries <tdevries@suse.de>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
---
 gdb/tui/tui-hooks.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index bd8f99b786b..3a4eda91aa3 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -63,9 +63,6 @@ static void
 tui_all_objfiles_removed (program_space *pspace)
 { tui_on_objfiles_changed (); }
 
-/* Prevent recursion of deprecated_register_changed_hook().  */
-static bool tui_refreshing_registers = false;
-
 /* Observer for the register_changed notification.  */
 
 static void
@@ -82,12 +79,7 @@ tui_register_changed (frame_info_ptr frame, int regno)
      up in the other.  So we always use the selected frame here, and ignore
      FRAME.  */
   fi = get_selected_frame (NULL);
-  if (!tui_refreshing_registers)
-    {
-      tui_refreshing_registers = true;
-      TUI_DATA_WIN->check_register_values (fi);
-      tui_refreshing_registers = false;
-    }
+  TUI_DATA_WIN->check_register_values (fi);
 }
 
 /* Breakpoint creation hook.
@@ -145,11 +137,7 @@ tui_refresh_frame_and_register_information ()
       /* Refresh the register window if it's visible.  */
       if (tui_is_window_visible (DATA_WIN)
 	  && (frame_info_changed_p || from_stack))
-	{
-	  tui_refreshing_registers = true;
-	  TUI_DATA_WIN->check_register_values (fi);
-	  tui_refreshing_registers = false;
-	}
+	TUI_DATA_WIN->check_register_values (fi);
     }
   else if (!from_stack)
     {

-- 
2.43.0

[PATCH v2 11/14] Remove redundant check from tui_refresh_frame_and_register_information

[Tom Tromey]

tui_refresh_frame_and_register_information checks 'from_stack' in a
block that's already guarded by a 'from_stack' check.  This patch
removes the redundant check.

Tested-By: Tom de Vries <tdevries@suse.de>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
---
 gdb/tui/tui-hooks.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index 3a4eda91aa3..fc7ffb4f202 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -132,11 +132,10 @@ tui_refresh_frame_and_register_information ()
 
       /* Display the frame position (even if there is no symbols or
 	 the PC is not known).  */
-      bool frame_info_changed_p = tui_show_frame_info (fi);
+      tui_show_frame_info (fi);
 
       /* Refresh the register window if it's visible.  */
-      if (tui_is_window_visible (DATA_WIN)
-	  && (frame_info_changed_p || from_stack))
+      if (tui_is_window_visible (DATA_WIN))
 	TUI_DATA_WIN->check_register_values (fi);
     }
   else if (!from_stack)

-- 
2.43.0

[PATCH v2 12/14] Return void from tui_show_frame_info

[Tom Tromey]

Nothing uses the tui_show_frame_info result any more, so change it to
return void.

Tested-By: Tom de Vries <tdevries@suse.de>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
---
 gdb/tui/tui-status.c | 16 ++++++----------
 gdb/tui/tui-status.h |  2 +-
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/gdb/tui/tui-status.c b/gdb/tui/tui-status.c
index e61bf71c600..df4675fe5df 100644
--- a/gdb/tui/tui-status.c
+++ b/gdb/tui/tui-status.c
@@ -262,13 +262,11 @@ tui_status_window::rerender ()
   wmove (handle.get (), 0, 0);
 }
 
-/* Function to print the frame information for the TUI.  The windows are
-   refreshed only if frame information has changed since the last refresh.
+/* Function to print the frame information for the TUI.  The windows
+   are refreshed only if frame information has changed since the last
+   refresh.  */
 
-   Return true if frame information has changed (and windows
-   subsequently refreshed), false otherwise.  */
-
-bool
+void
 tui_show_frame_info (frame_info_ptr fi)
 {
   bool status_changed_p;
@@ -292,7 +290,7 @@ tui_show_frame_info (frame_info_ptr fi)
 	 not changed.  If frame information has not changed, then the windows'
 	 contents will not change.  So don't bother refreshing the windows.  */
       if (!status_changed_p)
-	return false;
+	return;
 
       for (struct tui_source_window_base *win_info : tui_source_windows ())
 	{
@@ -307,13 +305,11 @@ tui_show_frame_info (frame_info_ptr fi)
       status_changed_p = tui_location.set_location (NULL, sal, "");
 
       if (!status_changed_p)
-	return false;
+	return;
 
       for (struct tui_source_window_base *win_info : tui_source_windows ())
 	win_info->erase_source_content ();
     }
-
-  return true;
 }
 
 void
diff --git a/gdb/tui/tui-status.h b/gdb/tui/tui-status.h
index 4a0f59a2eaa..efef5375e86 100644
--- a/gdb/tui/tui-status.h
+++ b/gdb/tui/tui-status.h
@@ -51,6 +51,6 @@ struct tui_status_window
 };
 
 extern void tui_show_status_content (void);
-extern bool tui_show_frame_info (frame_info_ptr);
+extern void tui_show_frame_info (frame_info_ptr);
 
 #endif /* TUI_TUI_STATUS_H */

-- 
2.43.0

[PATCH v2 13/14] Rename show_registers -> set_register_group

[Tom Tromey]

This renames a method on the TUI register window to reflect its real
purpose.

Tested-By: Tom de Vries <tdevries@suse.de>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
---
 gdb/tui/tui-regs.c | 6 +++---
 gdb/tui/tui-regs.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 10d9bc6ef7d..fa4d10d1039 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -164,7 +164,7 @@ tui_data_window::first_reg_element_no_inline (int line_no) const
 /* Show the registers of the given group in the data window
    and refresh the window.  */
 void
-tui_data_window::show_registers (const reggroup *group)
+tui_data_window::set_register_group (const reggroup *group)
 {
   update_register_data (group);
   rerender ();
@@ -404,7 +404,7 @@ void
 tui_data_window::check_register_values (frame_info_ptr frame)
 {
   if (m_regs_content.empty ())
-    show_registers (m_current_group);
+    set_register_group (m_current_group);
   else
     {
       for (tui_register_info &data_item_win : m_regs_content)
@@ -542,7 +542,7 @@ tui_reg_command (const char *args, int from_tty)
       if (match == NULL)
 	error (_("unknown register group '%s'"), args);
 
-      TUI_DATA_WIN->show_registers (match);
+      TUI_DATA_WIN->set_register_group (match);
     }
   else
     {
diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
index 880f360bec6..caa9d395bec 100644
--- a/gdb/tui/tui-regs.h
+++ b/gdb/tui/tui-regs.h
@@ -76,7 +76,7 @@ struct tui_data_window : public tui_win_info
 
   void check_register_values (frame_info_ptr frame);
 
-  void show_registers (const reggroup *group);
+  void set_register_group (const reggroup *group);
 
   const reggroup *get_current_group () const
   {

-- 
2.43.0

[PATCH v2 14/14] Update TUI register window when the inferior exits

[Tom Tromey]

When the inferior exits, the TUI register window should clear.

Fixing this was mostly a matter of sticking an assignment into
tui_inferior_exit.  However, some changes to the register window
itself were also needed.

While working on this, I realized that the TUI register window would
not work correctly when moving between frames of different
architectures.  This patch attempts to fix this as well, though I have
no way to test it.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28600
Tested-By: Tom de Vries <tdevries@suse.de>
Reviewed-By: Andrew Burgess <aburgess@redhat.com>
---
 gdb/testsuite/gdb.tui/regs.exp |  8 +++++++
 gdb/tui/tui-hooks.c            | 17 +++++++++------
 gdb/tui/tui-regs.c             | 48 ++++++++++++++++++++++++++----------------
 gdb/tui/tui-regs.h             |  4 ++++
 4 files changed, 53 insertions(+), 24 deletions(-)

diff --git a/gdb/testsuite/gdb.tui/regs.exp b/gdb/testsuite/gdb.tui/regs.exp
index c325952185f..ea78b57d1a7 100644
--- a/gdb/testsuite/gdb.tui/regs.exp
+++ b/gdb/testsuite/gdb.tui/regs.exp
@@ -30,6 +30,9 @@ if {![runto_main]} {
     return
 }
 
+# This is convenient later on.
+gdb_test_no_output "set confirm off"
+
 if {![Term::enter_tui]} {
     unsupported "TUI not supported"
     return
@@ -48,6 +51,11 @@ gdb_assert \
     { ![Term::check_region_contents_p 0 0 80 8 $re_reg_vals_unavailable] } \
     "Register values available"
 
+Term::command "kill"
+gdb_assert \
+    { [Term::check_region_contents_p 0 0 80 8 $re_reg_vals_unavailable] } \
+    "Register values no longer available"
+
 # Check that we can successfully cause the register window to appear
 # using the 'tui reg next' and 'tui reg prev' commands.
 foreach_with_prefix cmd { next prev } {
diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index fc7ffb4f202..28d0b742ed7 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -126,19 +126,23 @@ tui_refresh_frame_and_register_information ()
   target_terminal::scoped_restore_terminal_state term_state;
   target_terminal::ours_for_output ();
 
-  if (from_stack && has_stack_frames ())
+  if (from_stack)
     {
-      frame_info_ptr fi = get_selected_frame (NULL);
+      frame_info_ptr fi;
+      if (has_stack_frames ())
+	{
+	  fi = get_selected_frame (NULL);
 
-      /* Display the frame position (even if there is no symbols or
-	 the PC is not known).  */
-      tui_show_frame_info (fi);
+	  /* Display the frame position (even if there is no symbols or
+	     the PC is not known).  */
+	  tui_show_frame_info (fi);
+	}
 
       /* Refresh the register window if it's visible.  */
       if (tui_is_window_visible (DATA_WIN))
 	TUI_DATA_WIN->check_register_values (fi);
     }
-  else if (!from_stack)
+  else
     {
       /* Make sure that the source window is displayed.  */
       tui_add_win_to_layout (SRC_WIN);
@@ -169,6 +173,7 @@ tui_inferior_exit (struct inferior *inf)
   tui_set_key_mode (TUI_COMMAND_MODE);
   tui_show_frame_info (0);
   tui_display_main ();
+  from_stack = true;
 }
 
 /* Observer for the before_prompt notification.  */
diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index fa4d10d1039..504aed4b81f 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -107,12 +107,9 @@ tui_register_format (frame_info_ptr frame, int regnum)
 void
 tui_register_info::update (const frame_info_ptr &frame)
 {
-  if (target_has_registers ())
-    {
-      std::string new_content = tui_register_format (frame, m_regno);
-      highlight = content != new_content;
-      content = std::move (new_content);
-    }
+  std::string new_content = tui_register_format (frame, m_regno);
+  highlight = content != new_content;
+  content = std::move (new_content);
 }
 
 /* See tui-regs.h.  */
@@ -185,13 +182,22 @@ tui_data_window::update_register_data (const reggroup *group)
     {
       set_title (_("Registers"));
       m_current_group = nullptr;
+      m_gdbarch = nullptr;
       m_regs_content.clear ();
       return;
     }
 
   frame_info_ptr frame = get_selected_frame (nullptr);
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+
+  if (m_current_group == group && m_gdbarch == gdbarch)
+    {
+      /* Nothing to do here.  */
+      return;
+    }
 
   m_current_group = group;
+  m_gdbarch = gdbarch;
 
   /* Make a new title showing which group we display.  */
   this->set_title (string_printf ("Register group: %s", group->name ()));
@@ -199,7 +205,6 @@ tui_data_window::update_register_data (const reggroup *group)
   /* Create the registers.  */
   m_regs_content.clear ();
 
-  struct gdbarch *gdbarch = get_frame_arch (frame);
   for (int regnum = 0;
        regnum < gdbarch_num_cooked_regs (gdbarch);
        regnum++)
@@ -403,24 +408,31 @@ tui_data_window::do_scroll_vertical (int num_to_scroll)
 void
 tui_data_window::check_register_values (frame_info_ptr frame)
 {
-  if (m_regs_content.empty ())
-    set_register_group (m_current_group);
+  if (frame == nullptr)
+    set_register_group (nullptr);
   else
     {
-      for (tui_register_info &data_item_win : m_regs_content)
+      /* If the frame architecture changed, we need to reset the
+	 register group.  */
+      struct gdbarch *gdbarch = get_frame_arch (frame);
+      if (gdbarch != m_gdbarch)
+	set_register_group (nullptr);
+      else
 	{
-	  bool was_hilighted = data_item_win.highlight;
+	  for (tui_register_info &data_item_win : m_regs_content)
+	    {
+	      bool was_hilighted = data_item_win.highlight;
 
-	  data_item_win.update (frame);
+	      data_item_win.update (frame);
 
-	  /* Register windows whose y == 0 are outside the visible area.  */
-	  if ((data_item_win.highlight || was_hilighted)
-	      && data_item_win.visible ())
-	    data_item_win.rerender (handle.get (), m_item_width);
+	      /* Register windows whose y == 0 are outside the visible area.  */
+	      if ((data_item_win.highlight || was_hilighted)
+		  && data_item_win.visible ())
+		data_item_win.rerender (handle.get (), m_item_width);
+	    }
 	}
+      tui_wrefresh (handle.get ());
     }
-
-  tui_wrefresh (handle.get ());
 }
 
 /* Display a register in a window.  If hilite is TRUE, then the value
diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
index caa9d395bec..9d9cae40fe0 100644
--- a/gdb/tui/tui-regs.h
+++ b/gdb/tui/tui-regs.h
@@ -139,6 +139,10 @@ struct tui_data_window : public tui_win_info
 
   /* Width of each register's display area.  */
   int m_item_width = 0;
+
+  /* Architecture of frame whose registers are being displayed, or
+     nullptr if the display is empty (i.e., there is no frame).  */
+  gdbarch *m_gdbarch = nullptr;
 };
 
 #endif /* TUI_TUI_REGS_H */

-- 
2.43.0

Re: [PATCH v2 08/14] Remove the TUI register window rerender overload

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> After these restructurings, it should be clear that the rerender
> overload can be removed from the TUI register window.  This is done by
> moving a bit more logic from show_registers into update_register_data.
> After this, update_register_data simply updates the internal state,
> and rerender will write to the screen.  All the actual rendering work
> is done, ultimately, by display_registers_from.  This split between
> updating the mode and rendering makes it clear that the recursive case

I think you mean 'updating the model' here?

Thanks,
Andrew


> can't happen any longer.
>
> Tested-By: Tom de Vries <tdevries@suse.de>
> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
> ---
>  gdb/tui/tui-regs.c | 51 ++++++++++++++++++++++-----------------------------
>  gdb/tui/tui-regs.h | 14 ++++++--------
>  2 files changed, 28 insertions(+), 37 deletions(-)
>
> diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
> index 7af572c9da4..52cf6b7efdf 100644
> --- a/gdb/tui/tui-regs.c
> +++ b/gdb/tui/tui-regs.c
> @@ -165,30 +165,32 @@ tui_data_window::first_reg_element_no_inline (int line_no) const
>     and refresh the window.  */
>  void
>  tui_data_window::show_registers (const reggroup *group)
> +{
> +  update_register_data (group);
> +  rerender ();
> +}
> +
> +/* Set the data window to display the registers of the register group
> +   using the given frame.  */
> +
> +void
> +tui_data_window::update_register_data (const reggroup *group)
>  {
>    if (group == nullptr)
>      group = general_reggroup;
>  
> -  if (target_has_registers () && target_has_stack () && target_has_memory ())
> -    update_register_data (group, get_selected_frame (nullptr));
> -  else
> +  if (!target_has_registers ()
> +      || !target_has_stack ()
> +      || !target_has_memory ())
>      {
>        set_title (_("Registers"));
>        m_current_group = nullptr;
>        m_regs_content.clear ();
> +      return;
>      }
>  
> -  rerender (false);
> -}
> -
> +  frame_info_ptr frame = get_selected_frame (nullptr);
>  
> -/* Set the data window to display the registers of the register group
> -   using the given frame.  */
> -
> -void
> -tui_data_window::update_register_data (const reggroup *group,
> -				       frame_info_ptr frame)
> -{
>    m_current_group = group;
>  
>    /* Make a new title showing which group we display.  */
> @@ -221,6 +223,9 @@ tui_data_window::update_register_data (const reggroup *group,
>  void
>  tui_data_window::display_registers_from (int start_element_no)
>  {
> +  werase (handle.get ());
> +  check_and_display_highlight_if_needed ();
> +
>    /* In case the regs window is not boxed, we'll write the last char in the
>       last line here, causing a scroll, so prevent that.  */
>    scrollok (handle.get (), FALSE);
> @@ -357,29 +362,18 @@ tui_data_window::erase_data_content (const char *prompt)
>  	x_pos = half_width - strlen (prompt);
>        display_string (height / 2, x_pos, prompt);
>      }
> -  tui_wrefresh (handle.get ());
>  }
>  
>  /* See tui-regs.h.  */
>  
>  void
> -tui_data_window::rerender (bool toplevel)
> +tui_data_window::rerender ()
>  {
>    if (m_regs_content.empty ())
> -    {
> -      if (toplevel && has_stack_frames ())
> -	{
> -	  frame_info_ptr fi = get_selected_frame (NULL);
> -	  check_register_values (fi);
> -	}
> -      else
> -	erase_data_content (_("[ Register Values Unavailable ]"));
> -    }
> +    erase_data_content (_("[ Register Values Unavailable ]"));
>    else
> -    {
> -      erase_data_content (NULL);
> -      display_registers_from (0);
> -    }
> +    display_registers_from (0);
> +  tui_wrefresh (handle.get ());
>  }
>  
>  
> @@ -401,7 +395,6 @@ tui_data_window::do_scroll_vertical (int num_to_scroll)
>    if (first_line >= 0)
>      {
>        first_line += num_to_scroll;
> -      erase_data_content (NULL);
>        display_registers_from_line (first_line);
>      }
>  }
> diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
> index 1b0eaa2033b..7f1c30ca5d6 100644
> --- a/gdb/tui/tui-regs.h
> +++ b/gdb/tui/tui-regs.h
> @@ -62,7 +62,10 @@ struct tui_register_info
>  /* The TUI registers window.  */
>  struct tui_data_window : public tui_win_info
>  {
> -  tui_data_window () = default;
> +  tui_data_window ()
> +  {
> +    update_register_data (nullptr);
> +  }
>  
>    DISABLE_COPY_AND_ASSIGN (tui_data_window);
>  
> @@ -87,11 +90,7 @@ struct tui_data_window : public tui_win_info
>    {
>    }
>  
> -  void rerender (bool toplevel);
> -  void rerender () override
> -  {
> -    rerender (true);
> -  }
> +  void rerender () override;
>  
>  private:
>  
> @@ -116,8 +115,7 @@ struct tui_data_window : public tui_win_info
>       display off the end of the register display.  */
>    void display_reg_element_at_line (int start_element_no, int start_line_no);
>  
> -  void update_register_data (const reggroup *group,
> -			     frame_info_ptr frame);
> +  void update_register_data (const reggroup *group);
>  
>    /* Answer the number of the last line in the regs display.  If there
>       are no registers (-1) is returned.  */
>
> -- 
> 2.43.0

Re: [PATCH v2 09/14] Simplify tui_data_win::erase_data_content

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> There's only a single call to tui_data_win::erase_data_content now, so
> remove the parameter and make it just render the "empty window" text.
>
> Tested-By: Tom de Vries <tdevries@suse.de>
> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
> ---
>  gdb/tui/tui-regs.c | 22 ++++++++++------------
>  gdb/tui/tui-regs.h |  2 +-
>  2 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
> index 52cf6b7efdf..10d9bc6ef7d 100644
> --- a/gdb/tui/tui-regs.c
> +++ b/gdb/tui/tui-regs.c
> @@ -347,21 +347,19 @@ tui_data_window::first_data_item_displayed ()
>  }
>  
>  void
> -tui_data_window::erase_data_content (const char *prompt)
> +tui_data_window::erase_data_content ()
>  {
>    werase (handle.get ());
>    check_and_display_highlight_if_needed ();
> -  if (prompt != NULL)
> -    {
> -      int half_width = (width - box_size ()) / 2;
> -      int x_pos;
>  
> -      if (strlen (prompt) >= half_width)
> -	x_pos = 1;
> -      else
> -	x_pos = half_width - strlen (prompt);
> -      display_string (height / 2, x_pos, prompt);
> -    }
> +  const char *prompt = _("[ Register Values Unavailable ]");
> +  int half_width = (width - box_size ()) / 2;
> +  int x_pos;
> +  if (strlen (prompt) >= half_width)
> +    x_pos = 1;
> +  else
> +    x_pos = half_width - strlen (prompt);

Not your code, nor is this the only place this happens, but this string
placement algorithm is weird.  It places the string within one half of
the windows visible width, rather than in the centre (as I think it
should do).

We should say:

  if (strlen (prompt) / 2 >= half_width)
    x_pos = 1;
  else
    x_pos = half_width - strlen (prompt) / 2;

I only mention this because I noticed it.  We use the same incorrect
algorithm elsewhere in the TUI code, so I think this needs fixing in a
separate series after this work is merged.

Thanks,
Andrew


> +  display_string (height / 2, x_pos, prompt);
>  }
>  
>  /* See tui-regs.h.  */
> @@ -370,7 +368,7 @@ void
>  tui_data_window::rerender ()
>  {
>    if (m_regs_content.empty ())
> -    erase_data_content (_("[ Register Values Unavailable ]"));
> +    erase_data_content ();
>    else
>      display_registers_from (0);
>    tui_wrefresh (handle.get ());
> diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
> index 7f1c30ca5d6..880f360bec6 100644
> --- a/gdb/tui/tui-regs.h
> +++ b/gdb/tui/tui-regs.h
> @@ -130,7 +130,7 @@ struct tui_data_window : public tui_win_info
>       past the register area (-1) is returned.  */
>    int first_reg_element_no_inline (int line_no) const;
>  
> -  void erase_data_content (const char *prompt);
> +  void erase_data_content ();
>  
>    /* Information about each register in the current register group.  */
>    std::vector<tui_register_info> m_regs_content;
>
> -- 
> 2.43.0

Re: [PATCH v2 00/14] Cleanups for the TUi register window

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

> This series is a grab-bag of cleanups to the TUI register window.
>
> I've tried to untangle the code somewhat, with the goal being removing
> the extra rerender overload.
>
> This series also fixes the 'exit' bug and removes the hacky and
> unnecessary recursion flag from tui-hooks.c.
>
> More cleanups here are possible -- the register window is pretty old
> code and pretty ugly.  In particular:
>
> * the layout code is still pretty bad
>
> * it doesn't really make sense for check_register_values to accept a
>   frame, because the window can only really ever use the selected
>   frame anyway
>
> * there's no horizontal scrolling, but for vector registers this might
>   be nice (there's a bug about this)

This all looks great.  Spotted one typo in a commit message (see
separate email), but otherwise:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>
> ---
> Changes in v2:
> - Addressed review comments
> - Link to v1: https://inbox.sourceware.org/gdb-patches/20231217-tui-regs-cleanup-v1-0-67bd0ea1e8be@tromey.com
>
> ---
> Tom Tromey (14):
>       Use pop_back in tui_register_format
>       Minor C++ cleanups in tui-regs.c
>       Simplify tui_data_window::show_register_group
>       Rename tui_data_item_window -> tui_register_info
>       Change tui_register_info::visible to a method
>       Move scrollok call in register window
>       Simplify update_register_data
>       Remove the TUI register window rerender overload
>       Simplify tui_data_win::erase_data_content
>       Remove tui_refreshing_registers
>       Remove redundant check from tui_refresh_frame_and_register_information
>       Return void from tui_show_frame_info
>       Rename show_registers -> set_register_group
>       Update TUI register window when the inferior exits
>
>  gdb/testsuite/gdb.tui/regs.exp |   8 ++
>  gdb/tui/tui-hooks.c            |  36 +++----
>  gdb/tui/tui-regs.c             | 229 +++++++++++++++--------------------------
>  gdb/tui/tui-regs.h             |  59 ++++++-----
>  gdb/tui/tui-status.c           |  16 ++-
>  gdb/tui/tui-status.h           |   2 +-
>  6 files changed, 148 insertions(+), 202 deletions(-)
> ---
> base-commit: 7ae24327467750c445733e40d840e502795dbdf3
> change-id: 20231217-tui-regs-cleanup-36d8f390a65a
>
> Best regards,
> -- 
> Tom Tromey <tom@tromey.com>

Re: [PATCH v2 09/14] Simplify tui_data_win::erase_data_content

[Tom Tromey]

Andrew> I only mention this because I noticed it.  We use the same incorrect
Andrew> algorithm elsewhere in the TUI code, so I think this needs fixing in a
Andrew> separate series after this work is merged.

I filed this to not forget:

https://sourceware.org/bugzilla/show_bug.cgi?id=31355

Tom