[v2] Fix first time you type UP or DOWN in TUI's command window (Re: [RFC 8.3 0/3] Some style fixes)

Message ID bc92ee1d-78d4-f3e8-94d8-c0473f357006@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves March 15, 2019, 2:15 p.m. UTC
  On 03/12/2019 04:44 PM, Eli Zaretskii wrote:
> 3. This patch fixes another problem noticed by Pedro: the first time
> you type UP or DOWN arrow in the command window, GDB should scroll the
> source window, but instead it displays the line number and the file
> name in the command window(?).  What happens there is that the first
> time we call tui_ui_out::do_field_int, it doesn't initialize m_line,
> because m_start_of_line is -1, as set by the constructor; and then the
> following call to tui_ui_out::do_field_string falls back to
> cli_ui_out::do_field_string because m_line is zero.  The fix below is
> perhaps somewhat ad-hoc, mainly because I couldn't understand the
> semantics of the values of m_start_of_line, especially its
> non-positive values; please consider documenting them in the header.
> Maybe if someone explains the semantics, I could come up with a more
> sound patch (or conclude that the below is TRT).  Also, it looks like
> the second test for m_line > 0 in tui_ui_out::do_field_string is
> redundant?
> 
> --- gdb/tui/tui-out.c~4	2019-02-27 06:51:50.000000000 +0200
> +++ gdb/tui/tui-out.c	2019-03-12 12:30:23.924230000 +0200
> @@ -34,6 +34,9 @@ tui_ui_out::do_field_int (int fldno, int
>    if (suppress_output ())
>      return;
>  
> +  if (m_start_of_line < 0 && m_line == 0)
> +    m_start_of_line = 0;
> +
>    /* Don't print line number, keep it for later.  */
>    if (m_start_of_line == 0 && strcmp (fldname, "line") == 0)
>      {
> 

I noticed that m_start_of_line never goes back to -1.
It is reset to 0 here:

void
tui_ui_out::do_text (const char *string)
{
...
      if (strchr (string, '\n') != 0)
        {
          m_line = -1;
          m_start_of_line = 0;
        }


and notice how m_line is reset to -1.  This is the exact
opposite of how the fields are initialized in the ctor:

 tui_ui_out::tui_ui_out (ui_file *stream)
 : cli_ui_out (stream, 0),
   m_line (0),
   m_start_of_line (-1)
 {
 }

... which made me suspect of a typo in the C++ification
of tui_ui_out.  Looking at that commit, 112e8700a6f, we see:

 -struct ui_out *
 -tui_out_new (struct ui_file *stream)
 +tui_ui_out::tui_ui_out (ui_file *stream)
 +: cli_ui_out (stream, 0),
 +  m_line (0),
 +  m_start_of_line (-1)
  {
 -
 -  /* Initialize our fields.  */
 -  data->line = -1;
 -  data->start_of_line = 0;

Bingo.

So I think this is the right fix:

From 781ed3b6d82a4fb54f7bfe59185f0e6e9efd6b59 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 15 Mar 2019 13:05:26 +0000
Subject: [PATCH] Fix first time you type UP or DOWN in TUI's command window

The first time you type UP or DOWN arrow in the command window, GDB
should scroll the source window, but instead it displays the line
number and the file name in the command window(?).

What happens there is that the first time we call
tui_ui_out::do_field_int, it doesn't initialize m_line, because
m_start_of_line is -1, as set by the constructor; and then the
following call to tui_ui_out::do_field_string falls back to
cli_ui_out::do_field_string because m_line is zero.

The problem is caused by a typo in the C++ification of tui_ui_out,
commit 112e8700a6f, where m_line and m_start_of_line's initial values
were swapped from what they used to be:

 -struct ui_out *
 -tui_out_new (struct ui_file *stream)
 +tui_ui_out::tui_ui_out (ui_file *stream)
 +: cli_ui_out (stream, 0),
 +  m_line (0),
 +  m_start_of_line (-1)
  {
 -
 -  /* Initialize our fields.  */
 -  data->line = -1;
 -  data->start_of_line = 0;

This commit fixes it.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
	    Eli Zaretskii <eliz@gnu.org>

	* tui/tui-out.c (tui_ui_out::tui_ui_out): Fix initialization of
	m_line and m_start_of_line.
---
 gdb/tui/tui-out.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

Comments

Eli Zaretskii March 15, 2019, 3:37 p.m. UTC | #1
> Cc: gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 15 Mar 2019 14:15:23 +0000
> 
> Bingo.
> 
> So I think this is the right fix:
> 
> >From 781ed3b6d82a4fb54f7bfe59185f0e6e9efd6b59 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 15 Mar 2019 13:05:26 +0000
> Subject: [PATCH] Fix first time you type UP or DOWN in TUI's command window
> 
> The first time you type UP or DOWN arrow in the command window, GDB
> should scroll the source window, but instead it displays the line
> number and the file name in the command window(?).
> 
> What happens there is that the first time we call
> tui_ui_out::do_field_int, it doesn't initialize m_line, because
> m_start_of_line is -1, as set by the constructor; and then the
> following call to tui_ui_out::do_field_string falls back to
> cli_ui_out::do_field_string because m_line is zero.
> 
> The problem is caused by a typo in the C++ification of tui_ui_out,
> commit 112e8700a6f, where m_line and m_start_of_line's initial values
> were swapped from what they used to be:

Thanks, this sound right to me.  Still, I'd welcome some comments in
the header which explain the semantics of non-positive values of these
members, and for m_start_of_line, also its role in general.  Fixing
this bug could have been much easier if that information was
available to begin with.
  

Patch

diff --git a/gdb/tui/tui-out.c b/gdb/tui/tui-out.c
index d5a173b94a..dd37736c4a 100644
--- a/gdb/tui/tui-out.c
+++ b/gdb/tui/tui-out.c
@@ -109,8 +109,8 @@  tui_ui_out::do_text (const char *string)
 
 tui_ui_out::tui_ui_out (ui_file *stream)
 : cli_ui_out (stream, 0),
-  m_line (0),
-  m_start_of_line (-1)
+  m_line (-1),
+  m_start_of_line (0)
 {
 }