[PATCH 0/3] [gdb/tui] Improve handling of inferior output

[PATCH 0/3] [gdb/tui] Improve handling of inferior output

[Tom de Vries]

When inferior output in TUI causes scrolling, the TUI screen is garbled.  This
is a known issue, PR tui/14332.

This can be fixed manually by doing ^L.

I noticed though that doing ^L removes the inferior output.

The first patch fixes this.

The third patch automates doing ^L when the gdb_tty_state changes to
target_terminal_state::is_ours, such what we no longer have to do ^L.

The second patch is an infrastructure patch used by the third patch.

Tested on x86_64-linux.

Tom de Vries (3):
  [gdb/tui] Keep inferior output in cmd window with ^L
  [gdb] Add observable terminal_owner_changed
  [gdb/tui] Refresh on target_terminal_state::is_ours

 gdb/inflow.c                    | 176 ++++++++++++++++++--------------
 gdb/observable.c                |   1 +
 gdb/observable.h                |   5 +
 gdb/testsuite/gdb.tui/hello.c   |  25 +++++
 gdb/testsuite/gdb.tui/hello.exp |  54 ++++++++++
 gdb/tui/tui-hooks.c             |  11 ++
 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 +-
 11 files changed, 214 insertions(+), 87 deletions(-)
 create mode 100644 gdb/testsuite/gdb.tui/hello.c
 create mode 100644 gdb/testsuite/gdb.tui/hello.exp


base-commit: 796029320e75a141570220224731c8151311f8d9
-- 
2.35.3

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

[Tom de Vries]

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

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
-- 
2.35.3

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

[Bruno Larsen]

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.

-- 
Cheers,
Bruno

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

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

[Tom de Vries]

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

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

[Bruno Larsen]

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>

-- 
Cheers,
Bruno

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

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

[Tom de Vries]

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.

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

[Bruno Larsen]

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.

-- 
Cheers,
Bruno

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

[Tom de Vries]

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 2/3] [gdb] Add observable terminal_owner_changed

[Tom de Vries]

Add a new observable, gdb::observers::terminal_owner_changed.

Its observers are notified when gdb_tty_state changes.

On each change, they are notified twice:
- once with the old gdb_tty_state, and active == false, and
- once with the new gdb_tty_state, and active == true.

No functional changes.

Tested on x86_64-linux.
---
 gdb/inflow.c     | 176 ++++++++++++++++++++++++++---------------------
 gdb/observable.c |   1 +
 gdb/observable.h |   5 ++
 3 files changed, 103 insertions(+), 79 deletions(-)

diff --git a/gdb/inflow.c b/gdb/inflow.c
index 767cfd02c48..9c82306be62 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -311,67 +311,76 @@ child_terminal_inferior (struct target_ops *self)
   inferior *inf = current_inferior ();
   terminal_info *tinfo = get_inflow_inferior_data (inf);
 
-  if (gdb_has_a_terminal ()
-      && tinfo->ttystate != NULL
-      && sharing_input_terminal (inf))
-    {
-      int result;
+  if (!(gdb_has_a_terminal ()
+	&& tinfo->ttystate != NULL
+	&& sharing_input_terminal (inf)))
+    return;
 
-      /* Ignore SIGTTOU since it will happen when we try to set the
-	 terminal's state (if gdb_tty_state is currently
-	 ours_for_output).  */
-      scoped_ignore_sigttou ignore_sigttou;
+  /* Note that the old state will no longer be active.   */
+  gdb::observers::terminal_owner_changed.notify (gdb_tty_state, false);
+
+  /* Scope containing scoped_ignore_sigtto.  */
+  {
+    int result;
+
+    /* Ignore SIGTTOU since it will happen when we try to set the
+       terminal's state (if gdb_tty_state is currently
+       ours_for_output).  */
+    scoped_ignore_sigttou ignore_sigttou;
 
 #ifdef F_GETFL
-      result = fcntl (0, F_SETFL, tinfo->tflags);
-      OOPSY ("fcntl F_SETFL");
+    result = fcntl (0, F_SETFL, tinfo->tflags);
+    OOPSY ("fcntl F_SETFL");
 #endif
 
-      result = serial_set_tty_state (stdin_serial, tinfo->ttystate);
-      OOPSY ("setting tty state");
+    result = serial_set_tty_state (stdin_serial, tinfo->ttystate);
+    OOPSY ("setting tty state");
 
-      if (!job_control)
-	{
-	  sigint_ours = install_sigint_handler (SIG_IGN);
+    if (!job_control)
+      {
+	sigint_ours = install_sigint_handler (SIG_IGN);
 #ifdef SIGQUIT
-	  sigquit_ours = signal (SIGQUIT, SIG_IGN);
+	sigquit_ours = signal (SIGQUIT, SIG_IGN);
 #endif
-	}
+      }
 
-      if (job_control)
-	{
+    if (job_control)
+      {
 #ifdef HAVE_TERMIOS_H
-	  /* If we can't tell the inferior's actual process group,
-	     then restore whatever was the foreground pgrp the last
-	     time the inferior was running.  See also comments
-	     describing terminal_state::process_group.  */
+	/* If we can't tell the inferior's actual process group,
+	   then restore whatever was the foreground pgrp the last
+	   time the inferior was running.  See also comments
+	   describing terminal_state::process_group.  */
 #ifdef HAVE_GETPGID
-	  result = tcsetpgrp (0, getpgid (inf->pid));
+	result = tcsetpgrp (0, getpgid (inf->pid));
 #else
-	  result = tcsetpgrp (0, tinfo->process_group);
+	result = tcsetpgrp (0, tinfo->process_group);
 #endif
-	  if (result == -1)
-	    {
+	if (result == -1)
+	  {
 #if 0
-	      /* This fails if either GDB has no controlling terminal,
-		 e.g., running under 'setsid(1)', or if the inferior
-		 is not attached to GDB's controlling terminal.  E.g.,
-		 if it called setsid to create a new session or used
-		 the TIOCNOTTY ioctl, or simply if we've attached to a
-		 process running on another terminal and we couldn't
-		 tell whether it was sharing GDB's terminal (and so
-		 assumed yes).  */
-	      gdb_printf
-		(gdb_stderr,
-		 "[tcsetpgrp failed in child_terminal_inferior: %s]\n",
-		 safe_strerror (errno));
+	    /* This fails if either GDB has no controlling terminal,
+	       e.g., running under 'setsid(1)', or if the inferior
+	       is not attached to GDB's controlling terminal.  E.g.,
+	       if it called setsid to create a new session or used
+	       the TIOCNOTTY ioctl, or simply if we've attached to a
+	       process running on another terminal and we couldn't
+	       tell whether it was sharing GDB's terminal (and so
+	       assumed yes).  */
+	    gdb_printf
+	      (gdb_stderr,
+	       "[tcsetpgrp failed in child_terminal_inferior: %s]\n",
+	       safe_strerror (errno));
 #endif
-	    }
+	  }
 #endif
-	}
+      }
 
-      gdb_tty_state = target_terminal_state::is_inferior;
-    }
+    gdb_tty_state = target_terminal_state::is_inferior;
+  }
+
+  /* Note that the new state is active.   */
+  gdb::observers::terminal_owner_changed.notify (gdb_tty_state, true);
 }
 
 /* Put some of our terminal settings into effect,
@@ -447,55 +456,64 @@ child_terminal_ours_1 (target_terminal_state desired_state)
   if (!gdb_has_a_terminal ())
     return;
 
-  if (gdb_tty_state != desired_state)
-    {
-      int result ATTRIBUTE_UNUSED;
+  if (gdb_tty_state == desired_state)
+    return;
 
-      /* Ignore SIGTTOU since it will happen when we try to set the
-	 terminal's pgrp.  */
-      scoped_ignore_sigttou ignore_sigttou;
+  /* Note that the old state will no longer be active.   */
+  gdb::observers::terminal_owner_changed.notify (gdb_tty_state, false);
 
-      /* Set tty state to our_ttystate.  */
-      serial_set_tty_state (stdin_serial, our_terminal_info.ttystate);
+  /* Scope containing scoped_ignore_sigtto.  */
+  {
+    int result ATTRIBUTE_UNUSED;
 
-      /* If we only want output, then leave the inferior's pgrp in the
-	 foreground, so that Ctrl-C/Ctrl-Z reach the inferior
-	 directly.  */
-      if (job_control && desired_state == target_terminal_state::is_ours)
-	{
+    /* Ignore SIGTTOU since it will happen when we try to set the
+       terminal's pgrp.  */
+    scoped_ignore_sigttou ignore_sigttou;
+
+    /* Set tty state to our_ttystate.  */
+    serial_set_tty_state (stdin_serial, our_terminal_info.ttystate);
+
+    /* If we only want output, then leave the inferior's pgrp in the
+       foreground, so that Ctrl-C/Ctrl-Z reach the inferior
+       directly.  */
+    if (job_control && desired_state == target_terminal_state::is_ours)
+      {
 #ifdef HAVE_TERMIOS_H
-	  result = tcsetpgrp (0, our_terminal_info.process_group);
+	result = tcsetpgrp (0, our_terminal_info.process_group);
 #if 0
-	  /* This fails on Ultrix with EINVAL if you run the testsuite
-	     in the background with nohup, and then log out.  GDB never
-	     used to check for an error here, so perhaps there are other
-	     such situations as well.  */
-	  if (result == -1)
-	    gdb_printf (gdb_stderr,
-			"[tcsetpgrp failed in child_terminal_ours: %s]\n",
-			safe_strerror (errno));
+	/* This fails on Ultrix with EINVAL if you run the testsuite
+	   in the background with nohup, and then log out.  GDB never
+	   used to check for an error here, so perhaps there are other
+	   such situations as well.  */
+	if (result == -1)
+	  gdb_printf (gdb_stderr,
+		      "[tcsetpgrp failed in child_terminal_ours: %s]\n",
+		      safe_strerror (errno));
 #endif
 #endif /* termios */
-	}
+      }
 
-      if (!job_control && desired_state == target_terminal_state::is_ours)
-	{
-	  if (sigint_ours.has_value ())
-	    install_sigint_handler (*sigint_ours);
-	  sigint_ours.reset ();
+    if (!job_control && desired_state == target_terminal_state::is_ours)
+      {
+	if (sigint_ours.has_value ())
+	  install_sigint_handler (*sigint_ours);
+	sigint_ours.reset ();
 #ifdef SIGQUIT
-	  if (sigquit_ours.has_value ())
-	    signal (SIGQUIT, *sigquit_ours);
-	  sigquit_ours.reset ();
+	if (sigquit_ours.has_value ())
+	  signal (SIGQUIT, *sigquit_ours);
+	sigquit_ours.reset ();
 #endif
-	}
+      }
 
 #ifdef F_GETFL
-      result = fcntl (0, F_SETFL, our_terminal_info.tflags);
+    result = fcntl (0, F_SETFL, our_terminal_info.tflags);
 #endif
 
-      gdb_tty_state = desired_state;
-    }
+    gdb_tty_state = desired_state;
+  }
+
+  /* Note that the new state is active.   */
+  gdb::observers::terminal_owner_changed.notify (gdb_tty_state, true);
 }
 
 /* Interrupt the inferior.  Implementation of target_interrupt for
diff --git a/gdb/observable.c b/gdb/observable.c
index 82563e3fab4..2ae9f325899 100644
--- a/gdb/observable.c
+++ b/gdb/observable.c
@@ -82,6 +82,7 @@ DEFINE_OBSERVABLE (gdb_exiting);
 DEFINE_OBSERVABLE (connection_removed);
 DEFINE_OBSERVABLE (target_pre_wait);
 DEFINE_OBSERVABLE (target_post_wait);
+DEFINE_OBSERVABLE (terminal_owner_changed);
 
 } /* namespace observers */
 } /* namespace gdb */
diff --git a/gdb/observable.h b/gdb/observable.h
index e4e6f021f1a..9a4144a13ca 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -22,6 +22,7 @@
 
 #include "gdbsupport/observable.h"
 #include "target/waitstatus.h"
+#include "target/target.h"
 
 struct bpstat;
 struct so_list;
@@ -276,6 +277,10 @@ extern observable <ptid_t /* ptid */> target_pre_wait;
 /* About to leave target_wait (). */
 extern observable <ptid_t /* event_ptid */> target_post_wait;
 
+/* When the terminal owner changes.  */
+extern observable <target_terminal_state /* gdb_tty_state */, bool /* active */>
+    terminal_owner_changed;
+
 } /* namespace observers */
 
 } /* namespace gdb */
-- 
2.35.3

[PATCH 3/3] [gdb/tui] Refresh on target_terminal_state::is_ours

[Tom de Vries]

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
- the updates in the src and status window are misaligned with the current
  screen.

We can fix this by doing ^L.

Fix this by doing ^L automatically when gdb_tty_state changes to
target_terminal_state::is_ours.

Tested on x86_64-linux.
---
 gdb/testsuite/gdb.tui/hello.exp |  3 ---
 gdb/tui/tui-hooks.c             | 11 +++++++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.tui/hello.exp b/gdb/testsuite/gdb.tui/hello.exp
index 9634d9d8059..641b3b0c48e 100644
--- a/gdb/testsuite/gdb.tui/hello.exp
+++ b/gdb/testsuite/gdb.tui/hello.exp
@@ -47,9 +47,6 @@ 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
 
diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index d67c3d7f353..d1d307cf9ce 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -211,6 +211,15 @@ tui_symtab_changed ()
   from_source_symtab = true;
 }
 
+/* Observer for terminal_owner_changed.  */
+
+static void
+tui_terminal_owner_changed (target_terminal_state gdb_tty_state, bool active)
+{
+  if (active && gdb_tty_state == target_terminal_state::is_ours)
+    tui_refresh_all_win (false);
+}
+
 /* Token associated with observers registered while TUI hooks are
    installed.  */
 static const gdb::observers::token tui_observers_token {};
@@ -250,6 +259,8 @@ tui_attach_detach_observers (bool attach)
 		    tui_context_changed, attach);
   attach_or_detach (gdb::observers::current_source_symtab_and_line_changed,
 		    tui_symtab_changed, attach);
+  attach_or_detach (gdb::observers::terminal_owner_changed,
+		    tui_terminal_owner_changed, attach);
 }
 
 /* Install the TUI specific hooks.  */
-- 
2.35.3

Re: [PATCH 3/3] [gdb/tui] Refresh on target_terminal_state::is_ours

[Bruno Larsen]

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
> - the updates in the src and status window are misaligned with the current
>    screen.
>
> We can fix this by doing ^L.
>
> Fix this by doing ^L automatically when gdb_tty_state changes to
> target_terminal_state::is_ours.
>
> Tested on x86_64-linux.

This is a nice cleanup after patch #1. Once the issue I outlined in my 
comment to that patch is solved, this will be a nice cleanup for the 
functionality, and the series as a whole does not add any regressions.

Reviewed-By: Bruno Larsen <blarsen@redhat.com>

> ---
>   gdb/testsuite/gdb.tui/hello.exp |  3 ---
>   gdb/tui/tui-hooks.c             | 11 +++++++++++
>   2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.tui/hello.exp b/gdb/testsuite/gdb.tui/hello.exp
> index 9634d9d8059..641b3b0c48e 100644
> --- a/gdb/testsuite/gdb.tui/hello.exp
> +++ b/gdb/testsuite/gdb.tui/hello.exp
> @@ -47,9 +47,6 @@ 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
>   
> diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
> index d67c3d7f353..d1d307cf9ce 100644
> --- a/gdb/tui/tui-hooks.c
> +++ b/gdb/tui/tui-hooks.c
> @@ -211,6 +211,15 @@ tui_symtab_changed ()
>     from_source_symtab = true;
>   }
>   
> +/* Observer for terminal_owner_changed.  */
> +
> +static void
> +tui_terminal_owner_changed (target_terminal_state gdb_tty_state, bool active)
> +{
> +  if (active && gdb_tty_state == target_terminal_state::is_ours)
> +    tui_refresh_all_win (false);
> +}
> +
>   /* Token associated with observers registered while TUI hooks are
>      installed.  */
>   static const gdb::observers::token tui_observers_token {};
> @@ -250,6 +259,8 @@ tui_attach_detach_observers (bool attach)
>   		    tui_context_changed, attach);
>     attach_or_detach (gdb::observers::current_source_symtab_and_line_changed,
>   		    tui_symtab_changed, attach);
> +  attach_or_detach (gdb::observers::terminal_owner_changed,
> +		    tui_terminal_owner_changed, attach);
>   }
>   
>   /* Install the TUI specific hooks.  */