diff mbox series

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

Message ID 20231126075250.31557-1-tdevries@suse.de
State New
Headers
Series [v2,gdb/tui] Show regs when switching to regs layout |

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 Nov. 26, 2023, 7:52 a.m. UTC 
  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(-)


base-commit: 476bf7d5e6661b06eb9f8de9258cf48fd81919af

Comments

Alexandra Petlanova Hajkova Nov. 27, 2023, 12:27 p.m. UTC | #1 
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.
Tom Tromey Dec. 8, 2023, 3:29 p.m. UTC | #2 
>>>>> "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
Tom de Vries Dec. 9, 2023, 8:30 a.m. UTC | #3 
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
Tom de Vries Dec. 9, 2023, 10:01 a.m. UTC | #4 
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
Tom Tromey Dec. 16, 2023, 1:40 a.m. UTC | #5 
>>>>> "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
Tom de Vries Dec. 16, 2023, 8:34 a.m. UTC | #6 
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
Tom Tromey Dec. 17, 2023, 5:31 p.m. UTC | #7 
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
diff mbox series

Patch

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--;
 }