diff mbox series

[gdb/tui] Assert on tui_refreshing_registers recursion

Message ID 20231209082453.21019-1-tdevries@suse.de
State New
Headers
Series [gdb/tui] Assert on tui_refreshing_registers recursion |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Tom de Vries Dec. 9, 2023, 8:24 a.m. UTC 
  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(-)


base-commit: 95385060771b0cac95a39320c44eca857fb177ae

Comments

Tom Tromey Dec. 9, 2023, 4:06 p.m. UTC | #1 
>>>>> "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
diff mbox series

Patch

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)
     {