[PATCH v2] [gdb/tui] Show regs when switching to regs layout

[PATCH v2] [gdb/tui] Show regs when switching to regs layout

[Tom de Vries]

When starting gdb in CLI mode, running to main and switching into the TUI regs
layout:
...
$ gdb -q a.out -ex start -ex "layout regs"
...
we get:
...
+---------------------------------+
|                                 |
| [ Register Values Unavailable ] |
|                                 |
+---------------------------------+
...

Fix this by handling this case in tui_data_window::rerender.

Tested on x86_64-linux.

PR tui/28600
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28600
---
 gdb/testsuite/gdb.tui/regs.exp | 10 ++++++----
 gdb/tui/tui-regs.c             | 17 ++++++++++++++++-
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/gdb.tui/regs.exp b/gdb/testsuite/gdb.tui/regs.exp
index 520f6ddba96..0be99625b9f 100644
--- a/gdb/testsuite/gdb.tui/regs.exp
+++ b/gdb/testsuite/gdb.tui/regs.exp
@@ -41,10 +41,12 @@ Term::command "layout regs"
 Term::check_box "register box" 0 0 80 8
 Term::check_box "source box in regs layout" 0 7 80 8
 
-set text [Term::get_line 1]
-# Just check for any register window content at all.
-Term::check_contents "any register contents" "\\|.*\[^ \].*\\|"
-
+# The current frame is main, check that registers are available.
+set re_reg_vals_unavailable \
+    [string_to_regexp {[ Register Values Unavailable ]}]
+gdb_assert \
+    { ![Term::check_region_contents_p 0 0 80 8 $re_reg_vals_unavailable] } \
+    "Register values available"
 
 # Check that we can successfully cause the register window to appear
 # using the 'tui reg next' and 'tui reg prev' commands.
diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 01538d49961..fe9cd5ac722 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -417,14 +417,29 @@ tui_data_window::erase_data_content (const char *prompt)
 void
 tui_data_window::rerender ()
 {
+  /* Calling check_register_value calls rerender again.  We use this counter
+     to prevent enless recursion.  */
+  static int in_rerender;
+  in_rerender++;
+
   if (m_regs_content.empty ())
-    erase_data_content (_("[ Register Values Unavailable ]"));
+    {
+      if (has_stack_frames () && in_rerender == 1)
+	{
+	  frame_info_ptr fi = get_selected_frame (NULL);
+	  check_register_values (fi);
+	}
+      else
+	erase_data_content (_("[ Register Values Unavailable ]"));
+    }
   else
     {
       erase_data_content (NULL);
       delete_data_content_windows ();
       display_registers_from (0);
     }
+
+  in_rerender--;
 }
 
 

base-commit: 476bf7d5e6661b06eb9f8de9258cf48fd81919af
-- 
2.35.3

Re: [PATCH v2] [gdb/tui] Show regs when switching to regs layout

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

On Sun, Nov 26, 2023 at 8:52 AM Tom de Vries <tdevries@suse.de> wrote:

> When starting gdb in CLI mode, running to main and switching into the TUI
> regs
> layout:
> ...
> $ gdb -q a.out -ex start -ex "layout regs"
> ...
> we get:
> ...
> +---------------------------------+
> |                                 |
> | [ Register Values Unavailable ] |
> |                                 |
> +---------------------------------+
> ...
>
> Fix this by handling this case in tui_data_window::rerender.
>
> Tested on x86_64-linux.
>
> PR tui/28600
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28600
> ---
>
I can confirm I can see this issue on ppc64le Fedora Rawhide and this patch
> fixes this. I'm able to see the Register group now
>
when running  "gdb/gdb -q /bin/ls -ex start -ex "layout regs"". I can also
confirm this change does not introduce any regressions.

Re: [PATCH v2] [gdb/tui] Show regs when switching to regs layout

[Tom Tromey]

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

Tom>  void
Tom>  tui_data_window::rerender ()
Tom>  {
Tom> +  /* Calling check_register_value calls rerender again.  We use this counter
Tom> +     to prevent enless recursion.  */
Tom> +  static int in_rerender;

There's a typo, the function is check_register_values (missing an "s" in
the comment).

However, it seems bad to need new state for this.

This triggered a memory -- there's already weird state in this area, see
tui-hooks.c:tui_refreshing_registers.  I have never understood why that
is needed.

I wonder if this can be untangled.

Tom

Re: [PATCH v2] [gdb/tui] Show regs when switching to regs layout

[Tom de Vries]

On 12/8/23 16:29, Tom Tromey wrote:
> This triggered a memory -- there's already weird state in this area, see
> tui-hooks.c:tui_refreshing_registers.  I have never understood why that
> is needed.
> 
> I wonder if this can be untangled.

I don't know why it's needed either, and I didn't manage to trigger 
usage of it either.

I've submitted a patch ( 
https://sourceware.org/pipermail/gdb-patches/2023-December/204933.html ) 
that changes behaviour from recursion prevention to asserting on 
recursion detection.

Thanks,
- Tom

Re: [PATCH v2] [gdb/tui] Show regs when switching to regs layout

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 827 bytes --]

On 12/8/23 16:29, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom>  void
> Tom>  tui_data_window::rerender ()
> Tom>  {
> Tom> +  /* Calling check_register_value calls rerender again.  We use this counter
> Tom> +     to prevent enless recursion.  */
> Tom> +  static int in_rerender;
> 
> There's a typo, the function is check_register_values (missing an "s" in
> the comment).
> 
> However, it seems bad to need new state for this.
> 

How about this approach?

Rather than adding global state, it adds a function parameter.

It's currently called toplevel, though I've also consider "active".

Another solution is to split out the functionality between two 
functions, I didn't pursue this because I couldn't think of good names 
(I came up with rerender and rerender_1).

Thanks,
- Tom



[-- Attachment #2: v2-0001-gdb-tui-Show-regs-when-switching-to-regs-layout.patch --]
[-- Type: text/x-patch, Size: 3084 bytes --]

From 9590364f9dffc0fbb29f00690bae3df8b2c92cf3 Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Sun, 26 Nov 2023 08:49:28 +0100
Subject: [PATCH v2] [gdb/tui] Show regs when switching to regs layout

When starting gdb in CLI mode, running to main and switching into the TUI regs
layout:
...
$ gdb -q a.out -ex start -ex "layout regs"
...
we get:
...
+---------------------------------+
|                                 |
| [ Register Values Unavailable ] |
|                                 |
+---------------------------------+
...

Fix this by handling this case in tui_data_window::rerender.

Tested on x86_64-linux.

PR tui/28600
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28600
---
 gdb/testsuite/gdb.tui/regs.exp | 10 ++++++----
 gdb/tui/tui-regs.c             | 14 +++++++++++---
 gdb/tui/tui-regs.h             |  6 +++++-
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/gdb/testsuite/gdb.tui/regs.exp b/gdb/testsuite/gdb.tui/regs.exp
index 520f6ddba96..0be99625b9f 100644
--- a/gdb/testsuite/gdb.tui/regs.exp
+++ b/gdb/testsuite/gdb.tui/regs.exp
@@ -41,10 +41,12 @@ Term::command "layout regs"
 Term::check_box "register box" 0 0 80 8
 Term::check_box "source box in regs layout" 0 7 80 8
 
-set text [Term::get_line 1]
-# Just check for any register window content at all.
-Term::check_contents "any register contents" "\\|.*\[^ \].*\\|"
-
+# The current frame is main, check that registers are available.
+set re_reg_vals_unavailable \
+    [string_to_regexp {[ Register Values Unavailable ]}]
+gdb_assert \
+    { ![Term::check_region_contents_p 0 0 80 8 $re_reg_vals_unavailable] } \
+    "Register values available"
 
 # Check that we can successfully cause the register window to appear
 # using the 'tui reg next' and 'tui reg prev' commands.
diff --git a/gdb/tui/tui-regs.c b/gdb/tui/tui-regs.c
index 4c6ea8aff0d..da2c9a26995 100644
--- a/gdb/tui/tui-regs.c
+++ b/gdb/tui/tui-regs.c
@@ -192,7 +192,7 @@ tui_data_window::show_registers (const reggroup *group)
       m_regs_content.clear ();
     }
 
-  rerender ();
+  rerender (false);
 }
 
 
@@ -415,10 +415,18 @@ tui_data_window::erase_data_content (const char *prompt)
 /* See tui-regs.h.  */
 
 void
-tui_data_window::rerender ()
+tui_data_window::rerender (bool toplevel)
 {
   if (m_regs_content.empty ())
-    erase_data_content (_("[ Register Values Unavailable ]"));
+    {
+      if (toplevel && has_stack_frames ())
+	{
+	  frame_info_ptr fi = get_selected_frame (NULL);
+	  check_register_values (fi);
+	}
+      else
+	erase_data_content (_("[ Register Values Unavailable ]"));
+    }
   else
     {
       erase_data_content (NULL);
diff --git a/gdb/tui/tui-regs.h b/gdb/tui/tui-regs.h
index 5adff6300aa..1abd22cd382 100644
--- a/gdb/tui/tui-regs.h
+++ b/gdb/tui/tui-regs.h
@@ -75,7 +75,11 @@ struct tui_data_window : public tui_win_info
   {
   }
 
-  void rerender () override;
+  void rerender (bool toplevel);
+  void rerender () override
+  {
+    rerender (true);
+  }
 
 private:
 

base-commit: 1340072a5412877d9d98f5b010cd8e6e9e418905
-- 
2.35.3

Re: [PATCH v2] [gdb/tui] Show regs when switching to regs layout

[Tom Tromey]

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

Tom> How about this approach?

Tom> Rather than adding global state, it adds a function parameter.

Tom> It's currently called toplevel, though I've also consider "active".

This is fine by me.

Really I suspect the register window refresh code can be untangled and
not need any of this, but it's fine to just fix the bug and move on.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH v2] [gdb/tui] Show regs when switching to regs layout

[Tom de Vries]

On 12/16/23 02:40, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> How about this approach?
> 
> Tom> Rather than adding global state, it adds a function parameter.
> 
> Tom> It's currently called toplevel, though I've also consider "active".
> 
> This is fine by me.
> 
> Really I suspect the register window refresh code can be untangled and
> not need any of this, but it's fine to just fix the bug and move on.
> 

I suspect that as well, but unfortunately I don't understand how yet.

> Approved-By: Tom Tromey <tom@tromey.com>

Thanks for the review, pushed.

- Tom

Re: [PATCH v2] [gdb/tui] Show regs when switching to regs layout

[Tom Tromey]

Tom> I suspect that as well, but unfortunately I don't understand how yet.

I have some patches I'll send to clean this up.  It's still a bit ugly
at the end but it at least gets rid of the "two rerender methods"
problem.  (Specifically, the layout code is over-complicated; this is
pretty old stuff...)

I also found a generic window refresh problem that I'm still
investigating.  It seems like at some point I tried to tackle this but I
fundamentally misunderstood wnoutrefresh.

Tom