[1/3,gdb/tui] Keep inferior output in cmd window with ^L

Message ID 20230530105324.23089-2-tdevries@suse.de
State New
Headers
Series Improve handling of inferior output |

Commit Message

Tom de Vries May 30, 2023, 10:53 a.m. UTC
  Consider a hello world compiled with -g into an a.out:
...
int main (void) {
  printf ("hello\n");
  return 0;
}
...

After starting TUI like this:
...
$ gdb -q a.out -ex start -ex "tui enable"
...
we do "echo \n" and type enter until the prompt is at the bottom of the screen.

After doing next to execute the printf, we have:
...
(gdb) next
hello
(gdb)
...
but the entire screen scrolled, so:
- the src window is missing the top border, and
- the updates in the src and status window are misaligned with the current
  screen.

We can fix this by doing ^L, but that removes the "hello", giving us just:
...
(gdb) next
(gdb)
...

The ^L key-combo (and likewise the command refresh) calls tui_refresh_all_win,
which works by:
- clearing the screen, and then
- refreshing all visible windows.
Since curses has no notion of the inferior output, this overwrites the
"hello".

Fix this by excluding the command window from ^L.

Tested on x86_64-linux.
---
 gdb/testsuite/gdb.tui/hello.c   | 25 +++++++++++++++
 gdb/testsuite/gdb.tui/hello.exp | 57 +++++++++++++++++++++++++++++++++
 gdb/tui/tui-io.c                |  2 +-
 gdb/tui/tui-win.c               | 18 ++++++++---
 gdb/tui/tui-win.h               |  2 +-
 gdb/tui/tui-wingeneral.c        |  5 ++-
 gdb/tui/tui-wingeneral.h        |  2 +-
 7 files changed, 103 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.tui/hello.c
 create mode 100644 gdb/testsuite/gdb.tui/hello.exp
  

Comments

Guinevere Larsen May 31, 2023, 1:35 p.m. UTC | #1
On 30/05/2023 12:53, Tom de Vries via Gdb-patches wrote:
> Consider a hello world compiled with -g into an a.out:
> ...
> int main (void) {
>    printf ("hello\n");
>    return 0;
> }
> ...
>
> After starting TUI like this:
> ...
> $ gdb -q a.out -ex start -ex "tui enable"
> ...
> we do "echo \n" and type enter until the prompt is at the bottom of the screen.
>
> After doing next to execute the printf, we have:
> ...
> (gdb) next
> hello
> (gdb)
> ...
> but the entire screen scrolled, so:
> - the src window is missing the top border, and
> - the updates in the src and status window are misaligned with the current
>    screen.
>
> We can fix this by doing ^L, but that removes the "hello", giving us just:
> ...
> (gdb) next
> (gdb)
> ...
>
> The ^L key-combo (and likewise the command refresh) calls tui_refresh_all_win,
> which works by:
> - clearing the screen, and then
> - refreshing all visible windows.
> Since curses has no notion of the inferior output, this overwrites the
> "hello".
>
> Fix this by excluding the command window from ^L.
>
> Tested on x86_64-linux.
> ---

Thank you for working on this, this has bugged me for years at this point.

I can confirm that this fixes the issue you mentioned when the terminal 
scrolls. The problem is that now, if the output doesn't cause the screen 
to scroll, it doesn't show up on screen at all.

I also am not a great fan of the test name, I would suggest calling it 
gdb.tui/inferior-stdout-output.exp or something like that, hello sounds 
a little too generic IMO.
  
Tom de Vries May 31, 2023, 2:19 p.m. UTC | #2
On 5/31/23 15:35, Bruno Larsen wrote:
> I can confirm that this fixes the issue you mentioned when the terminal 
> scrolls. The problem is that now, if the output doesn't cause the screen 
> to scroll, it doesn't show up on screen at all.

Is it possible that you're talking about a pre-existing issue?

That is, we have:
...
(gdb) next
...
and after <enter> we have very, very briefly:
...
(gdb) next<enter>
hello
...
before the prompt overwrites it:
...
(gdb) next<enter>
(gdb)
...

I get this behaviour with and without the patch series.  AFAIU, the only 
way to deal with this (that doesn't go all the way into introducing 
pseudo-terminals) is by introducing a separate cmd and output window in 
TUI.  Alternatively, we can move the prompt to the bottom of the command 
window, I've spent a day or so trying to make that work, but abandoned that.

If this is not the behaviour you're talking about, please describe a way 
of reproducing what you observe.

Anyway, another way of showing the effect of the patch series in the 
no-scrolling-case is to add an extra hello to the test-case, and do 
"next 2".

Without the patch series we have:
...
(gdb) next 2
(gdb)
hello
...
and after ^L just:
...
(gdb) next 2
(gdb)
...

With the patch series the same:
...
(gdb) n 2
(gdb)
hello
...
and the same after ^L.

Thanks,
- Tom
  
Guinevere Larsen May 31, 2023, 2:27 p.m. UTC | #3
On 31/05/2023 16:19, Tom de Vries wrote:
> On 5/31/23 15:35, Bruno Larsen wrote:
>> I can confirm that this fixes the issue you mentioned when the 
>> terminal scrolls. The problem is that now, if the output doesn't 
>> cause the screen to scroll, it doesn't show up on screen at all.
>
> Is it possible that you're talking about a pre-existing issue?
>
> That is, we have:
> ...
> (gdb) next
> ...
> and after <enter> we have very, very briefly:
> ...
> (gdb) next<enter>
> hello
> ...
> before the prompt overwrites it:
> ...
> (gdb) next<enter>
> (gdb)
> ...
>
> I get this behaviour with and without the patch series.  AFAIU, the 
> only way to deal with this (that doesn't go all the way into 
> introducing pseudo-terminals) is by introducing a separate cmd and 
> output window in TUI.  Alternatively, we can move the prompt to the 
> bottom of the command window, I've spent a day or so trying to make 
> that work, but abandoned that.
>
> If this is not the behaviour you're talking about, please describe a 
> way of reproducing what you observe.

Huh... I think I remember this not happening last time I tried using the 
TUI, which would have been almost 2 years ago. I wonder if I am 
misremembering, if this use case never came up or if it is an actual 
regression... Either way, since it wasn't introduced by this series, I'm 
fine with it.

Reviewed-By: Bruno Larsen <blarsen@redhat.com>
  
Tom de Vries May 31, 2023, 3:24 p.m. UTC | #4
On 5/31/23 16:27, Bruno Larsen wrote:
> On 31/05/2023 16:19, Tom de Vries wrote:
>> On 5/31/23 15:35, Bruno Larsen wrote:
>>> I can confirm that this fixes the issue you mentioned when the 
>>> terminal scrolls. The problem is that now, if the output doesn't 
>>> cause the screen to scroll, it doesn't show up on screen at all.
>>
>> Is it possible that you're talking about a pre-existing issue?
>>
>> That is, we have:
>> ...
>> (gdb) next
>> ...
>> and after <enter> we have very, very briefly:
>> ...
>> (gdb) next<enter>
>> hello
>> ...
>> before the prompt overwrites it:
>> ...
>> (gdb) next<enter>
>> (gdb)
>> ...
>>
>> I get this behaviour with and without the patch series.  AFAIU, the 
>> only way to deal with this (that doesn't go all the way into 
>> introducing pseudo-terminals) is by introducing a separate cmd and 
>> output window in TUI.  Alternatively, we can move the prompt to the 
>> bottom of the command window, I've spent a day or so trying to make 
>> that work, but abandoned that.
>>
>> If this is not the behaviour you're talking about, please describe a 
>> way of reproducing what you observe.
> 
> Huh... I think I remember this not happening last time I tried using the 
> TUI, which would have been almost 2 years ago. I wonder if I am 
> misremembering, if this use case never came up or if it is an actual 
> regression... 
Reproduced with (using in-repo readline instead of system readline, to 
eliminate a constant system readline as source of non-regression):
- gdb-13-branch
- gdb-12-branch
- gdb-11-branch
- gdb-10-branch
- gdb-9-branch
- gdb-8.3-branch
- gdb-8.2-branch
- gdb-8.1-branch (*)
- gdb-8.0-branch (*) (**)

So, if there is a regression, we're not talking about a recent one 
(8.0.1 released 2017-09-05).

Thanks,
- Tom

(*) Using backport of commit 5a6c3296a7a ("gdb: Fix ia64 defining 
TRAP_HWBKPT before including gdb_wait.h") to fix build.
(**) Without libipt to fix build.
  
Guinevere Larsen May 31, 2023, 3:27 p.m. UTC | #5
On 31/05/2023 17:24, Tom de Vries wrote:
> On 5/31/23 16:27, Bruno Larsen wrote:
>> On 31/05/2023 16:19, Tom de Vries wrote:
>>> On 5/31/23 15:35, Bruno Larsen wrote:
>>>> I can confirm that this fixes the issue you mentioned when the 
>>>> terminal scrolls. The problem is that now, if the output doesn't 
>>>> cause the screen to scroll, it doesn't show up on screen at all.
>>>
>>> Is it possible that you're talking about a pre-existing issue?
>>>
>>> That is, we have:
>>> ...
>>> (gdb) next
>>> ...
>>> and after <enter> we have very, very briefly:
>>> ...
>>> (gdb) next<enter>
>>> hello
>>> ...
>>> before the prompt overwrites it:
>>> ...
>>> (gdb) next<enter>
>>> (gdb)
>>> ...
>>>
>>> I get this behaviour with and without the patch series. AFAIU, the 
>>> only way to deal with this (that doesn't go all the way into 
>>> introducing pseudo-terminals) is by introducing a separate cmd and 
>>> output window in TUI.  Alternatively, we can move the prompt to the 
>>> bottom of the command window, I've spent a day or so trying to make 
>>> that work, but abandoned that.
>>>
>>> If this is not the behaviour you're talking about, please describe a 
>>> way of reproducing what you observe.
>>
>> Huh... I think I remember this not happening last time I tried using 
>> the TUI, which would have been almost 2 years ago. I wonder if I am 
>> misremembering, if this use case never came up or if it is an actual 
>> regression... 
> Reproduced with (using in-repo readline instead of system readline, to 
> eliminate a constant system readline as source of non-regression):
> - gdb-13-branch
> - gdb-12-branch
> - gdb-11-branch
> - gdb-10-branch
> - gdb-9-branch
> - gdb-8.3-branch
> - gdb-8.2-branch
> - gdb-8.1-branch (*)
> - gdb-8.0-branch (*) (**)
>
> So, if there is a regression, we're not talking about a recent one 
> (8.0.1 released 2017-09-05).
>
> Thanks,
> - Tom
>
> (*) Using backport of commit 5a6c3296a7a ("gdb: Fix ia64 defining 
> TRAP_HWBKPT before including gdb_wait.h") to fix build.
> (**) Without libipt to fix build.
>
Thanks for checking so far! it would definitely not have been earlier 
than 2017, so I most likely am just misremembering.
  
Tom de Vries May 31, 2023, 11:37 p.m. UTC | #6
On 5/31/23 16:19, Tom de Vries wrote:
> That is, we have:
> ...
> (gdb) next
> ...
> and after <enter> we have very, very briefly:
> ...
> (gdb) next<enter>
> hello
> ...
> before the prompt overwrites it:
> ...
> (gdb) next<enter>
> (gdb)
> ...
> 
> I get this behaviour with and without the patch series.  AFAIU, the only 
> way to deal with this (that doesn't go all the way into introducing 
> pseudo-terminals) is by introducing a separate cmd and output window in 
> TUI.  

I gave the output window idea a try, WIP attached at 
https://sourceware.org/bugzilla/show_bug.cgi?id=30502 .

Thanks,
- Tom
  

Patch

diff --git a/gdb/testsuite/gdb.tui/hello.c b/gdb/testsuite/gdb.tui/hello.c
new file mode 100644
index 00000000000..c3f5c6ea2cb
--- /dev/null
+++ b/gdb/testsuite/gdb.tui/hello.c
@@ -0,0 +1,25 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+
+int
+main (void)
+{
+  printf ("hello\n");
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.tui/hello.exp b/gdb/testsuite/gdb.tui/hello.exp
new file mode 100644
index 00000000000..9634d9d8059
--- /dev/null
+++ b/gdb/testsuite/gdb.tui/hello.exp
@@ -0,0 +1,57 @@ 
+# Copyright 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Check that a print by an inferior doesn't mess up the screen.
+
+tuiterm_env
+
+standard_testfile hello.c
+
+if {[build_executable "failed to prepare" ${testfile} ${srcfile}] == -1} {
+    return -1
+}
+
+Term::clean_restart 24 80 $testfile
+
+if {![runto_main]} {
+    perror "test suppressed"
+    return
+}
+
+if {![Term::enter_tui]} {
+    unsupported "TUI not supported"
+    return
+}
+
+# Make sure the prompt is at the last line.
+Term::command "echo \\n"
+Term::command "echo \\n"
+Term::command "echo \\n"
+Term::command "echo \\n"
+
+# Check the source box.
+Term::check_box "before next: source box" 0 0 80 15
+
+# Make the inferior print "hello".
+Term::command "next"
+
+# Refresh to redraw the screen.
+Term::command "refresh"
+
+# Check the source box again.
+Term::check_box "after next: source box" 0 0 80 15
+
+# Check that we can still see the printed "hello".
+Term::check_contents "hello" "\nhello"
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index a1eadcd937d..6ae7948b786 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -1219,7 +1219,7 @@  tui_getc_1 (FILE *fp)
   /* Handle the CTRL-L refresh for each window.  */
   if (ch == '\f')
     {
-      tui_refresh_all_win ();
+      tui_refresh_all_win (false);
       return ch;
     }
 
diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
index 7abd1e225b9..3b41b59cdd3 100644
--- a/gdb/tui/tui-win.c
+++ b/gdb/tui/tui-win.c
@@ -508,10 +508,20 @@  tui_win_info::right_scroll (int num_to_scroll)
 
 
 void
-tui_refresh_all_win (void)
+tui_refresh_all_win (bool cmd_window)
 {
-  clearok (curscr, TRUE);
-  tui_refresh_all ();
+  if (cmd_window)
+    clearok (curscr, TRUE);
+  else
+    for (tui_win_info *wi : all_tui_windows ())
+      {
+	if (wi == TUI_CMD_WIN)
+	  continue;
+
+	redrawwin (wi->handle.get ());
+      }
+
+  tui_refresh_all (cmd_window);
 }
 
 void
@@ -827,7 +837,7 @@  tui_refresh_all_command (const char *arg, int from_tty)
   /* Make sure the curses mode is enabled.  */
   tui_enable ();
 
-  tui_refresh_all_win ();
+  tui_refresh_all_win (false);
 }
 
 #define DEFAULT_TAB_LEN         8
diff --git a/gdb/tui/tui-win.h b/gdb/tui/tui-win.h
index 3d35f1dfb7f..41408a0af3d 100644
--- a/gdb/tui/tui-win.h
+++ b/gdb/tui/tui-win.h
@@ -26,7 +26,7 @@ 
 
 extern void tui_set_win_focus_to (struct tui_win_info *);
 extern void tui_resize_all (void);
-extern void tui_refresh_all_win (void);
+extern void tui_refresh_all_win (bool cmd_win = true);
 extern void tui_rehighlight_all (void);
 
 extern chtype tui_border_ulcorner;
diff --git a/gdb/tui/tui-wingeneral.c b/gdb/tui/tui-wingeneral.c
index 82a023d09fe..73c047a8ac4 100644
--- a/gdb/tui/tui-wingeneral.c
+++ b/gdb/tui/tui-wingeneral.c
@@ -198,10 +198,13 @@  tui_win_info::make_visible (bool visible)
 /* Function to refresh all the windows currently displayed.  */
 
 void
-tui_refresh_all ()
+tui_refresh_all (bool cmd_window)
 {
   for (tui_win_info *win_info : all_tui_windows ())
     {
+      if (!cmd_window && win_info == tui_win_list[CMD_WIN])
+	continue;
+
       if (win_info->is_visible ())
 	win_info->refresh_window ();
     }
diff --git a/gdb/tui/tui-wingeneral.h b/gdb/tui/tui-wingeneral.h
index 5b82fcd6b21..051dbf3b5ca 100644
--- a/gdb/tui/tui-wingeneral.h
+++ b/gdb/tui/tui-wingeneral.h
@@ -28,7 +28,7 @@  struct tui_win_info;
 
 extern void tui_unhighlight_win (struct tui_win_info *);
 extern void tui_highlight_win (struct tui_win_info *);
-extern void tui_refresh_all ();
+extern void tui_refresh_all (bool cmd_window = true);
 
 /* An RAII class that suppresses output on construction (calling
    wnoutrefresh on the existing windows), and then flushes the output