[PATCH] [gdb/tui] Assert on tui_refreshing_registers recursion

[PATCH] [gdb/tui] Assert on tui_refreshing_registers recursion

[Tom de Vries]

In gdb/tui/tui-hooks.c, we have a variable:
...
/* Prevent recursion of deprecated_register_changed_hook().  */
static bool tui_refreshing_registers = false;
...

I tried to detect the moment it's preventing recursion, both by running the
TUI testsuite and test-driving TUI, but didn't manage.

Change the behaviour from preventing recursion to asserting on detecting
recursion.

Also as a cleanup, fold usage of the variable into a single function.

Tested on x86_64-linux.
---
 gdb/tui/tui-hooks.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index 6525f0f2b6c..fa9fd443db2 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -63,8 +63,28 @@ 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;
+/* As TUI_DATA_WIN->check_register_values, but with recursion detection.  */
+
+static void
+check_register_values (frame_info_ptr fi)
+{
+  static bool tui_refreshing_registers = false;
+
+  if (tui_refreshing_registers)
+    {
+      /* Recursion detected.  We used to stop recursion here, but we
+	 think it no longer occurs, so instead we detect it verbosely.
+	 If we don't manage to trigger it for a while, we can just remove the
+	 detection.  */
+      gdb_assert_not_reached ("recursion detected in check_register_values");
+    }
+  else
+    {
+      tui_refreshing_registers = true;
+      TUI_DATA_WIN->check_register_values (fi);
+      tui_refreshing_registers = false;
+    }
+}
 
 /* Observer for the register_changed notification.  */
 
@@ -82,12 +102,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;
-    }
+  check_register_values (fi);
 }
 
 /* Breakpoint creation hook.
@@ -145,11 +160,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;
-	}
+	check_register_values (fi);
     }
   else if (!from_stack)
     {

base-commit: 95385060771b0cac95a39320c44eca857fb177ae
-- 
2.35.3

Re: [PATCH] [gdb/tui] Assert on tui_refreshing_registers recursion

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Change the behaviour from preventing recursion to asserting on detecting
Tom> recursion.

If there is a bug here, it will be inflicted on users in the form of a
crash -- so I tend to think that it is better to just leave some ugly
code in there.  Probably the real way to convince ourselves it is ok to
remove is following the code paths by hand, but of course in the TUI
that's a pain.

Tom