Remove tui-out.[ch]

Message ID 20231216010428.1815384-1-tom@tromey.com
State New
Headers
Series Remove tui-out.[ch] |

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 Tromey Dec. 16, 2023, 1:04 a.m. UTC
  The other day on irc, we were discussing the "m_line" hack in
tui-out.c, and I mentioned that it would be nice to replace this with
a new ui_out_flag.

Later, I looked at ui_out_flag and found:

      ui_source_list = (1 << 0),

... and sure enough, this is tested already.

This patch removes tui-out.[ch] and changes the TUI to use an ordinary
cli-out object without this flag set.

As far as I can tell, this doesn't affect behavior at all -- the TUI
tests all pass, and interactively I tried switching stack frames,
"list", etc, and it all seems to work.
---
 gdb/Makefile.in   |   2 -
 gdb/source.c      |  19 +++-----
 gdb/tui/tui-io.c  |   3 +-
 gdb/tui/tui-out.c | 111 ----------------------------------------------
 gdb/tui/tui-out.h |  64 --------------------------
 5 files changed, 6 insertions(+), 193 deletions(-)
 delete mode 100644 gdb/tui/tui-out.c
 delete mode 100644 gdb/tui/tui-out.h
  

Comments

Keith Seitz Jan. 5, 2024, 11:04 p.m. UTC | #1
Hi, Tom,

On 12/15/23 17:04, Tom Tromey wrote:
> The other day on irc, we were discussing the "m_line" hack in
> tui-out.c, and I mentioned that it would be nice to replace this with
> a new ui_out_flag.
> 
> Later, I looked at ui_out_flag and found:
> 
>        ui_source_list = (1 << 0),
> 
> ... and sure enough, this is tested already.
> 
> This patch removes tui-out.[ch] and changes the TUI to use an ordinary
> cli-out object without this flag set.

This is a nice clean-up, however, my testing has shown a small problem
(which I haven't yet dug into).

> As far as I can tell, this doesn't affect behavior at all -- the TUI
> tests all pass, and interactively I tried switching stack frames,
> "list", etc, and it all seems to work.

I can verify that the testsuite shows no regressions, and manual
testing of this with the TUI does appear to work, however, I noted
some subtle changes in behavior which are, unfortunately, wrong.

Consider some generic binary, "bin". If I load this into gdb
via the command line, manually enter TUI, and then issue the
commands "list", "start", and "list", let's compare before/after
behavior.

BEFORE (15.0.50.20240103-git):
$ gdb -nx -q ~/tmp/bin
(gdb) tui en
<<source window centers at main() on line 13>>
<<console displays only prompt>>
(tui) list
<<source window jumps to line 1>>
<<console displays new prompt>>
(tui) start
[snip generic output]
Temporary breakpoint 1, main() at bin.c:13
<<source window jumps to line 13>>
(tui) list
<<no change to source window (still at line 13)>>
<<console displays new prompt>>

AFTER:
$ gdb -nx -q ~/tmp/bin
(gdb) tui en
<<no change from BEFORE>>
(tui) list
1       in
<<source window jumps to line 1>>
(tui) start
<<no change from BEFORE>>
(tui) list
8       in
<<no change to source window (still at line 13)>>

I hope it is somewhat apparent what's happening. Essentially when using
"list", there is new output to the TUI console, and it is indicating
the wrong stop point (line 8 (incorrect) vs 13 (correct)).

> diff --git a/gdb/source.c b/gdb/source.c
> index f648adc4520..1b58497dbd1 100644
> --- a/gdb/source.c
> +++ b/gdb/source.c
> @@ -1345,25 +1345,16 @@ print_source_lines_base (struct symtab *s, int line, int stopline,
>   	  uiout->field_signed ("line", line);
>   	  uiout->text ("\tin ");
>   
> -	  /* CLI expects only the "file" field.  TUI expects only the
> -	     "fullname" field (and TUI does break if "file" is printed).
> -	     MI expects both fields.  ui_source_list is set only for CLI,
> -	     not for TUI.  */
> +	  /* CLI expects only the "file" field.  MI expects both
> +	     fields.  ui_source_list is set only for CLI, not for
> +	     TUI.  */
>   	  if (uiout->is_mi_like_p () || uiout->test_flags (ui_source_list))
>   	    uiout->field_string ("file", symtab_to_filename_for_display (s),
>   				 file_name_style.style ());
> -	  if (uiout->is_mi_like_p () || !uiout->test_flags (ui_source_list))
> +	  if (uiout->is_mi_like_p ())
>   	    {
>   	      const char *s_fullname = symtab_to_fullname (s);
> -	      char *local_fullname;
> -
> -	      /* ui_out_field_string may free S_FULLNAME by calling
> -		 open_source_file for it again.  See e.g.,
> -		 tui_field_string->tui_show_source.  */
> -	      local_fullname = (char *) alloca (strlen (s_fullname) + 1);
> -	      strcpy (local_fullname, s_fullname);
> -
> -	      uiout->field_string ("fullname", local_fullname);
> +	      uiout->field_string ("fullname", s_fullname);
>   	    }
>   
>   	  uiout->text ("\n");

The hunk above changes the case where both uiout->is_mi_like_p and
ui_source_list are false/unset. `fullname' is never output as the
previous comment stated. You altered that comment, so maybe this
doesn't apply anymore. Nonetheless, it is an unexplained change (that
isn't entirely obvious to me), so I'm mentioning it.

FWIW, if I add the removed check back to output `fullname', then the
listings in the AFTER case at least now show "in /home/keiths/tmp/bin.c.
So while that's closer to correct (at least it displays the symtab's
name now), it is still incorrect that it outputs anything at all
compared to previously.

My apologies this explanation has been a bit sloppy. Copy/paste is
disabled on TUI for me.

Keith
  
Tom Tromey March 5, 2024, 12:21 a.m. UTC | #2
>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

>> As far as I can tell, this doesn't affect behavior at all -- the TUI
>> tests all pass, and interactively I tried switching stack frames,
>> "list", etc, and it all seems to work.

Keith> I can verify that the testsuite shows no regressions, and manual
Keith> testing of this with the TUI does appear to work, however, I noted
Keith> some subtle changes in behavior which are, unfortunately, wrong.

Thanks for looking at this.

Keith> BEFORE (15.0.50.20240103-git):
Keith> $ gdb -nx -q ~/tmp/bin
Keith> (gdb) tui en
Keith> <<source window centers at main() on line 13>>
Keith> <<console displays only prompt>>
Keith> (tui) list
Keith> <<source window jumps to line 1>>
Keith> <<console displays new prompt>>

This seems to be a little different on git master.

Keith> AFTER:
...
Keith> (tui) list
Keith> 1       in
Keith> <<source window jumps to line 1>>

I've fixed this.

>> -	  /* CLI expects only the "file" field.  TUI expects only the
>> -	     "fullname" field (and TUI does break if "file" is printed).
>> -	     MI expects both fields.  ui_source_list is set only for CLI,
>> -	     not for TUI.  */
>> +	  /* CLI expects only the "file" field.  MI expects both
>> +	     fields.  ui_source_list is set only for CLI, not for
>> +	     TUI.  */

Keith> The hunk above changes the case where both uiout->is_mi_like_p and
Keith> ui_source_list are false/unset. `fullname' is never output as the
Keith> previous comment stated. You altered that comment, so maybe this
Keith> doesn't apply anymore. Nonetheless, it is an unexplained change (that
Keith> isn't entirely obvious to me), so I'm mentioning it.

Yeah, this stuff was only needed because the TUI ui-out has this hack in
it.  Without that hack, we can also simplify this area.

I still haven't written a new test to check this change, I'll do that
before submitting v2.

Tom
  

Patch

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 0886c0e8495..ce78d917764 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -318,7 +318,6 @@  SUBDIR_TUI_SRCS = \
 	tui/tui-io.c \
 	tui/tui-layout.c \
 	tui/tui-location.c \
-	tui/tui-out.c \
 	tui/tui-regs.c \
 	tui/tui-source.c \
 	tui/tui-stack.c \
@@ -1605,7 +1604,6 @@  HFILES_NO_SRCDIR = \
 	tui/tui-io.h \
 	tui/tui-layout.h \
 	tui/tui-location.h \
-	tui/tui-out.h \
 	tui/tui-regs.h \
 	tui/tui-source.h \
 	tui/tui-stack.h \
diff --git a/gdb/source.c b/gdb/source.c
index f648adc4520..1b58497dbd1 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1345,25 +1345,16 @@  print_source_lines_base (struct symtab *s, int line, int stopline,
 	  uiout->field_signed ("line", line);
 	  uiout->text ("\tin ");
 
-	  /* CLI expects only the "file" field.  TUI expects only the
-	     "fullname" field (and TUI does break if "file" is printed).
-	     MI expects both fields.  ui_source_list is set only for CLI,
-	     not for TUI.  */
+	  /* CLI expects only the "file" field.  MI expects both
+	     fields.  ui_source_list is set only for CLI, not for
+	     TUI.  */
 	  if (uiout->is_mi_like_p () || uiout->test_flags (ui_source_list))
 	    uiout->field_string ("file", symtab_to_filename_for_display (s),
 				 file_name_style.style ());
-	  if (uiout->is_mi_like_p () || !uiout->test_flags (ui_source_list))
+	  if (uiout->is_mi_like_p ())
 	    {
 	      const char *s_fullname = symtab_to_fullname (s);
-	      char *local_fullname;
-
-	      /* ui_out_field_string may free S_FULLNAME by calling
-		 open_source_file for it again.  See e.g.,
-		 tui_field_string->tui_show_source.  */
-	      local_fullname = (char *) alloca (strlen (s_fullname) + 1);
-	      strcpy (local_fullname, s_fullname);
-
-	      uiout->field_string ("fullname", local_fullname);
+	      uiout->field_string ("fullname", s_fullname);
 	    }
 
 	  uiout->text ("\n");
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 84724643fd5..382239e4dd5 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -33,7 +33,6 @@ 
 #include "tui/tui-win.h"
 #include "tui/tui-wingeneral.h"
 #include "tui/tui-file.h"
-#include "tui/tui-out.h"
 #include "ui-out.h"
 #include "cli-out.h"
 #include <fcntl.h>
@@ -917,7 +916,7 @@  tui_initialize_io (void)
   tui_stdout = new pager_file (new tui_file (stdout, true));
   tui_stderr = new tui_file (stderr, false);
   tui_stdlog = new timestamped_file (tui_stderr);
-  tui_out = new tui_ui_out (tui_stdout);
+  tui_out = new cli_ui_out (tui_stdout, 0);
 
   /* Create the default UI.  */
   tui_old_uiout = new cli_ui_out (gdb_stdout);
diff --git a/gdb/tui/tui-out.c b/gdb/tui/tui-out.c
deleted file mode 100644
index b8e7de197cf..00000000000
--- a/gdb/tui/tui-out.c
+++ /dev/null
@@ -1,111 +0,0 @@ 
-/* Output generating routines for GDB CLI.
-
-   Copyright (C) 1999-2023 Free Software Foundation, Inc.
-
-   Contributed by Cygnus Solutions.
-   Written by Fernando Nasser for Cygnus.
-
-   This file is part of GDB.
-
-   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 "defs.h"
-#include "ui-out.h"
-#include "tui-out.h"
-#include "tui.h"
-
-/* Output an int field.  */
-
-void
-tui_ui_out::do_field_signed (int fldno, int width, ui_align alignment,
-			     const char *fldname, LONGEST value)
-{
-  if (suppress_output ())
-    return;
-
-  /* Don't print line number, keep it for later.  */
-  if (m_start_of_line == 0 && strcmp (fldname, "line") == 0)
-    {
-      m_start_of_line++;
-      m_line = value;
-      return;
-    }
-  m_start_of_line++;
-
-  cli_ui_out::do_field_signed (fldno, width, alignment, fldname, value);
-}
-
-/* Other cli_field_* end up here so alignment and field separators are
-   both handled by tui_field_string.  */
-
-void
-tui_ui_out::do_field_string (int fldno, int width, ui_align align,
-			     const char *fldname, const char *string,
-			     const ui_file_style &style)
-{
-  if (suppress_output ())
-    return;
-
-  m_start_of_line++;
-
-  if (fldname && m_line > 0 && strcmp (fldname, "fullname") == 0)
-    return;
-
-  cli_ui_out::do_field_string (fldno, width, align, fldname, string, style);
-}
-
-void
-tui_ui_out::do_field_fmt (int fldno, int width, ui_align align,
-			  const char *fldname, const ui_file_style &style,
-			  const char *format, va_list args)
-{
-  if (suppress_output ())
-    return;
-
-  m_start_of_line++;
-
-  cli_ui_out::do_field_fmt (fldno, width, align, fldname, style, format, args);
-}
-
-void
-tui_ui_out::do_text (const char *string)
-{
-  if (suppress_output ())
-    return;
-
-  m_start_of_line++;
-  if (m_line > 0)
-    {
-      /* Printing a source line, so suppress regular output -- the
-	 line was shown on the TUI's source window by tui_show_source
-	 above instead.  */
-      if (strchr (string, '\n') != 0)
-	{
-	  /* We've reached the end of the line, so go back to letting
-	     text output go to the console.  */
-	  m_line = 0;
-	  m_start_of_line = 0;
-	}
-      return;
-    }
-  if (strchr (string, '\n'))
-    m_start_of_line = 0;
-
-  cli_ui_out::do_text (string);
-}
-
-tui_ui_out::tui_ui_out (ui_file *stream)
-  : cli_ui_out (stream, 0)
-{
-}
diff --git a/gdb/tui/tui-out.h b/gdb/tui/tui-out.h
deleted file mode 100644
index ec7e02bb900..00000000000
--- a/gdb/tui/tui-out.h
+++ /dev/null
@@ -1,64 +0,0 @@ 
-/* Copyright (C) 2016-2023 Free Software Foundation, Inc.
-
-   This file is part of GDB.
-
-   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/>.  */
-
-#ifndef TUI_TUI_OUT_H
-#define TUI_TUI_OUT_H
-
-#include "cli-out.h"
-
-/* A ui_out class for the TUI.  This is just like the CLI's ui_out,
-   except that it overrides output methods to detect when a source
-   line is being printed and show the source in the TUI's source
-   window instead of printing the line in the console window.  */
-class tui_ui_out : public cli_ui_out
-{
-public:
-
-  explicit tui_ui_out (ui_file *stream);
-
-protected:
-
-  void do_field_signed (int fldno, int width, ui_align align, const char *fldname,
-			LONGEST value) override;
-  void do_field_string (int fldno, int width, ui_align align, const char *fldname,
-			const char *string, const ui_file_style &style) override;
-  void do_field_fmt (int fldno, int width, ui_align align, const char *fldname,
-		     const ui_file_style &style,
-		     const char *format, va_list args) override
-    ATTRIBUTE_PRINTF (7, 0);
-  void do_text (const char *string) override;
-
-private:
-
-  /* These fields are used to make print_source_lines show the source
-     in the TUI's source window instead of in the console.
-     M_START_OF_LINE is incremented whenever something is output to
-     the ui_out.  If an integer field named "line" is printed on the
-     ui_out, and nothing else has been printed yet (both
-     M_START_OF_LINE and M_LINE are still 0), we assume
-     print_source_lines is starting to print a source line, and thus
-     record the line number in M_LINE.  Afterwards, when we see a
-     string field named "fullname" being output, we take the fullname
-     and the recorded line and show the source line in the TUI's
-     source window.  tui_ui_out::do_text() suppresses text output
-     until it sees an endline being printed, at which point these
-     variables are reset back to 0.  */
-  int m_line = 0;
-  int m_start_of_line = 0;
-};
-
-#endif /* TUI_TUI_OUT_H */