[RFC 8.3 0/3] Some style fixes

[RFC 8.3 0/3] Some style fixes

[Tom Tromey]

This series fixes some of the TUI/styling regressions noted by Pedro.
I would like to push this to the 8.3 branch as well.

Patch #2 is a new feature that Eli mentioned.  Since it's an addition
to a feature that is also new in 8.3, it seemed reasonable to do at
this time.

Let me know what you think.

Tom

[RFC 8.3 2/3] Add the "set style source" command

[Tom Tromey]

This adds "set style source" (and "show style source") commands.  This
gives the user control over whether source code is highlighted.

gdb/ChangeLog
2019-03-08  Tom Tromey  <tromey@adacore.com>

	* NEWS: Add item for "style sources" commands.
	* source-cache.c (source_cache::get_source_lines): Check
	source_styling.
	* cli/cli-style.c (source_styling): New global.
	(_initialize_cli_style): Add "style sources" commands.
	(show_style_sources): New function.
	* cli/cli-style.h (source_styling): Declare.

gdb/doc/ChangeLog
2019-03-08  Tom Tromey  <tromey@adacore.com>

	* gdb.texinfo (Output Styling): Document "set style source" and
	"show style source".

gdb/testsuite/ChangeLog
2019-03-08  Tom Tromey  <tromey@adacore.com>

	* gdb.base/style.exp: Add "set style sources" test.
---
 gdb/ChangeLog                    | 10 ++++++++++
 gdb/NEWS                         |  6 ++++++
 gdb/cli/cli-style.c              | 22 ++++++++++++++++++++++
 gdb/cli/cli-style.h              |  3 +++
 gdb/doc/ChangeLog                |  5 +++++
 gdb/doc/gdb.texinfo              |  9 +++++++++
 gdb/source-cache.c               |  2 +-
 gdb/testsuite/ChangeLog          |  4 ++++
 gdb/testsuite/gdb.base/style.exp |  6 ++++++
 9 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index cc7c35c0642..06564311643 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -145,6 +145,12 @@ show style enabled
   Enable or disable terminal styling.  Styling is enabled by default
   on most hosts, but disabled by default when in batch mode.
 
+set style sources [on|off]
+show style sources
+  Enable or disable source code styling.  Source code styling is
+  enabled by default, but only takes effect if styling in general is
+  enabled, if if GDB was linked with GNU Source Highlight.
+
 set style filename foreground COLOR
 set style filename background COLOR
 set style filename intensity VALUE
diff --git a/gdb/cli/cli-style.c b/gdb/cli/cli-style.c
index 729a024c47a..6b3e92f9ec4 100644
--- a/gdb/cli/cli-style.c
+++ b/gdb/cli/cli-style.c
@@ -31,6 +31,11 @@ int cli_styling = 0;
 int cli_styling = 1;
 #endif
 
+/* True if source styling is enabled.  Note that this is only
+   consulted when cli_styling is true.  */
+
+int source_styling = 1;
+
 /* Name of colors; must correspond to ui_file_style::basic_color.  */
 static const char * const cli_colors[] = {
   "none",
@@ -230,6 +235,16 @@ show_style_enabled (struct ui_file *file, int from_tty,
     fprintf_filtered (file, _("CLI output styling is disabled.\n"));
 }
 
+static void
+show_style_sources (struct ui_file *file, int from_tty,
+		    struct cmd_list_element *c, const char *value)
+{
+  if (source_styling)
+    fprintf_filtered (file, _("Source code styling is enabled.\n"));
+  else
+    fprintf_filtered (file, _("Source code styling is disabled.\n"));
+}
+
 void
 _initialize_cli_style ()
 {
@@ -249,6 +264,13 @@ If enabled, output to the terminal is styled."),
 			   set_style_enabled, show_style_enabled,
 			   &style_set_list, &style_show_list);
 
+  add_setshow_boolean_cmd ("sources", no_class, &source_styling, _("\
+Set whether source code styling is enabled."), _("\
+Show whether source code styling is enabled."), _("\
+If enabled, source code is styled."),
+			   set_style_enabled, show_style_sources,
+			   &style_set_list, &style_show_list);
+
 #define STYLE_ADD_SETSHOW_COMMANDS(STYLE, NAME, PREFIX_DOC)	  \
   STYLE.add_setshow_commands (NAME, no_class, PREFIX_DOC,		\
 			      &style_set_list,				\
diff --git a/gdb/cli/cli-style.h b/gdb/cli/cli-style.h
index 287bad7d201..c0520f9e23f 100644
--- a/gdb/cli/cli-style.h
+++ b/gdb/cli/cli-style.h
@@ -93,6 +93,9 @@ extern cli_style_option variable_name_style;
 /* The address style.  */
 extern cli_style_option address_style;
 
+/* True if source styling is enabled.  */
+extern int source_styling;
+
 /* True if styling is enabled.  */
 extern int cli_styling;
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f2028f86b0d..346d3c8d6be 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -24525,6 +24525,15 @@ most hosts defaulting to @samp{on}.
 
 @item show style enabled
 Show the current state of styling.
+
+@item set style sources @samp{on|off}
+Enable or disable source code styling.  This affects whether source
+code, such as the output of the @code{list} command, is styled.  This
+only works if @value{GDBN} was linked with the GNU Source Highlight
+library.  The default is @samp{on}.
+
+@item show style sources
+Show the current state of source code styling.
 @end table
 
 Subcommands of @code{set style} control specific forms of styling.
diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 097c8a3ae12..27a0ade959c 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -174,7 +174,7 @@ source_cache::get_source_lines (struct symtab *s, int first_line,
     return false;
 
 #ifdef HAVE_SOURCE_HIGHLIGHT
-  if (can_emit_style_escape (gdb_stdout))
+  if (source_styling && can_emit_style_escape (gdb_stdout))
     {
       const char *fullname = symtab_to_fullname (s);
 
diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp
index 2778001fed5..369c1f59a88 100644
--- a/gdb/testsuite/gdb.base/style.exp
+++ b/gdb/testsuite/gdb.base/style.exp
@@ -50,6 +50,12 @@ save_vars { env(TERM) } {
 	"$main_expr.*$arg_expr.*$arg_expr.*$file_expr.*"
     gdb_test "info breakpoints" "$main_expr at $file_expr.*"
 
+    gdb_test_no_output "set style sources off"
+    gdb_test "frame" \
+	"\r\n\[^\033\]*break here.*" \
+	"frame without styling"
+    gdb_test_no_output "set style sources on"
+
     gdb_test "break main" "file $base_file_expr.*"
 
     gdb_test "print &main" " = .* \033\\\[34m$hex\033\\\[m <$main_expr>"
-- 
2.20.1

Re: [RFC 8.3 2/3] Add the "set style source" command

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Cc: Tom Tromey <tromey@adacore.com>
> Date: Fri,  8 Mar 2019 14:04:32 -0700
> 
> This adds "set style source" (and "show style source") commands.  This
> gives the user control over whether source code is highlighted.

Thanks.

> +set style sources [on|off]
> +show style sources
> +  Enable or disable source code styling.  Source code styling is
> +  enabled by default, but only takes effect if styling in general is
> +  enabled, if if GDB was linked with GNU Source Highlight.
              ^^^^^
A typo (you probably meant "and if").

Otherwise, the documentation parts are OK, thanks.

Re: [RFC 8.3 2/3] Add the "set style source" command

[Tom Tromey]

>> +  enabled, if if GDB was linked with GNU Source Highlight.
Eli>               ^^^^^
Eli> A typo (you probably meant "and if").

Thanks, I've fixed this.
New version to arrive soon.

Tom

Re: [RFC 8.3 2/3] Add the "set style source" command

[Philippe Waroquiers]

On Fri, 2019-03-08 at 14:04 -0700, Tom Tromey wrote:
> diff --git a/gdb/NEWS b/gdb/NEWS
> index cc7c35c0642..06564311643 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -145,6 +145,12 @@ show style enabled
>    Enable or disable terminal styling.  Styling is enabled by default
>    on most hosts, but disabled by default when in batch mode.
>  
> +set style sources [on|off]
> +show style sources
> +  Enable or disable source code styling.  Source code styling is
> +  enabled by default, but only takes effect if styling in general is
> +  enabled, if if GDB was linked with GNU Source Highlight.
As already reported by Eli : if if -> and if ?

Also, this NEWS entry explains that 'on' takes effect only
if styling is on, but neither the help nor the user manual
are explaining this.
Maybe the help and/or user manual should also explain this ?

Thanks

Philippe

Re: [RFC 8.3 2/3] Add the "set style source" command

[Tom Tromey]

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> Also, this NEWS entry explains that 'on' takes effect only
Philippe> if styling is on, but neither the help nor the user manual
Philippe> are explaining this.
Philippe> Maybe the help and/or user manual should also explain this ?

Seems reasonable, let me know what you think of the appended.

Tom

commit 82897b4c31ad6d65073c64529325830a9c70c65f
Author: Tom Tromey <tromey@adacore.com>
Date:   Fri Mar 8 13:54:07 2019 -0700

    Add the "set style source" command
    
    This adds "set style source" (and "show style source") commands.  This
    gives the user control over whether source code is highlighted.
    
    gdb/ChangeLog
    2019-03-08  Tom Tromey  <tromey@adacore.com>
    
            * NEWS: Add item for "style sources" commands.
            * source-cache.c (source_cache::get_source_lines): Check
            source_styling.
            * cli/cli-style.c (source_styling): New global.
            (_initialize_cli_style): Add "style sources" commands.
            (show_style_sources): New function.
            * cli/cli-style.h (source_styling): Declare.
    
    gdb/doc/ChangeLog
    2019-03-08  Tom Tromey  <tromey@adacore.com>
    
            * gdb.texinfo (Output Styling): Document "set style source" and
            "show style source".
    
    gdb/testsuite/ChangeLog
    2019-03-08  Tom Tromey  <tromey@adacore.com>
    
            * gdb.base/style.exp: Add "set style sources" test.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6d58321c0d6..efaa82f3a00 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2019-03-08  Tom Tromey  <tromey@adacore.com>
+
+	* NEWS: Add item for "style sources" commands.
+	* source-cache.c (source_cache::get_source_lines): Check
+	source_styling.
+	* cli/cli-style.c (source_styling): New global.
+	(_initialize_cli_style): Add "style sources" commands.
+	(show_style_sources): New function.
+	* cli/cli-style.h (source_styling): Declare.
+
 2019-03-08  Tom Tromey  <tromey@adacore.com>
 
 	* cli/cli-style.c (set_style_enabled): Notify new observable.
diff --git a/gdb/NEWS b/gdb/NEWS
index cc7c35c0642..3f465856cc4 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -145,6 +145,12 @@ show style enabled
   Enable or disable terminal styling.  Styling is enabled by default
   on most hosts, but disabled by default when in batch mode.
 
+set style sources [on|off]
+show style sources
+  Enable or disable source code styling.  Source code styling is
+  enabled by default, but only takes effect if styling in general is
+  enabled, and if GDB was linked with GNU Source Highlight.
+
 set style filename foreground COLOR
 set style filename background COLOR
 set style filename intensity VALUE
diff --git a/gdb/cli/cli-style.c b/gdb/cli/cli-style.c
index fc91504d1f8..f6f6c7be5d9 100644
--- a/gdb/cli/cli-style.c
+++ b/gdb/cli/cli-style.c
@@ -31,6 +31,11 @@ int cli_styling = 0;
 int cli_styling = 1;
 #endif
 
+/* True if source styling is enabled.  Note that this is only
+   consulted when cli_styling is true.  */
+
+int source_styling = 1;
+
 /* Name of colors; must correspond to ui_file_style::basic_color.  */
 static const char * const cli_colors[] = {
   "none",
@@ -230,6 +235,16 @@ show_style_enabled (struct ui_file *file, int from_tty,
     fprintf_filtered (file, _("CLI output styling is disabled.\n"));
 }
 
+static void
+show_style_sources (struct ui_file *file, int from_tty,
+		    struct cmd_list_element *c, const char *value)
+{
+  if (source_styling)
+    fprintf_filtered (file, _("Source code styling is enabled.\n"));
+  else
+    fprintf_filtered (file, _("Source code styling is disabled.\n"));
+}
+
 void
 _initialize_cli_style ()
 {
@@ -249,6 +264,20 @@ If enabled, output to the terminal is styled."),
 			   set_style_enabled, show_style_enabled,
 			   &style_set_list, &style_show_list);
 
+  add_setshow_boolean_cmd ("sources", no_class, &source_styling, _("\
+Set whether source code styling is enabled."), _("\
+Show whether source code styling is enabled."), _("\
+If enabled, source code is styled.\n"
+#ifdef HAVE_SOURCE_HIGHLIGHT
+"Note that source styling only works if styling in general is enabled,\n\
+see \"show style enabled\"."
+#else
+"Source highlighting is disabled in this installation of gdb, because\n\
+it was not linked against GNU Source Highlight."
+#endif
+			   ), set_style_enabled, show_style_sources,
+			   &style_set_list, &style_show_list);
+
 #define STYLE_ADD_SETSHOW_COMMANDS(STYLE, NAME, PREFIX_DOC)	  \
   STYLE.add_setshow_commands (NAME, no_class, PREFIX_DOC,		\
 			      &style_set_list,				\
diff --git a/gdb/cli/cli-style.h b/gdb/cli/cli-style.h
index 287bad7d201..c0520f9e23f 100644
--- a/gdb/cli/cli-style.h
+++ b/gdb/cli/cli-style.h
@@ -93,6 +93,9 @@ extern cli_style_option variable_name_style;
 /* The address style.  */
 extern cli_style_option address_style;
 
+/* True if source styling is enabled.  */
+extern int source_styling;
+
 /* True if styling is enabled.  */
 extern int cli_styling;
 
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 0380322dfee..1f10b346866 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,8 @@
+2019-03-08  Tom Tromey  <tromey@adacore.com>
+
+	* gdb.texinfo (Output Styling): Document "set style source" and
+	"show style source".
+
 2019-03-05  Simon Marchi  <simon.marchi@efficios.com>
 
 	* python.texi (Values From Inferior): Change synopsys of the
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index f2028f86b0d..27c996b33f9 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -24525,6 +24525,16 @@ most hosts defaulting to @samp{on}.
 
 @item show style enabled
 Show the current state of styling.
+
+@item set style sources @samp{on|off}
+Enable or disable source code styling.  This affects whether source
+code, such as the output of the @code{list} command, is styled.  Note
+that source styling only works if styling in general is enabled, and
+if @value{GDBN} was linked with the GNU Source Highlight library.  The
+default is @samp{on}.
+
+@item show style sources
+Show the current state of source code styling.
 @end table
 
 Subcommands of @code{set style} control specific forms of styling.
diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 097c8a3ae12..27a0ade959c 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -174,7 +174,7 @@ source_cache::get_source_lines (struct symtab *s, int first_line,
     return false;
 
 #ifdef HAVE_SOURCE_HIGHLIGHT
-  if (can_emit_style_escape (gdb_stdout))
+  if (source_styling && can_emit_style_escape (gdb_stdout))
     {
       const char *fullname = symtab_to_fullname (s);
 
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 57f80b49b59..d75c2386fdb 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2019-03-08  Tom Tromey  <tromey@adacore.com>
+
+	* gdb.base/style.exp: Add "set style sources" test.
+
 2019-03-06  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* gdb.fortran/type-kinds.exp: Extend to cover TYPE*SIZE cases.
diff --git a/gdb/testsuite/gdb.base/style.exp b/gdb/testsuite/gdb.base/style.exp
index 2778001fed5..369c1f59a88 100644
--- a/gdb/testsuite/gdb.base/style.exp
+++ b/gdb/testsuite/gdb.base/style.exp
@@ -50,6 +50,12 @@ save_vars { env(TERM) } {
 	"$main_expr.*$arg_expr.*$arg_expr.*$file_expr.*"
     gdb_test "info breakpoints" "$main_expr at $file_expr.*"
 
+    gdb_test_no_output "set style sources off"
+    gdb_test "frame" \
+	"\r\n\[^\033\]*break here.*" \
+	"frame without styling"
+    gdb_test_no_output "set style sources on"
+
     gdb_test "break main" "file $base_file_expr.*"
 
     gdb_test "print &main" " = .* \033\\\[34m$hex\033\\\[m <$main_expr>"

Re: [RFC 8.3 2/3] Add the "set style source" command

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Cc: Tom Tromey <tromey@adacore.com>,  gdb-patches@sourceware.org
> Date: Mon, 11 Mar 2019 14:13:52 -0600
> 
> Philippe> Also, this NEWS entry explains that 'on' takes effect only
> Philippe> if styling is on, but neither the help nor the user manual
> Philippe> are explaining this.
> Philippe> Maybe the help and/or user manual should also explain this ?
> 
> Seems reasonable, let me know what you think of the appended.

LGTM, thanks.

[RFC 8.3 3/3] Avoid a crash in source_cache::extract_lines

[Tom Tromey]

If the first requested line is larger than the number of lines in the
source buffer, source_cache::extract_lines could crash, because it
would try to pass string::npos" to string::substr.

This patch avoids the crash by checking for this case.

gdb/ChangeLog
2019-03-08  Tom Tromey  <tromey@adacore.com>

	* source-cache.c (source_cache::extract_lines): Handle case where
	first_pos==npos.
---
 gdb/ChangeLog      | 5 +++++
 gdb/source-cache.c | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 27a0ade959c..b5d0d6cb7fc 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -98,6 +98,8 @@ source_cache::extract_lines (const struct source_text &text, int first_line,
 	{
 	  if (pos == std::string::npos)
 	    pos = text.contents.size ();
+	  if (first_pos == std::string::npos)
+	    first_pos = text.contents.size ();
 	  *lines = text.contents.substr (first_pos, pos - first_pos);
 	  return true;
 	}
-- 
2.20.1

Re: [RFC 8.3 3/3] Avoid a crash in source_cache::extract_lines

[Pedro Alves]

On 03/08/2019 09:04 PM, Tom Tromey wrote:
> If the first requested line is larger than the number of lines in the
> source buffer, source_cache::extract_lines could crash, because it
> would try to pass string::npos" to string::substr.
> 
> This patch avoids the crash by checking for this case.

Can you clarify how can first_pos end up as npos?  Is that a bug in the
caller, or is it normal?  The documentation doesn't seem to allow for that:

  /* Get the source text for the source file in symtab S.  FIRST_LINE
     and LAST_LINE are the first and last lines to return; line
     numbers are 1-based.  If the file cannot be read, false is
     returned.  Otherwise, LINES_OUT is set to the desired text.  The
     returned text may include ANSI terminal escapes.  */

> 
> gdb/ChangeLog
> 2019-03-08  Tom Tromey  <tromey@adacore.com>
> 
> 	* source-cache.c (source_cache::extract_lines): Handle case where
> 	first_pos==npos.
> ---
>  gdb/ChangeLog      | 5 +++++
>  gdb/source-cache.c | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> index 27a0ade959c..b5d0d6cb7fc 100644
> --- a/gdb/source-cache.c
> +++ b/gdb/source-cache.c
> @@ -98,6 +98,8 @@ source_cache::extract_lines (const struct source_text &text, int first_line,
>  	{
>  	  if (pos == std::string::npos)
>  	    pos = text.contents.size ();
> +	  if (first_pos == std::string::npos)
> +	    first_pos = text.contents.size ();
>  	  *lines = text.contents.substr (first_pos, pos - first_pos);
>  	  return true;
>  	}
> 

Thanks,
Pedro Alves

Re: [RFC 8.3 3/3] Avoid a crash in source_cache::extract_lines

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Can you clarify how can first_pos end up as npos?  Is that a bug in the
Pedro> caller, or is it normal?  The documentation doesn't seem to allow for that:

Pedro>   /* Get the source text for the source file in symtab S.  FIRST_LINE
Pedro>      and LAST_LINE are the first and last lines to return; line
Pedro>      numbers are 1-based.  If the file cannot be read, false is
Pedro>      returned.  Otherwise, LINES_OUT is set to the desired text.  The
Pedro>      returned text may include ANSI terminal escapes.  */

I think you're just confusing first_pos and first_line here.
first_pos is a local variable that's used to track the position where
the first line starts:

  std::string::size_type first_pos = std::string::npos;
[...]
      if (lineno == first_line)
	first_pos = pos;

It can still be npos if first_line is greater than the number of lines
in the file.

Tom

Re: [RFC 8.3 3/3] Avoid a crash in source_cache::extract_lines

[Pedro Alves]

On 03/13/2019 05:20 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Can you clarify how can first_pos end up as npos?  Is that a bug in the
> Pedro> caller, or is it normal?  The documentation doesn't seem to allow for that:
> 
> Pedro>   /* Get the source text for the source file in symtab S.  FIRST_LINE
> Pedro>      and LAST_LINE are the first and last lines to return; line
> Pedro>      numbers are 1-based.  If the file cannot be read, false is
> Pedro>      returned.  Otherwise, LINES_OUT is set to the desired text.  The
> Pedro>      returned text may include ANSI terminal escapes.  */
> 
> I think you're just confusing first_pos and first_line here.
> first_pos is a local variable that's used to track the position where
> the first line starts:
> 
>   std::string::size_type first_pos = std::string::npos;
> [...]
>       if (lineno == first_line)
> 	first_pos = pos;
> 
> It can still be npos if first_line is greater than the number of lines
> in the file.

Oh, I see now, now that I actually look at the code, rather than just the
patch.  Sorry about that.  And now that I look, I admit it took me a bit
to grok the function, but I got it.

IIUC, the function can never really return false, right?  Since
get_source_lines already validates input.  If you made extract_lines
return std::string instead of using an output parameter, then you
could conveniently write:

	  if (first_pos == std::string::npos)
	    return {};

for this case, which might be a little clearer than the
resulting "npos - npos" with your patch.

Anyhow, not that important.

Patch LGTM.

Thanks,
Pedro Alves

Re: [RFC 8.3 3/3] Avoid a crash in source_cache::extract_lines

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> IIUC, the function can never really return false, right?  Since
Pedro> get_source_lines already validates input.  If you made extract_lines
Pedro> return std::string instead of using an output parameter,

I went ahead and made this change.

Tom

[RFC 8.3 1/3] Make TUI react to "set style enabled"

[Tom Tromey]

When the user toggles "set style enabled", the TUI should react by
redrawing the source window, if necessary.  This patch implements this
behavior.

No test because the TUI is generally not tested.

I was not sure of a clean way to force an update of the window's
contents; see the new tui_redisplay_source for the workaround I used.

gdb/ChangeLog
2019-03-08  Tom Tromey  <tromey@adacore.com>

	* cli/cli-style.c (set_style_enabled): Notify new observable.
	* tui/tui-hooks.c (tui_redisplay_source): New function.
	(tui_attach_detach_observers): Attach or detach
	tui_redisplay_source.
	* observable.h (source_styling_changed): New observable.
	* observable.c: Define source_styling_changed observable.
---
 gdb/ChangeLog       |  9 +++++++++
 gdb/cli/cli-style.c |  2 ++
 gdb/observable.c    |  1 +
 gdb/observable.h    |  4 ++++
 gdb/tui/tui-hooks.c | 14 ++++++++++++++
 5 files changed, 30 insertions(+)

diff --git a/gdb/cli/cli-style.c b/gdb/cli/cli-style.c
index 234cc410dda..729a024c47a 100644
--- a/gdb/cli/cli-style.c
+++ b/gdb/cli/cli-style.c
@@ -21,6 +21,7 @@
 #include "cli/cli-cmds.h"
 #include "cli/cli-style.h"
 #include "source-cache.h"
+#include "observable.h"
 
 /* True if styling is enabled.  */
 
@@ -216,6 +217,7 @@ static void
 set_style_enabled  (const char *args, int from_tty, struct cmd_list_element *c)
 {
   g_source_cache.clear ();
+  gdb::observers::source_styling_changed.notify ();
 }
 
 static void
diff --git a/gdb/observable.c b/gdb/observable.c
index 33112062424..c077b025932 100644
--- a/gdb/observable.c
+++ b/gdb/observable.c
@@ -74,6 +74,7 @@ DEFINE_OBSERVABLE (inferior_call_pre);
 DEFINE_OBSERVABLE (inferior_call_post);
 DEFINE_OBSERVABLE (register_changed);
 DEFINE_OBSERVABLE (user_selected_context_changed);
+DEFINE_OBSERVABLE (source_styling_changed);
 
 } /* namespace observers */
 } /* namespace gdb */
diff --git a/gdb/observable.h b/gdb/observable.h
index 999ecfbc353..edd1fffae0a 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -228,6 +228,10 @@ extern observable<struct frame_info *, int> register_changed;
    frame has changed.  */
 extern observable<user_selected_what> user_selected_context_changed;
 
+/* This is notified when the source styling setting has changed and
+   should be reconsulted.  */
+extern observable<> source_styling_changed;
+
 } /* namespace observers */
 
 } /* namespace gdb */
diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index 98c6fd651fa..162d6ef525a 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -204,6 +204,18 @@ tui_normal_stop (struct bpstats *bs, int print_frame)
   tui_refresh_frame_and_register_information (/*registers_too_p=*/1);
 }
 
+/* Observer for source_cache_cleared.  */
+
+static void
+tui_redisplay_source ()
+{
+  if (tui_is_window_visible (SRC_WIN))
+    {
+      /* Force redisplay.  */
+      tui_horizontal_source_scroll (tui_win_list[SRC_WIN], LEFT_SCROLL, 0);
+    }
+}
+
 /* Token associated with observers registered while TUI hooks are
    installed.  */
 static const gdb::observers::token tui_observers_token {};
@@ -239,6 +251,8 @@ tui_attach_detach_observers (bool attach)
 		    tui_normal_stop, attach);
   attach_or_detach (gdb::observers::register_changed,
 		    tui_register_changed, attach);
+  attach_or_detach (gdb::observers::source_styling_changed,
+		    tui_redisplay_source, attach);
 }
 
 /* Install the TUI specific hooks.  */
-- 
2.20.1

Re: [RFC 8.3 1/3] Make TUI react to "set style enabled"

[Pedro Alves]

On 03/08/2019 09:04 PM, Tom Tromey wrote:
> When the user toggles "set style enabled", the TUI should react by
> redrawing the source window, if necessary.  This patch implements this
> behavior.
> 
> No test because the TUI is generally not tested.
> 
> I was not sure of a clean way to force an update of the window's
> contents; see the new tui_redisplay_source for the workaround I used.

I went looking, and thought that it could be:

  tui_show_source_content (tui_win_list[SRC_WIN]);

That's what tui_refresh_all_win, coming from Ctrl-L, does.

But that won't work I think because we'd just refresh the current
source code cache, which includes the previous/now-stale
highlighting mode.

Maybe something like this?

From 5b7c86c6666ddc16b206fe04a492e99a33570a7f Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 13 Mar 2019 18:14:14 +0000
Subject: [PATCH] refactor

---
 gdb/tui/tui-hooks.c     |  2 +-
 gdb/tui/tui-winsource.c | 44 +++++++++++++++++++++++++-------------------
 gdb/tui/tui-winsource.h |  1 +
 3 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index 162d6ef525..4a1d79e0ad 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -212,7 +212,7 @@ tui_redisplay_source ()
   if (tui_is_window_visible (SRC_WIN))
     {
       /* Force redisplay.  */
-      tui_horizontal_source_scroll (tui_win_list[SRC_WIN], LEFT_SCROLL, 0);
+      tui_refill_source_window (tui_win_list[SRC_WIN]);
     }
 }
 
diff --git a/gdb/tui/tui-winsource.c b/gdb/tui/tui-winsource.c
index 9336f7b1f7..ab7756c1c4 100644
--- a/gdb/tui/tui-winsource.c
+++ b/gdb/tui/tui-winsource.c
@@ -306,8 +306,32 @@ tui_show_source_content (struct tui_win_info *win_info)
   win_info->generic.content_in_use = TRUE;
 }
 
+/* Refill the source window's source cache and update it.  If WIN_INFO
+   is a disassembly window, then just update it.  */
+
+void
+tui_refill_source_window (struct tui_win_info *win_info)
+{
+  symtab *s = nullptr;
+
+  if (win_info->generic.type == SRC_WIN)
+    {
+      symtab_and_line cursal = get_current_source_symtab_and_line ();
+      s = (cursal.symtab == NULL
+	   ? find_pc_line_symtab (get_frame_pc (get_selected_frame (NULL)))
+	   : cursal.symtab);
+    }
+
+  tui_update_source_window_as_is (win_info,
+				  win_info->detail.source_info.gdbarch,
+				  s,
+				  win_info->generic.content[0]
+				    ->which_element.source.line_or_addr,
+				  FALSE);
+}
 
 /* Scroll the source forward or backward horizontally.  */
+
 void
 tui_horizontal_source_scroll (struct tui_win_info *win_info,
 			      enum tui_scroll_direction direction,
@@ -315,20 +339,7 @@ tui_horizontal_source_scroll (struct tui_win_info *win_info,
 {
   if (win_info->generic.content != NULL)
     {
-      struct gdbarch *gdbarch = win_info->detail.source_info.gdbarch;
       int offset;
-      struct symtab *s = NULL;
-
-      if (win_info->generic.type == SRC_WIN)
-	{
-	  struct symtab_and_line cursal
-	    = get_current_source_symtab_and_line ();
-
-	  if (cursal.symtab == NULL)
-	    s = find_pc_line_symtab (get_frame_pc (get_selected_frame (NULL)));
-	  else
-	    s = cursal.symtab;
-	}
 
       if (direction == LEFT_SCROLL)
 	offset = win_info->detail.source_info.horizontal_offset
@@ -341,13 +352,8 @@ tui_horizontal_source_scroll (struct tui_win_info *win_info,
 	    offset = 0;
 	}
       win_info->detail.source_info.horizontal_offset = offset;
-      tui_update_source_window_as_is (win_info, gdbarch, s,
-				      win_info->generic.content[0]
-					->which_element.source.line_or_addr,
-				      FALSE);
+      tui_refill_source_window (win_info);
     }
-
-  return;
 }
 
 
diff --git a/gdb/tui/tui-winsource.h b/gdb/tui/tui-winsource.h
index db33a4f73e..920032b04e 100644
--- a/gdb/tui/tui-winsource.h
+++ b/gdb/tui/tui-winsource.h
@@ -56,6 +56,7 @@ extern void tui_show_source_content (struct tui_win_info *);
 extern void tui_horizontal_source_scroll (struct tui_win_info *,
 					  enum tui_scroll_direction, 
 					  int);
+extern void tui_refill_source_window (struct tui_win_info *);
 extern enum tui_status tui_set_exec_info_content (struct tui_win_info *);
 extern void tui_show_exec_info_content (struct tui_win_info *);
 extern void tui_erase_exec_info_content (struct tui_win_info *);
-- 
2.14.4

Re: [RFC 8.3 1/3] Make TUI react to "set style enabled"

[Tom Tromey]

Pedro> Maybe something like this?
[...]

Thanks.  I've incorporated this into the patch.

Tom

Re: [RFC 8.3 0/3] Some style fixes

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Date: Fri,  8 Mar 2019 14:04:30 -0700
> 
> This series fixes some of the TUI/styling regressions noted by Pedro.
> I would like to push this to the 8.3 branch as well.
> 
> Patch #2 is a new feature that Eli mentioned.  Since it's an addition
> to a feature that is also new in 8.3, it seemed reasonable to do at
> this time.
> 
> Let me know what you think.

Thanks.  I won't have time today to test the patch, but I will
certainly try tomorrow.

Re: [RFC 8.3 0/3] Some style fixes

[Eli Zaretskii]

> Date: Sat, 09 Mar 2019 08:17:27 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: gdb-patches@sourceware.org
> 
> > From: Tom Tromey <tromey@adacore.com>
> > Date: Fri,  8 Mar 2019 14:04:30 -0700
> > 
> > This series fixes some of the TUI/styling regressions noted by Pedro.
> > I would like to push this to the 8.3 branch as well.
> > 
> > Patch #2 is a new feature that Eli mentioned.  Since it's an addition
> > to a feature that is also new in 8.3, it seemed reasonable to do at
> > this time.
> > 
> > Let me know what you think.
> 
> Thanks.  I won't have time today to test the patch, but I will
> certainly try tomorrow.

Tried that now.  The new "set style sources" toggle works nicely,
thanks.  However, the artifacts I saw in the TUI display in the
pretest are still there.  In particular, the right-hand side of the
frame of the source window still gets overwritten when stepping
through the inferior's code, and it looks like the newline is not
echoed after commands typed in the command window.  E.g., here's what
I see after starting Emacs with "start -Q" and then typing "print 42":

     Type "apropos word" to search for commands related to "word"...
     Reading symbols from d:/usr/eli/emacs-master_2019-03-04/src/emacs.exe...
     (gdb) start -QTemporary breakpoint 1 at 0x114b4b7: file emacs.c, line 839.
     Starting program: d:\usr\eli\emacs-master_2019-03-04\src\emacs.exe -Q

     Temporary breakpoint 1, main (argc=2, argv=0x9c11a0) at emacs.c:839
     (gdb) p 42$1 = 42

As you see, the newlines after "start -Q" and "p 42" were not
output/acted upon, so the following text is displayed on the same
screen line.

Thanks.

Re: [RFC 8.3 0/3] Some style fixes

[Tom Tromey]

Eli> However, the artifacts I saw in the TUI display in the
Eli> pretest are still there.  In particular, the right-hand side of the
Eli> frame of the source window still gets overwritten when stepping
Eli> through the inferior's code, and it looks like the newline is not
Eli> echoed after commands typed in the command window.

I wonder if this is the "nonl" bug.
You can test that theory by applying the appended.

Tom

diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index d006e41cabb..b25b55b8515 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -606,15 +606,15 @@ tui_mld_beep (const struct match_list_displayer *displayer)
 static int
 gdb_wgetch (WINDOW *win)
 {
-  nonl ();
+  /* nonl (); */
   int r = wgetch (win);
-  nl ();
+  /* nl (); */
   /* In nonl mode, if the user types Enter, it will not be echoed
      properly.  This will result in gdb output appearing immediately
      after the command.  So, if we read \r, emit a \r now, after nl
      mode has been re-entered, so that the output looks correct.  */
-  if (r == '\r')
-    puts ("\r");
+  /* if (r == '\r') */
+  /*   puts ("\r"); */
   return r;
 }
 

Re: [RFC 8.3 0/3] Some style fixes

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Cc: tromey@adacore.com,  gdb-patches@sourceware.org
> Date: Mon, 11 Mar 2019 14:15:36 -0600
> 
> Eli> However, the artifacts I saw in the TUI display in the
> Eli> pretest are still there.  In particular, the right-hand side of the
> Eli> frame of the source window still gets overwritten when stepping
> Eli> through the inferior's code, and it looks like the newline is not
> Eli> echoed after commands typed in the command window.
> 
> I wonder if this is the "nonl" bug.
> You can test that theory by applying the appended.

It fixes the problems in the command window, so I think we should
apply this if no better ideas are brought up.

But the problems in the source window are not solved by this patch,
they have other reasons.  I debugged almost all of the problems I'm
aware of, please see the annotated patches below.  If they look
reasonable, I will push them.

After these patches, I have only one more problem: horizontal
scrolling of the source window.  First, the first press of left-arrow
doesn't have any effect; and second, scrolling draws TAB characters
incorrectly, it treats each TAB as if it took a single column on the
screen, so text after a TAB jumps by 8 columns when you scroll instead
of moving by one column.  I hope to have solution for this tomorrow.

Here are the patches for the other problems:

1. The first patch fixes the problem noticed by Pedro: pressing DOWN
arrow in the command window doesn't scroll the source window.  This is
because we don't initialize the s->nlines field, and then
tui_vertical_source_scroll thinks we are off the chart.  This fixes
that:

--- gdb/source-cache.c~4	2019-03-10 08:34:47.422752400 +0200
+++ gdb/source-cache.c	2019-03-12 11:50:15.094147600 +0200
@@ -194,6 +194,12 @@ source_cache::get_source_lines (struct s
 	  std::ifstream input (fullname);
 	  if (input.is_open ())
 	    {
+	      if (s->line_charpos == 0)
+		{
+		  scoped_fd desc = open_source_file (s);
+		  if (desc.get () >= 0)
+		    find_source_lines (s, desc.get ());
+		}
 	      srchilite::SourceHighlight highlighter ("esc.outlang");
 	      highlighter.setStyleFile("esc.style");
 
2. The fix for avoiding to overwrite the source window frame is
simple: revert 4a3045920.  I don't really understand why that change
was made: AFAIU, wclrtoeol clears to the end of the window line, and
cannot be told to clear only part of the line.

--- gdb/tui/tui-winsource.c~4	2019-02-27 06:51:50.000000000 +0200
+++ gdb/tui/tui-winsource.c	2019-03-12 10:57:02.052875200 +0200
@@ -285,7 +285,12 @@ tui_show_source_line (struct tui_win_inf
     wattroff (win_info->generic.handle, A_STANDOUT);
 
   /* Clear to end of line but stop before the border.  */
-  wclrtoeol (win_info->generic.handle);
+  int x = getcurx (win_info->generic.handle);
+  while (x + 1 < win_info->generic.width)
+    {
+      waddch (win_info->generic.handle, ' ');
+      x = getcurx (win_info->generic.handle);
+    }
 }
 
 void

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

Re: [RFC 8.3 0/3] Some style fixes

[Eli Zaretskii]

For the MinGW-specific problem with TUI colors, described here:

  https://www.sourceware.org/ml/gdb-patches/2019-03/msg00000.html
  https://www.sourceware.org/ml/gdb-patches/2019-03/msg00034.html

I propose to fix that as in the patch below.  Any objections to my
pushing this to the master and th gdb-8.3 branch (with suitable
ChangeLog entries)?

--- gdb/tui/tui-io.c~5	2019-03-12 09:18:11.713960000 +0200
+++ gdb/tui/tui-io.c	2019-03-13 09:09:15.185827600 +0200
@@ -37,6 +37,10 @@
 #include "cli-out.h"
 #include <fcntl.h>
 #include <signal.h>
+#ifdef __MINGW32__
+#include <windows.h>
+static SHORT ncurses_norm_attr;
+#endif
 #include "common/filestuff.h"
 #include "completer.h"
 #include "gdb_curses.h"
@@ -322,6 +326,16 @@ apply_ansi_escape (WINDOW *w, const char
       int fgi, bgi;
       if (get_color (fg, &fgi) && get_color (bg, &bgi))
 	{
+#ifdef __MINGW32__
+	  /* MS-Windows port of ncurses doesn't support implicit
+	     default foreground and background colors, so we must
+	     specify them explicitly when needed, using the colors we
+	     saw at startup.  */
+	  if (fgi == -1)
+	    fgi = ncurses_norm_attr & 15;
+	  if (bgi == -1)
+	    bgi = (ncurses_norm_attr >> 4) & 15;
+#endif
 	  int pair = get_color_pair (fgi, bgi);
 	  if (last_color_pair != -1)
 	    wattroff (w, COLOR_PAIR (last_color_pair));
@@ -803,6 +817,19 @@ tui_initialize_io (void)
 #else
   tui_rl_outstream = stdout;
 #endif
+
+#ifdef __MINGW32__
+  /* MS-Windows port of ncurses doesn't support default foreground and
+     background colors, so we must record the default colors at startup.  */
+  HANDLE hstdout = (HANDLE)_get_osfhandle (fileno (stdout));
+  DWORD cmode;
+  CONSOLE_SCREEN_BUFFER_INFO csbi;
+
+  if (hstdout != INVALID_HANDLE_VALUE
+      && GetConsoleMode (hstdout, &cmode) != 0
+      && GetConsoleScreenBufferInfo (hstdout, &csbi))
+    ncurses_norm_attr = csbi.wAttributes;
+#endif
 }
 
 /* Get a character from the command window.  This is called from the

Re: [RFC 8.3 0/3] Some style fixes

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> +#ifdef __MINGW32__
Eli> +#include <windows.h>
Eli> +static SHORT ncurses_norm_attr;
Eli> +#endif

I'd somewhat prefer it if the definition were in a separate block after
all the includes.

The rest seems fine to me.

Tom

Re: [RFC 8.3 0/3] Some style fixes

[Pedro Alves]

On 03/14/2019 12:20 PM, Tom Tromey wrote:
>>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> Eli> +#ifdef __MINGW32__
> Eli> +#include <windows.h>
> Eli> +static SHORT ncurses_norm_attr;
> Eli> +#endif
> 
> I'd somewhat prefer it if the definition were in a separate block after
> all the includes.
> 

Me too.  It would help with the "namespace gdb" effort as well,
since the variable will be braced under "namespace gdb" while
the header won't.

Thanks,
Pedro Alves

Re: [RFC 8.3 0/3] Some style fixes

[Eli Zaretskii]

> Cc: tromey@adacore.com, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 14 Mar 2019 14:40:37 +0000
> 
> On 03/14/2019 12:20 PM, Tom Tromey wrote:
> >>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > Eli> +#ifdef __MINGW32__
> > Eli> +#include <windows.h>
> > Eli> +static SHORT ncurses_norm_attr;
> > Eli> +#endif
> > 
> > I'd somewhat prefer it if the definition were in a separate block after
> > all the includes.
> 
> Me too.  It would help with the "namespace gdb" effort as well,
> since the variable will be braced under "namespace gdb" while
> the header won't.

Thanks for the review.  I made the requested change and pushed to both
branches.

Fix pressing down in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

[Pedro Alves]

On 03/12/2019 04:44 PM, Eli Zaretskii wrote:
> 1. The first patch fixes the problem noticed by Pedro: pressing DOWN
> arrow in the command window doesn't scroll the source window.  This is
> because we don't initialize the s->nlines field, and then
> tui_vertical_source_scroll thinks we are off the chart.  This fixes
> that:
> 
> --- gdb/source-cache.c~4	2019-03-10 08:34:47.422752400 +0200
> +++ gdb/source-cache.c	2019-03-12 11:50:15.094147600 +0200
> @@ -194,6 +194,12 @@ source_cache::get_source_lines (struct s
>  	  std::ifstream input (fullname);
>  	  if (input.is_open ())
>  	    {
> +	      if (s->line_charpos == 0)
> +		{
> +		  scoped_fd desc = open_source_file (s);
> +		  if (desc.get () >= 0)
> +		    find_source_lines (s, desc.get ());

I think this should return false if open_source_file fails?  I.e., like this:

	      if (s->line_charpos == 0)
		{
		  scoped_fd desc = open_source_file (s);
		  if (desc.get () < 0)
                    return false;
		  find_source_lines (s, desc.get ());

Otherwise this LGTM.  I see get_plain_source_lines has similar
code, so my immediate thought was to move that to a helper function,
but there's a difference that makes that unviable -- get_plain_source_lines
always wants to open the source file first, while here we can avoid
it unless line_charpos is 0.

Thanks,
Pedro Alves

Re: Fix pressing down in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 15 Mar 2019 12:34:34 +0000
> 
> On 03/12/2019 04:44 PM, Eli Zaretskii wrote:
> > 1. The first patch fixes the problem noticed by Pedro: pressing DOWN
> > arrow in the command window doesn't scroll the source window.  This is
> > because we don't initialize the s->nlines field, and then
> > tui_vertical_source_scroll thinks we are off the chart.  This fixes
> > that:
> > 
> > --- gdb/source-cache.c~4	2019-03-10 08:34:47.422752400 +0200
> > +++ gdb/source-cache.c	2019-03-12 11:50:15.094147600 +0200
> > @@ -194,6 +194,12 @@ source_cache::get_source_lines (struct s
> >  	  std::ifstream input (fullname);
> >  	  if (input.is_open ())
> >  	    {
> > +	      if (s->line_charpos == 0)
> > +		{
> > +		  scoped_fd desc = open_source_file (s);
> > +		  if (desc.get () >= 0)
> > +		    find_source_lines (s, desc.get ());
> 
> I think this should return false if open_source_file fails?

How could it fail if input.is_open returns non-zero?  Are you thinking
about some race, whereby something deletes the file after the
constructor for 'input' returns?

> Otherwise this LGTM.  I see get_plain_source_lines has similar
> code, so my immediate thought was to move that to a helper function,
> but there's a difference that makes that unviable -- get_plain_source_lines
> always wants to open the source file first, while here we can avoid
> it unless line_charpos is 0.

Right.  Also, I originally hoped std::ifstream will have a way to get
at the underlying file descriptor, but no such luck, AFAICT.

Re: Fix pressing down in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

[Pedro Alves]

On 03/15/2019 01:36 PM, Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Fri, 15 Mar 2019 12:34:34 +0000
>>
>> On 03/12/2019 04:44 PM, Eli Zaretskii wrote:
>>> 1. The first patch fixes the problem noticed by Pedro: pressing DOWN
>>> arrow in the command window doesn't scroll the source window.  This is
>>> because we don't initialize the s->nlines field, and then
>>> tui_vertical_source_scroll thinks we are off the chart.  This fixes
>>> that:
>>>
>>> --- gdb/source-cache.c~4	2019-03-10 08:34:47.422752400 +0200
>>> +++ gdb/source-cache.c	2019-03-12 11:50:15.094147600 +0200
>>> @@ -194,6 +194,12 @@ source_cache::get_source_lines (struct s
>>>  	  std::ifstream input (fullname);
>>>  	  if (input.is_open ())
>>>  	    {
>>> +	      if (s->line_charpos == 0)
>>> +		{
>>> +		  scoped_fd desc = open_source_file (s);
>>> +		  if (desc.get () >= 0)
>>> +		    find_source_lines (s, desc.get ());
>>
>> I think this should return false if open_source_file fails?
> 
> How could it fail if input.is_open returns non-zero?  Are you thinking
> about some race, whereby something deletes the file after the
> constructor for 'input' returns?

Yes, something like that.  The possibility is small, but non-zero
that it could fail.  And if it does, we end up with s->nlines
uninitialized again.

> 
>> Otherwise this LGTM.  I see get_plain_source_lines has similar
>> code, so my immediate thought was to move that to a helper function,
>> but there's a difference that makes that unviable -- get_plain_source_lines
>> always wants to open the source file first, while here we can avoid
>> it unless line_charpos is 0.
> 
> Right.  Also, I originally hoped std::ifstream will have a way to get
> at the underlying file descriptor, but no such luck, AFAICT.

Yeah, I believe GNU has a non-standard way for that, but it's not portable.
(And not worth the bother to #ifdef here for that, IMHO.

Thanks,
Pedro Alves

Re: Fix pressing down in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

[Eli Zaretskii]

> Cc: tromey@adacore.com, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 15 Mar 2019 13:56:39 +0000
> 
> >> I think this should return false if open_source_file fails?
> > 
> > How could it fail if input.is_open returns non-zero?  Are you thinking
> > about some race, whereby something deletes the file after the
> > constructor for 'input' returns?
> 
> Yes, something like that.  The possibility is small, but non-zero
> that it could fail.  And if it does, we end up with s->nlines
> uninitialized again.

OK, pushed with that change to both branches.

Thanks.

Re: Fix pressing down in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

[Simon Marchi]

On 2019-03-16 1:58 p.m., Eli Zaretskii wrote:
>> Cc: tromey@adacore.com, gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Fri, 15 Mar 2019 13:56:39 +0000
>>
>>>> I think this should return false if open_source_file fails?
>>>
>>> How could it fail if input.is_open returns non-zero?  Are you thinking
>>> about some race, whereby something deletes the file after the
>>> constructor for 'input' returns?
>>
>> Yes, something like that.  The possibility is small, but non-zero
>> that it could fail.  And if it does, we end up with s->nlines
>> uninitialized again.
> 
> OK, pushed with that change to both branches.
> 
> Thanks.

Hi all,

I get an AddressSanitizer failure, and bisecting points to this commit.

I simply "start" an executable, and there is a use-after-free happening when
trying to print the stop location.  See the dump below.

Simon

$ ./gdb -nx --data-directory=data-directory ./a.out
(gdb) start
Temporary breakpoint 1 at 0x1130: file test.cpp, line 12.
Starting program: /home/simark/build/binutils-gdb/gdb/a.out

Temporary breakpoint 1, main () at test.cpp:12
=================================================================
==26068==ERROR: AddressSanitizer: heap-use-after-free on address 0x6210003d4100 at pc 0x7fed89a34681 bp 0x7ffd8d185d80 sp 0x7ffd8d185528
READ of size 2 at 0x6210003d4100 thread T0
    #0 0x7fed89a34680 in __interceptor_strlen /build/gcc/src/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:301
    #1 0x55b6edf6c2f7 in std::char_traits<char>::length(char const*) /usr/include/c++/8.2.1/bits/char_traits.h:320
    #2 0x55b6edf6c9b2 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(char const*, std::allocator<char> const&) /usr/include/c++/8.2.1/bits/basic_string.h:516
    #3 0x55b6ef09121b in source_cache::get_source_lines(symtab*, int, int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) /home/simark/src/binutils-gdb/gdb/source-cache.c:214
    #4 0x55b6ef0a15cb in print_source_lines_base /home/simark/src/binutils-gdb/gdb/source.c:1340
    #5 0x55b6ef0a2045 in print_source_lines(symtab*, int, int, enum_flags<print_source_lines_flag>) /home/simark/src/binutils-gdb/gdb/source.c:1415
    #6 0x55b6ef112c87 in print_frame_info(frame_info*, int, print_what, int, int) /home/simark/src/binutils-gdb/gdb/stack.c:914
    #7 0x55b6ef10e90d in print_stack_frame(frame_info*, int, print_what, int) /home/simark/src/binutils-gdb/gdb/stack.c:180
    #8 0x55b6ee9592f8 in print_stop_location /home/simark/src/binutils-gdb/gdb/infrun.c:7853
    #9 0x55b6ee95948f in print_stop_event(ui_out*) /home/simark/src/binutils-gdb/gdb/infrun.c:7870
    #10 0x55b6ef34b962 in tui_on_normal_stop /home/simark/src/binutils-gdb/gdb/tui/tui-interp.c:98
    #11 0x55b6ee01a14d in std::_Function_handler<void (bpstats*, int), void (*)(bpstats*, int)>::_M_invoke(std::_Any_data const&, bpstats*&&, int&&) /usr/include/c++/8.2.1/bits/std_function.h:297
    #12 0x55b6ee965415 in std::function<void (bpstats*, int)>::operator()(bpstats*, int) const /usr/include/c++/8.2.1/bits/std_function.h:687
    #13 0x55b6ee962f1b in gdb::observers::observable<bpstats*, int>::notify(bpstats*, int) const /home/simark/src/binutils-gdb/gdb/common/observable.h:106
    #14 0x55b6ee95a6e7 in normal_stop() /home/simark/src/binutils-gdb/gdb/infrun.c:8142
    #15 0x55b6ee93f236 in fetch_inferior_event(void*) /home/simark/src/binutils-gdb/gdb/infrun.c:3782
    #16 0x55b6ee8f2641 in inferior_event_handler(inferior_event_type, void*) /home/simark/src/binutils-gdb/gdb/inf-loop.c:43
    #17 0x55b6eea2a1f0 in handle_target_event /home/simark/src/binutils-gdb/gdb/linux-nat.c:4358
    #18 0x55b6ee7045f1 in handle_file_event /home/simark/src/binutils-gdb/gdb/event-loop.c:733
    #19 0x55b6ee704e89 in gdb_wait_for_event /home/simark/src/binutils-gdb/gdb/event-loop.c:859
    #20 0x55b6ee7027b5 in gdb_do_one_event() /home/simark/src/binutils-gdb/gdb/event-loop.c:322
    #21 0x55b6ee702907 in start_event_loop() /home/simark/src/binutils-gdb/gdb/event-loop.c:371
    #22 0x55b6eeadfc16 in captured_command_loop /home/simark/src/binutils-gdb/gdb/main.c:331
    #23 0x55b6eeae2ef9 in captured_main /home/simark/src/binutils-gdb/gdb/main.c:1174
    #24 0x55b6eeae30c2 in gdb_main(captured_main_args*) /home/simark/src/binutils-gdb/gdb/main.c:1190
    #25 0x55b6edf4fa89 in main /home/simark/src/binutils-gdb/gdb/gdb.c:32
    #26 0x7fed88ad8222 in __libc_start_main (/usr/lib/libc.so.6+0x24222)
    #27 0x55b6edf4f86d in _start (/home/simark/build/binutils-gdb/gdb/gdb+0x197186d)

0x6210003d4100 is located 0 bytes inside of 4096-byte region [0x6210003d4100,0x6210003d5100)
freed by thread T0 here:
    #0 0x7fed89a8ac19 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:66
    #1 0x55b6edfe12df in xfree<char> /home/simark/src/binutils-gdb/gdb/common/common-utils.h:60
    #2 0x55b6edfea675 in gdb::xfree_deleter<char>::operator()(char*) const /home/simark/src/binutils-gdb/gdb/common/gdb_unique_ptr.h:34
    #3 0x55b6edfe532c in std::unique_ptr<char, gdb::xfree_deleter<char> >::reset(char*) /usr/include/c++/8.2.1/bits/unique_ptr.h:382
    #4 0x55b6edfe7329 in std::unique_ptr<char, gdb::xfree_deleter<char> >::operator=(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) /usr/include/c++/8.2.1/bits/unique_ptr.h:289
    #5 0x55b6ef09ec2b in find_and_open_source(char const*, char const*, std::unique_ptr<char, gdb::xfree_deleter<char> >*) /home/simark/src/binutils-gdb/gdb/source.c:990
    #6 0x55b6ef09f56a in open_source_file(symtab*) /home/simark/src/binutils-gdb/gdb/source.c:1069
    #7 0x55b6ef090f78 in source_cache::get_source_lines(symtab*, int, int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) /home/simark/src/binutils-gdb/gdb/source-cache.c:205
    #8 0x55b6ef0a15cb in print_source_lines_base /home/simark/src/binutils-gdb/gdb/source.c:1340
    #9 0x55b6ef0a2045 in print_source_lines(symtab*, int, int, enum_flags<print_source_lines_flag>) /home/simark/src/binutils-gdb/gdb/source.c:1415
    #10 0x55b6ef112c87 in print_frame_info(frame_info*, int, print_what, int, int) /home/simark/src/binutils-gdb/gdb/stack.c:914
    #11 0x55b6ef10e90d in print_stack_frame(frame_info*, int, print_what, int) /home/simark/src/binutils-gdb/gdb/stack.c:180
    #12 0x55b6ee9592f8 in print_stop_location /home/simark/src/binutils-gdb/gdb/infrun.c:7853
    #13 0x55b6ee95948f in print_stop_event(ui_out*) /home/simark/src/binutils-gdb/gdb/infrun.c:7870
    #14 0x55b6ef34b962 in tui_on_normal_stop /home/simark/src/binutils-gdb/gdb/tui/tui-interp.c:98
    #15 0x55b6ee01a14d in std::_Function_handler<void (bpstats*, int), void (*)(bpstats*, int)>::_M_invoke(std::_Any_data const&, bpstats*&&, int&&) /usr/include/c++/8.2.1/bits/std_function.h:297
    #16 0x55b6ee965415 in std::function<void (bpstats*, int)>::operator()(bpstats*, int) const /usr/include/c++/8.2.1/bits/std_function.h:687
    #17 0x55b6ee962f1b in gdb::observers::observable<bpstats*, int>::notify(bpstats*, int) const /home/simark/src/binutils-gdb/gdb/common/observable.h:106
    #18 0x55b6ee95a6e7 in normal_stop() /home/simark/src/binutils-gdb/gdb/infrun.c:8142
    #19 0x55b6ee93f236 in fetch_inferior_event(void*) /home/simark/src/binutils-gdb/gdb/infrun.c:3782
    #20 0x55b6ee8f2641 in inferior_event_handler(inferior_event_type, void*) /home/simark/src/binutils-gdb/gdb/inf-loop.c:43
    #21 0x55b6eea2a1f0 in handle_target_event /home/simark/src/binutils-gdb/gdb/linux-nat.c:4358
    #22 0x55b6ee7045f1 in handle_file_event /home/simark/src/binutils-gdb/gdb/event-loop.c:733
    #23 0x55b6ee704e89 in gdb_wait_for_event /home/simark/src/binutils-gdb/gdb/event-loop.c:859
    #24 0x55b6ee7027b5 in gdb_do_one_event() /home/simark/src/binutils-gdb/gdb/event-loop.c:322
    #25 0x55b6ee702907 in start_event_loop() /home/simark/src/binutils-gdb/gdb/event-loop.c:371
    #26 0x55b6eeadfc16 in captured_command_loop /home/simark/src/binutils-gdb/gdb/main.c:331
    #27 0x55b6eeae2ef9 in captured_main /home/simark/src/binutils-gdb/gdb/main.c:1174
    #28 0x55b6eeae30c2 in gdb_main(captured_main_args*) /home/simark/src/binutils-gdb/gdb/main.c:1190
    #29 0x55b6edf4fa89 in main /home/simark/src/binutils-gdb/gdb/gdb.c:32

previously allocated by thread T0 here:
    #0 0x7fed89a8b019 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:86
    #1 0x7fed88af983f in realpath@@GLIBC_2.3 (/usr/lib/libc.so.6+0x4583f)
    #2 0x7fed899dbbbc in __interceptor_canonicalize_file_name /build/gcc/src/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:3297
    #3 0x55b6ee376a03 in gdb_realpath(char const*) /home/simark/src/binutils-gdb/gdb/common/pathstuff.c:72
    #4 0x55b6ef09ec12 in find_and_open_source(char const*, char const*, std::unique_ptr<char, gdb::xfree_deleter<char> >*) /home/simark/src/binutils-gdb/gdb/source.c:990
    #5 0x55b6ef09f56a in open_source_file(symtab*) /home/simark/src/binutils-gdb/gdb/source.c:1069
    #6 0x55b6ef0a0f12 in print_source_lines_base /home/simark/src/binutils-gdb/gdb/source.c:1270
    #7 0x55b6ef0a2045 in print_source_lines(symtab*, int, int, enum_flags<print_source_lines_flag>) /home/simark/src/binutils-gdb/gdb/source.c:1415
    #8 0x55b6ef112c87 in print_frame_info(frame_info*, int, print_what, int, int) /home/simark/src/binutils-gdb/gdb/stack.c:914
    #9 0x55b6ef10e90d in print_stack_frame(frame_info*, int, print_what, int) /home/simark/src/binutils-gdb/gdb/stack.c:180
    #10 0x55b6ee9592f8 in print_stop_location /home/simark/src/binutils-gdb/gdb/infrun.c:7853
    #11 0x55b6ee95948f in print_stop_event(ui_out*) /home/simark/src/binutils-gdb/gdb/infrun.c:7870
    #12 0x55b6ef34b962 in tui_on_normal_stop /home/simark/src/binutils-gdb/gdb/tui/tui-interp.c:98
    #13 0x55b6ee01a14d in std::_Function_handler<void (bpstats*, int), void (*)(bpstats*, int)>::_M_invoke(std::_Any_data const&, bpstats*&&, int&&) /usr/include/c++/8.2.1/bits/std_function.h:297
    #14 0x55b6ee965415 in std::function<void (bpstats*, int)>::operator()(bpstats*, int) const /usr/include/c++/8.2.1/bits/std_function.h:687
    #15 0x55b6ee962f1b in gdb::observers::observable<bpstats*, int>::notify(bpstats*, int) const /home/simark/src/binutils-gdb/gdb/common/observable.h:106
    #16 0x55b6ee95a6e7 in normal_stop() /home/simark/src/binutils-gdb/gdb/infrun.c:8142
    #17 0x55b6ee93f236 in fetch_inferior_event(void*) /home/simark/src/binutils-gdb/gdb/infrun.c:3782
    #18 0x55b6ee8f2641 in inferior_event_handler(inferior_event_type, void*) /home/simark/src/binutils-gdb/gdb/inf-loop.c:43
    #19 0x55b6eea2a1f0 in handle_target_event /home/simark/src/binutils-gdb/gdb/linux-nat.c:4358
    #20 0x55b6ee7045f1 in handle_file_event /home/simark/src/binutils-gdb/gdb/event-loop.c:733
    #21 0x55b6ee704e89 in gdb_wait_for_event /home/simark/src/binutils-gdb/gdb/event-loop.c:859
    #22 0x55b6ee7027b5 in gdb_do_one_event() /home/simark/src/binutils-gdb/gdb/event-loop.c:322
    #23 0x55b6ee702907 in start_event_loop() /home/simark/src/binutils-gdb/gdb/event-loop.c:371
    #24 0x55b6eeadfc16 in captured_command_loop /home/simark/src/binutils-gdb/gdb/main.c:331
    #25 0x55b6eeae2ef9 in captured_main /home/simark/src/binutils-gdb/gdb/main.c:1174
    #26 0x55b6eeae30c2 in gdb_main(captured_main_args*) /home/simark/src/binutils-gdb/gdb/main.c:1190
    #27 0x55b6edf4fa89 in main /home/simark/src/binutils-gdb/gdb/gdb.c:32
    #28 0x7fed88ad8222 in __libc_start_main (/usr/lib/libc.so.6+0x24222)

Re: Fix pressing down in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

[Simon Marchi]

On 2019-03-24 11:35 a.m., Simon Marchi wrote:
> Hi all,
> 
> I get an AddressSanitizer failure, and bisecting points to this commit.
> 
> I simply "start" an executable, and there is a use-after-free happening when
> trying to print the stop location.  See the dump below.

I investigated quickly, here's what I found.  We first get the symtab's fullname
with

  const char *fullname = symtab_to_fullname (s);

fullname essentially is the same as s->fullname.

The call to open_source_file that was added by this patch deallocates s->fullname
and replaces it with a new value (if though it may be an identical string).  When
we pass fullname (the local variable) to ighlighter.highlight, it still points to
now free'd memory.

The obvious patch would be to fetch fullname again after calling open_source_file,
like so:

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 9211f098eb70..ac97d79cdb31 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -206,6 +206,8 @@ source_cache::get_source_lines (struct symtab *s, int first_line,
 		  if (desc.get () < 0)
 		    return false;
 		  find_source_lines (s, desc.get ());
+
+		  fullname = symtab_to_fullname (s);
 		}
 	      srchilite::SourceHighlight highlighter ("esc.outlang");
 	      highlighter.setStyleFile("esc.style");


... but maybe there's a better way?  Should we instead create a local copy of FULLNAME?

Simon

Re: Fix pressing down in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> The obvious patch would be to fetch fullname again after calling open_source_file,
Simon> like so:

I think this is fine, especially with a comment explaining it.

Simon> ... but maybe there's a better way?  Should we instead create a
Simon> local copy of FULLNAME?

Ideally we'd rewrite this whole area.  I don't much like stashing the
full name in the symtab, and also it seems to me that gdb calls open,
etc, far too much.

Tom

Re: Fix pressing down in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

[Simon Marchi]

On 2019-03-25 11:14 a.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> The obvious patch would be to fetch fullname again after calling open_source_file,
> Simon> like so:
> 
> I think this is fine, especially with a comment explaining it.
> 
> Simon> ... but maybe there's a better way?  Should we instead create a
> Simon> local copy of FULLNAME?
> 
> Ideally we'd rewrite this whole area.  I don't much like stashing the
> full name in the symtab, and also it seems to me that gdb calls open,
> etc, far too much.
> 
> Tom
> 

Thanks, I have pushed the following patch to master and gdb-8.3-branch.

From 154eab6dec772e705b4eefe34ebfefec8808c2cf Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Mon, 25 Mar 2019 20:29:18 -0400
Subject: [PATCH] Fix use-after-free in source_cache::get_source_lines

Commit ab42892fb7d2 ("Fix vertical scrolling of TUI source window")
introduced a use-after-free in source_cache::get_source_lines.

At the beginning of the method, we get the fullname of the symtab:

    const char *fullname = symtab_to_fullname (s);

fullname points to the string owned by the symtab (s.fullname).  When we
later do

    scoped_fd desc = open_source_file (s);

s.fullname gets reallocated (even though the string contents may not
change).  The fullname local variable now points to freed memory.

To avoid it, refresh the value of fullname after calling
open_source_file.

Here is the ASan report:

$ ./gdb -nx --data-directory=data-directory ./a.out
(gdb) start
Temporary breakpoint 1 at 0x1130: file test.cpp, line 12.
Starting program: /home/simark/build/binutils-gdb/gdb/a.out

Temporary breakpoint 1, main () at test.cpp:12
=================================================================
==26068==ERROR: AddressSanitizer: heap-use-after-free on address 0x6210003d4100 at pc 0x7fed89a34681 bp 0x7ffd8d185d80 sp 0x7ffd8d185528
READ of size 2 at 0x6210003d4100 thread T0
    #0 0x7fed89a34680 in __interceptor_strlen /build/gcc/src/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:301
    #1 0x55b6edf6c2f7 in std::char_traits<char>::length(char const*) /usr/include/c++/8.2.1/bits/char_traits.h:320
    #2 0x55b6edf6c9b2 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(char const*, std::allocator<char> const&) /usr/include/c++/8.2.1/bits/basic_string.h:516
    #3 0x55b6ef09121b in source_cache::get_source_lines(symtab*, int, int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) /home/simark/src/binutils-gdb/gdb/source-cache.c:214
    #4 0x55b6ef0a15cb in print_source_lines_base /home/simark/src/binutils-gdb/gdb/source.c:1340
    #5 0x55b6ef0a2045 in print_source_lines(symtab*, int, int, enum_flags<print_source_lines_flag>) /home/simark/src/binutils-gdb/gdb/source.c:1415
    #6 0x55b6ef112c87 in print_frame_info(frame_info*, int, print_what, int, int) /home/simark/src/binutils-gdb/gdb/stack.c:914
    #7 0x55b6ef10e90d in print_stack_frame(frame_info*, int, print_what, int) /home/simark/src/binutils-gdb/gdb/stack.c:180
    #8 0x55b6ee9592f8 in print_stop_location /home/simark/src/binutils-gdb/gdb/infrun.c:7853
    #9 0x55b6ee95948f in print_stop_event(ui_out*) /home/simark/src/binutils-gdb/gdb/infrun.c:7870
    #10 0x55b6ef34b962 in tui_on_normal_stop /home/simark/src/binutils-gdb/gdb/tui/tui-interp.c:98
    #11 0x55b6ee01a14d in std::_Function_handler<void (bpstats*, int), void (*)(bpstats*, int)>::_M_invoke(std::_Any_data const&, bpstats*&&, int&&) /usr/include/c++/8.2.1/bits/std_function.h:297
    #12 0x55b6ee965415 in std::function<void (bpstats*, int)>::operator()(bpstats*, int) const /usr/include/c++/8.2.1/bits/std_function.h:687
    #13 0x55b6ee962f1b in gdb::observers::observable<bpstats*, int>::notify(bpstats*, int) const /home/simark/src/binutils-gdb/gdb/common/observable.h:106
    #14 0x55b6ee95a6e7 in normal_stop() /home/simark/src/binutils-gdb/gdb/infrun.c:8142
    #15 0x55b6ee93f236 in fetch_inferior_event(void*) /home/simark/src/binutils-gdb/gdb/infrun.c:3782
    #16 0x55b6ee8f2641 in inferior_event_handler(inferior_event_type, void*) /home/simark/src/binutils-gdb/gdb/inf-loop.c:43
    #17 0x55b6eea2a1f0 in handle_target_event /home/simark/src/binutils-gdb/gdb/linux-nat.c:4358
    #18 0x55b6ee7045f1 in handle_file_event /home/simark/src/binutils-gdb/gdb/event-loop.c:733
    #19 0x55b6ee704e89 in gdb_wait_for_event /home/simark/src/binutils-gdb/gdb/event-loop.c:859
    #20 0x55b6ee7027b5 in gdb_do_one_event() /home/simark/src/binutils-gdb/gdb/event-loop.c:322
    #21 0x55b6ee702907 in start_event_loop() /home/simark/src/binutils-gdb/gdb/event-loop.c:371
    #22 0x55b6eeadfc16 in captured_command_loop /home/simark/src/binutils-gdb/gdb/main.c:331
    #23 0x55b6eeae2ef9 in captured_main /home/simark/src/binutils-gdb/gdb/main.c:1174
    #24 0x55b6eeae30c2 in gdb_main(captured_main_args*) /home/simark/src/binutils-gdb/gdb/main.c:1190
    #25 0x55b6edf4fa89 in main /home/simark/src/binutils-gdb/gdb/gdb.c:32
    #26 0x7fed88ad8222 in __libc_start_main (/usr/lib/libc.so.6+0x24222)
    #27 0x55b6edf4f86d in _start (/home/simark/build/binutils-gdb/gdb/gdb+0x197186d)

0x6210003d4100 is located 0 bytes inside of 4096-byte region [0x6210003d4100,0x6210003d5100)
freed by thread T0 here:
    #0 0x7fed89a8ac19 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:66
    #1 0x55b6edfe12df in xfree<char> /home/simark/src/binutils-gdb/gdb/common/common-utils.h:60
    #2 0x55b6edfea675 in gdb::xfree_deleter<char>::operator()(char*) const /home/simark/src/binutils-gdb/gdb/common/gdb_unique_ptr.h:34
    #3 0x55b6edfe532c in std::unique_ptr<char, gdb::xfree_deleter<char> >::reset(char*) /usr/include/c++/8.2.1/bits/unique_ptr.h:382
    #4 0x55b6edfe7329 in std::unique_ptr<char, gdb::xfree_deleter<char> >::operator=(std::unique_ptr<char, gdb::xfree_deleter<char> >&&) /usr/include/c++/8.2.1/bits/unique_ptr.h:289
    #5 0x55b6ef09ec2b in find_and_open_source(char const*, char const*, std::unique_ptr<char, gdb::xfree_deleter<char> >*) /home/simark/src/binutils-gdb/gdb/source.c:990
    #6 0x55b6ef09f56a in open_source_file(symtab*) /home/simark/src/binutils-gdb/gdb/source.c:1069
    #7 0x55b6ef090f78 in source_cache::get_source_lines(symtab*, int, int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) /home/simark/src/binutils-gdb/gdb/source-cache.c:205
    #8 0x55b6ef0a15cb in print_source_lines_base /home/simark/src/binutils-gdb/gdb/source.c:1340
    #9 0x55b6ef0a2045 in print_source_lines(symtab*, int, int, enum_flags<print_source_lines_flag>) /home/simark/src/binutils-gdb/gdb/source.c:1415
    #10 0x55b6ef112c87 in print_frame_info(frame_info*, int, print_what, int, int) /home/simark/src/binutils-gdb/gdb/stack.c:914
    #11 0x55b6ef10e90d in print_stack_frame(frame_info*, int, print_what, int) /home/simark/src/binutils-gdb/gdb/stack.c:180
    #12 0x55b6ee9592f8 in print_stop_location /home/simark/src/binutils-gdb/gdb/infrun.c:7853
    #13 0x55b6ee95948f in print_stop_event(ui_out*) /home/simark/src/binutils-gdb/gdb/infrun.c:7870
    #14 0x55b6ef34b962 in tui_on_normal_stop /home/simark/src/binutils-gdb/gdb/tui/tui-interp.c:98
    #15 0x55b6ee01a14d in std::_Function_handler<void (bpstats*, int), void (*)(bpstats*, int)>::_M_invoke(std::_Any_data const&, bpstats*&&, int&&) /usr/include/c++/8.2.1/bits/std_function.h:297
    #16 0x55b6ee965415 in std::function<void (bpstats*, int)>::operator()(bpstats*, int) const /usr/include/c++/8.2.1/bits/std_function.h:687
    #17 0x55b6ee962f1b in gdb::observers::observable<bpstats*, int>::notify(bpstats*, int) const /home/simark/src/binutils-gdb/gdb/common/observable.h:106
    #18 0x55b6ee95a6e7 in normal_stop() /home/simark/src/binutils-gdb/gdb/infrun.c:8142
    #19 0x55b6ee93f236 in fetch_inferior_event(void*) /home/simark/src/binutils-gdb/gdb/infrun.c:3782
    #20 0x55b6ee8f2641 in inferior_event_handler(inferior_event_type, void*) /home/simark/src/binutils-gdb/gdb/inf-loop.c:43
    #21 0x55b6eea2a1f0 in handle_target_event /home/simark/src/binutils-gdb/gdb/linux-nat.c:4358
    #22 0x55b6ee7045f1 in handle_file_event /home/simark/src/binutils-gdb/gdb/event-loop.c:733
    #23 0x55b6ee704e89 in gdb_wait_for_event /home/simark/src/binutils-gdb/gdb/event-loop.c:859
    #24 0x55b6ee7027b5 in gdb_do_one_event() /home/simark/src/binutils-gdb/gdb/event-loop.c:322
    #25 0x55b6ee702907 in start_event_loop() /home/simark/src/binutils-gdb/gdb/event-loop.c:371
    #26 0x55b6eeadfc16 in captured_command_loop /home/simark/src/binutils-gdb/gdb/main.c:331
    #27 0x55b6eeae2ef9 in captured_main /home/simark/src/binutils-gdb/gdb/main.c:1174
    #28 0x55b6eeae30c2 in gdb_main(captured_main_args*) /home/simark/src/binutils-gdb/gdb/main.c:1190
    #29 0x55b6edf4fa89 in main /home/simark/src/binutils-gdb/gdb/gdb.c:32

previously allocated by thread T0 here:
    #0 0x7fed89a8b019 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:86
    #1 0x7fed88af983f in realpath@@GLIBC_2.3 (/usr/lib/libc.so.6+0x4583f)
    #2 0x7fed899dbbbc in __interceptor_canonicalize_file_name /build/gcc/src/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:3297
    #3 0x55b6ee376a03 in gdb_realpath(char const*) /home/simark/src/binutils-gdb/gdb/common/pathstuff.c:72
    #4 0x55b6ef09ec12 in find_and_open_source(char const*, char const*, std::unique_ptr<char, gdb::xfree_deleter<char> >*) /home/simark/src/binutils-gdb/gdb/source.c:990
    #5 0x55b6ef09f56a in open_source_file(symtab*) /home/simark/src/binutils-gdb/gdb/source.c:1069
    #6 0x55b6ef0a0f12 in print_source_lines_base /home/simark/src/binutils-gdb/gdb/source.c:1270
    #7 0x55b6ef0a2045 in print_source_lines(symtab*, int, int, enum_flags<print_source_lines_flag>) /home/simark/src/binutils-gdb/gdb/source.c:1415
    #8 0x55b6ef112c87 in print_frame_info(frame_info*, int, print_what, int, int) /home/simark/src/binutils-gdb/gdb/stack.c:914
    #9 0x55b6ef10e90d in print_stack_frame(frame_info*, int, print_what, int) /home/simark/src/binutils-gdb/gdb/stack.c:180
    #10 0x55b6ee9592f8 in print_stop_location /home/simark/src/binutils-gdb/gdb/infrun.c:7853
    #11 0x55b6ee95948f in print_stop_event(ui_out*) /home/simark/src/binutils-gdb/gdb/infrun.c:7870
    #12 0x55b6ef34b962 in tui_on_normal_stop /home/simark/src/binutils-gdb/gdb/tui/tui-interp.c:98
    #13 0x55b6ee01a14d in std::_Function_handler<void (bpstats*, int), void (*)(bpstats*, int)>::_M_invoke(std::_Any_data const&, bpstats*&&, int&&) /usr/include/c++/8.2.1/bits/std_function.h:297
    #14 0x55b6ee965415 in std::function<void (bpstats*, int)>::operator()(bpstats*, int) const /usr/include/c++/8.2.1/bits/std_function.h:687
    #15 0x55b6ee962f1b in gdb::observers::observable<bpstats*, int>::notify(bpstats*, int) const /home/simark/src/binutils-gdb/gdb/common/observable.h:106
    #16 0x55b6ee95a6e7 in normal_stop() /home/simark/src/binutils-gdb/gdb/infrun.c:8142
    #17 0x55b6ee93f236 in fetch_inferior_event(void*) /home/simark/src/binutils-gdb/gdb/infrun.c:3782
    #18 0x55b6ee8f2641 in inferior_event_handler(inferior_event_type, void*) /home/simark/src/binutils-gdb/gdb/inf-loop.c:43
    #19 0x55b6eea2a1f0 in handle_target_event /home/simark/src/binutils-gdb/gdb/linux-nat.c:4358
    #20 0x55b6ee7045f1 in handle_file_event /home/simark/src/binutils-gdb/gdb/event-loop.c:733
    #21 0x55b6ee704e89 in gdb_wait_for_event /home/simark/src/binutils-gdb/gdb/event-loop.c:859
    #22 0x55b6ee7027b5 in gdb_do_one_event() /home/simark/src/binutils-gdb/gdb/event-loop.c:322
    #23 0x55b6ee702907 in start_event_loop() /home/simark/src/binutils-gdb/gdb/event-loop.c:371
    #24 0x55b6eeadfc16 in captured_command_loop /home/simark/src/binutils-gdb/gdb/main.c:331
    #25 0x55b6eeae2ef9 in captured_main /home/simark/src/binutils-gdb/gdb/main.c:1174
    #26 0x55b6eeae30c2 in gdb_main(captured_main_args*) /home/simark/src/binutils-gdb/gdb/main.c:1190
    #27 0x55b6edf4fa89 in main /home/simark/src/binutils-gdb/gdb/gdb.c:32
    #28 0x7fed88ad8222 in __libc_start_main (/usr/lib/libc.so.6+0x24222)

gdb/ChangeLog:

	* source-cache.c (source_cache::get_source_lines): Re-read
	fullname after calling open_source_file.
---
 gdb/ChangeLog      | 5 +++++
 gdb/source-cache.c | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index bfcfc748cc03..b12ce2a99cf6 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2019-03-25  Simon Marchi  <simon.marchi@polymtl.ca>
+
+	* source-cache.c (source_cache::get_source_lines): Re-read
+	fullname after calling open_source_file.
+
 2019-03-18  Pedro Alves  <palves@redhat.com>
 	    Eli Zaretskii <eliz@gnu.org>

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 9211f098eb70..5eae02082df8 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -206,6 +206,12 @@ source_cache::get_source_lines (struct symtab *s, int first_line,
 		  if (desc.get () < 0)
 		    return false;
 		  find_source_lines (s, desc.get ());
+
+		  /* FULLNAME points to a value owned by the symtab
+		     (symtab::fullname).  Calling open_source_file reallocates
+		     that value, so we must refresh FULLNAME to avoid a
+		     use-after-free.  */
+		  fullname = symtab_to_fullname (s);
 		}
 	      srchilite::SourceHighlight highlighter ("esc.outlang");
 	      highlighter.setStyleFile("esc.style");
-- 
2.21.0

Re: Fix pressing down in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Otherwise this LGTM.  I see get_plain_source_lines has similar
Pedro> code, so my immediate thought was to move that to a helper function,
Pedro> but there's a difference that makes that unviable -- get_plain_source_lines
Pedro> always wants to open the source file first, while here we can avoid
Pedro> it unless line_charpos is 0.

I don't really like having to open the file like this on every step, and
especially not just do call some other function for its side-effects.
However, this is the status quo ante, so the patch seems fine; and I
made a note to fix this up eventually.

Tom

Avoid overwriting the TUI source window frame (Re: [RFC 8.3 0/3] Some style fixes)

[Pedro Alves]

On 03/12/2019 04:44 PM, Eli Zaretskii wrote:
> 2. The fix for avoiding to overwrite the source window frame is
> simple: revert 4a3045920.  I don't really understand why that change
> was made: AFAIU, wclrtoeol clears to the end of the window line, and
> cannot be told to clear only part of the line.
> 
> --- gdb/tui/tui-winsource.c~4	2019-02-27 06:51:50.000000000 +0200
> +++ gdb/tui/tui-winsource.c	2019-03-12 10:57:02.052875200 +0200
> @@ -285,7 +285,12 @@ tui_show_source_line (struct tui_win_inf
>      wattroff (win_info->generic.handle, A_STANDOUT);
>  
>    /* Clear to end of line but stop before the border.  */
> -  wclrtoeol (win_info->generic.handle);
> +  int x = getcurx (win_info->generic.handle);
> +  while (x + 1 < win_info->generic.width)
> +    {
> +      waddch (win_info->generic.handle, ' ');
> +      x = getcurx (win_info->generic.handle);
> +    }
>  }
>  

Makes sense to me.  I confirm this fixes it for me too.

>  void

Thanks,
Pedro Alves

Re: Avoid overwriting the TUI source window frame (Re: [RFC 8.3 0/3] Some style fixes)

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 15 Mar 2019 12:43:21 +0000
> 
> On 03/12/2019 04:44 PM, Eli Zaretskii wrote:
> > 2. The fix for avoiding to overwrite the source window frame is
> > simple: revert 4a3045920.  I don't really understand why that change
> > was made: AFAIU, wclrtoeol clears to the end of the window line, and
> > cannot be told to clear only part of the line.
> > 
> > --- gdb/tui/tui-winsource.c~4	2019-02-27 06:51:50.000000000 +0200
> > +++ gdb/tui/tui-winsource.c	2019-03-12 10:57:02.052875200 +0200
> > @@ -285,7 +285,12 @@ tui_show_source_line (struct tui_win_inf
> >      wattroff (win_info->generic.handle, A_STANDOUT);
> >  
> >    /* Clear to end of line but stop before the border.  */
> > -  wclrtoeol (win_info->generic.handle);
> > +  int x = getcurx (win_info->generic.handle);
> > +  while (x + 1 < win_info->generic.width)
> > +    {
> > +      waddch (win_info->generic.handle, ' ');
> > +      x = getcurx (win_info->generic.handle);
> > +    }
> >  }
> >  
> 
> Makes sense to me.  I confirm this fixes it for me too.

Thanks, pushed to both branches.

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

[Pedro Alves]

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

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)
 {
 }
 
-- 
2.14.4

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

[Eli Zaretskii]

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

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

[Pedro Alves]

On 03/15/2019 03:37 PM, Eli Zaretskii wrote:
>> 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.  

I've merged the patch to master and the 8.3 branch.

> 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.
For sure.  This code predates me by a long shot.  I wouldn't be
surprised if it was already in the original TUI dump from HP.

Anyway, I've stared at this for a while, and I _think_ this captures
the idea.  WDYT?

From 8e37e7076f6ddca767db35b66284268935bcc186 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 18 Mar 2019 19:10:42 +0000
Subject: [PATCH] Add comments describing tui_ui_out and its fields, cleanup a
 bit

This commit add comments describing tui_ui_out and its fields, and
cleans up the code a little bit.

Also switch to using in-class initialization so that the initial
values can be seen alongside the comments.

I see no reason for initializing m_line as -1 instead of 0, since all
the checks in the .c file are of the form "> 0".  AFAICS there's no
practical difference between -1 and 0.  So it seems simpler to
initialize it as 0.

There's a bit of redundancy in tui_ui_out::do_field_string, which is
fixed by this commit.

gdb/ChangeLog:
2019-03-18  Pedro Alves  <palves@redhat.com>

	* tui/tui-out.c (tui_ui_out::do_field_string): Simplify.
	(tui_ui_out::do_text): Add comments.  Reset M_LINE to 0 instead of
	to -1.  Fix TABs vs spaces.
	(tui_ui_out::tui_ui_out): Don't initialize fields here.
	* tui/tui-out.h (tui_ui_out) Add intro comments.
	<m_line, m_start_of_line>: In-class initialize, and add describing
	comment.
---
 gdb/tui/tui-out.c | 27 +++++++++++++--------------
 gdb/tui/tui-out.h | 21 +++++++++++++++++++--
 2 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/gdb/tui/tui-out.c b/gdb/tui/tui-out.c
index dd37736c4a..64f77077c8 100644
--- a/gdb/tui/tui-out.c
+++ b/gdb/tui/tui-out.c
@@ -57,17 +57,13 @@ tui_ui_out::do_field_string (int fldno, int width, ui_align align,
   if (suppress_output ())
     return;
 
+  m_start_of_line++;
+
   if (fldname && m_line > 0 && strcmp (fldname, "fullname") == 0)
     {
-      m_start_of_line++;
-      if (m_line > 0)
-        {
-          tui_show_source (string, m_line);
-        }
+      tui_show_source (string, m_line);
       return;
     }
-  
-  m_start_of_line++;
 
   cli_ui_out::do_field_string (fldno, width, align, fldname, string, style);
 }
@@ -94,11 +90,16 @@ tui_ui_out::do_text (const char *string)
   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)
-        {
-          m_line = -1;
-          m_start_of_line = 0;
-        }
+	{
+	  /* If 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'))
@@ -108,9 +109,7 @@ tui_ui_out::do_text (const char *string)
 }
 
 tui_ui_out::tui_ui_out (ui_file *stream)
-: cli_ui_out (stream, 0),
-  m_line (-1),
-  m_start_of_line (0)
+  : cli_ui_out (stream, 0)
 {
 }
 
diff --git a/gdb/tui/tui-out.h b/gdb/tui/tui-out.h
index 10311c9255..b0d8b8d898 100644
--- a/gdb/tui/tui-out.h
+++ b/gdb/tui/tui-out.h
@@ -20,6 +20,10 @@
 
 #include "cli-out.h"
 
+/* An 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:
@@ -39,8 +43,21 @@ protected:
 
 private:
 
-  int m_line;
-  int m_start_of_line;
+  /* 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;
 };
 
 extern tui_ui_out *tui_out_new (struct ui_file *stream);
-- 
2.14.4

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

[Eli Zaretskii]

> Cc: tromey@adacore.com, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Mon, 18 Mar 2019 20:24:28 +0000
> 
> > 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.
> For sure.  This code predates me by a long shot.  I wouldn't be
> surprised if it was already in the original TUI dump from HP.
> 
> Anyway, I've stared at this for a while, and I _think_ this captures
> the idea.  WDYT?

Thanks, LGTM.  It certainly explains the values of these two members
and how they are used.

> +	{
> +	  /* If we've reached the end of the line, so go back to
> +	     letting text output go to the console.  */

A minor typo: "If ..., so ..." sounds weird.  I think you wanted to
get rid of that "If".

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

[Pedro Alves]

On 03/19/2019 06:08 AM, Eli Zaretskii wrote:
>> Cc: tromey@adacore.com, gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>

>> Anyway, I've stared at this for a while, and I _think_ this captures
>> the idea.  WDYT?
> 
> Thanks, LGTM.  It certainly explains the values of these two members
> and how they are used.
> 
>> +	{
>> +	  /* If we've reached the end of the line, so go back to
>> +	     letting text output go to the console.  */
> 
> A minor typo: "If ..., so ..." sounds weird.  I think you wanted to
> get rid of that "If".

Indeed.  Fixed that, and merged to master.

Thanks,
Pedro Alves

Re: [RFC 8.3 0/3] Some style fixes

[Hannes Domani via gdb-patches]

Am Freitag, 8. März 2019, 22:04:47 MEZ hat Tom Tromey <tromey@adacore.com> Folgendes geschrieben: 

> This series fixes some of the TUI/styling regressions noted by Pedro.
> I would like to push this to the 8.3 branch as well.
>
> Patch #2 is a new feature that Eli mentioned.  Since it's an addition
> to a feature that is also new in 8.3, it seemed reasonable to do at
> this time.
>
> Let me know what you think.

These patches work great for me.


But while testing them I saw some weird behavior which was introduced with the style patches.

1) Outside of TUI, escaped characters (< 040 and 0177) aren't handled correctly any more when
list'ing some source code, resulting in an endless loop.
See print_source_lines_base(), I just added "++iter" in the last 2 if() blocks, that fixed it for me.

2) In TUI, scrolling right with the arrow keys, the first keypress doesn't do anything.
(that's just a very minor nuisance)

3) Again in TUI, scrolling right handles TABS and escaped characters as single characters,
which just looks weird.


Regards
Hannes Domani

Re: [RFC 8.3 0/3] Some style fixes

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani via gdb-patches <gdb-patches@sourceware.org> writes:

Hannes> 1) Outside of TUI, escaped characters (< 040 and 0177) aren't handled correctly any more when
Hannes> list'ing some source code, resulting in an endless loop.
Hannes> See print_source_lines_base(), I just added "++iter" in the last 2 if() blocks, that fixed it for me.

Hannes> 2) In TUI, scrolling right with the arrow keys, the first keypress doesn't do anything.
Hannes> (that's just a very minor nuisance)

Hannes> 3) Again in TUI, scrolling right handles TABS and escaped characters as single characters,
Hannes> which just looks weird.

Thanks, I will take a look at these.

I suspect #2 is the nonl problem.  Could you try the patch I sent in
another branch of this thread?

Tom

Re: [RFC 8.3 0/3] Some style fixes

[Hannes Domani via gdb-patches]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

 Am Dienstag, 12. März 2019, 17:48:12 MEZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben: 

> >>>>> "Hannes" == Hannes Domani via gdb-patches <gdb-patches@sourceware.org> writes:
>
> Hannes> 1) Outside of TUI, escaped characters (< 040 and 0177) aren't handled correctly any more when
> Hannes> list'ing some source code, resulting in an endless loop.
> Hannes> See print_source_lines_base(), I just added "++iter" in the last 2 if() blocks, that fixed it for me.
>
> Hannes> 2) In TUI, scrolling right with the arrow keys, the first keypress doesn't do anything.
> Hannes> (that's just a very minor nuisance)
>
> Hannes> 3) Again in TUI, scrolling right handles TABS and escaped characters as single characters,
> Hannes> which just looks weird.
>
> Thanks, I will take a look at these.
>
> I suspect #2 is the nonl problem.  Could you try the patch I sent in
> another branch of this thread?

No, applying that doesn't fix this.
In copy_source_line() it checks if (column < first_col), and because of the ++column directly
before, it basically starts with 1 instead of 0.

Attached is a patch that fixes most of 2) and 3), but I ignored the handling of escaped
characters, because I just don't have them in any of my sources.

[-- Attachment #2: 0001-Fix-scrolling-right-in-TUI.patch --]
[-- Type: application/octet-stream, Size: 1500 bytes --]

From 02b18ad0da85fd0ec6082b5c8e31a3f98fc8a5b0 Mon Sep 17 00:00:00 2001
From: Hannes Domani <ssbssa@yahoo.de>
Date: Sat, 9 Mar 2019 15:30:07 +0100
Subject: [PATCH] Fix scrolling right in TUI.

---
 gdb/tui/tui-source.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
index 63e752cd04..142ce86338 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -73,8 +73,21 @@ copy_source_line (const char **ptr, int line_no, int first_col,
       ++column;
       /* We have to process all the text in order to pick up all the
 	 escapes.  */
-      if (column < first_col || column > first_col + line_width)
-	continue;
+      if (column <= first_col || column > first_col + line_width)
+	{
+	  if (c == '\t')
+	    {
+	      int j, max_tab_len = tui_tab_width;
+
+	      --column;
+	      for (j = column % max_tab_len;
+		   j < max_tab_len && column < first_col + line_width;
+		   column++, j++)
+		if (column >= first_col)
+		  result.push_back (' ');
+	    }
+	  continue;
+	}
 
       if (c == '\n' || c == '\r' || c == '\0')
 	{
@@ -94,7 +107,8 @@ copy_source_line (const char **ptr, int line_no, int first_col,
 	{
 	  int j, max_tab_len = tui_tab_width;
 
-	  for (j = column - ((column / max_tab_len) * max_tab_len);
+	  --column;
+	  for (j = column % max_tab_len;
 	       j < max_tab_len && column < first_col + line_width;
 	       column++, j++)
 	    result.push_back (' ');
-- 
2.15.1.windows.2

Re: [RFC 8.3 0/3] Some style fixes

[Eli Zaretskii]

> Date: Tue, 12 Mar 2019 17:08:33 +0000 (UTC)
> From: "Hannes Domani via gdb-patches" <gdb-patches@sourceware.org>
> 
> In copy_source_line() it checks if (column < first_col), and because of the ++column directly
> before, it basically starts with 1 instead of 0.
> 
> Attached is a patch that fixes most of 2) and 3), but I ignored the handling of escaped
> characters, because I just don't have them in any of my sources.

I can confirm that this patch fixes the problems with horizontal
scrolling, with or without TAB characters in the sources.  I think we
should push it.

I also found another regression in the TUI configuration: if you step
with, say, "next" through the program, and execution winds up on a
line that is longer than the source window-width, then the current
line gets redrawn in reverse video, and as result it overwrites the
window-frame and a portion of the next source line.  This happens
because we only update one source line, and we do that in a way that
doesn't take the window width and horizontal scrolling offset into
account.

Here's the patch that fixes this regression:

--- gdb/tui/tui-winsource.c~5	2019-03-12 16:51:50.000000000 +0200
+++ gdb/tui/tui-winsource.c	2019-03-13 08:21:02.303443600 +0200
@@ -382,12 +387,30 @@ tui_set_is_exec_point_at (struct tui_lin
         {
           changed++;
           content[i]->which_element.source.is_exec_point = new_state;
-          tui_show_source_line (win_info, i + 1);
         }
       i++;
     }
   if (changed)
-    tui_refresh_win (&win_info->generic);
+    {
+      struct gdbarch *gdbarch = win_info->detail.source_info.gdbarch;
+      struct symtab *s = NULL;
+
+      if (win_info->generic.type == SRC_WIN)
+	{
+	  struct symtab_and_line cursal
+	    = get_current_source_symtab_and_line ();
+
+	  if (cursal.symtab == NULL)
+	    s = find_pc_line_symtab (get_frame_pc (get_selected_frame (NULL)));
+	  else
+	    s = cursal.symtab;
+	}
+      tui_update_source_window_as_is (win_info, gdbarch, s,
+				      win_info->generic.content[0]
+					->which_element.source.line_or_addr,
+				      FALSE);
+      tui_refresh_win (&win_info->generic);
+    }
 }
 
 /* Update the execution windows to show the active breakpoints.

"next" into line longer than the source window-width (Re: [RFC 8.3 0/3] Some style fixes)

[Pedro Alves]

On 03/13/2019 03:44 PM, Eli Zaretskii wrote:
> I also found another regression in the TUI configuration: if you step
> with, say, "next" through the program, and execution winds up on a
> line that is longer than the source window-width, then the current
> line gets redrawn in reverse video, and as result it overwrites the
> window-frame and a portion of the next source line.  This happens
> because we only update one source line, and we do that in a way that
> doesn't take the window width and horizontal scrolling offset into
> account.
> 
> Here's the patch that fixes this regression:
> 
> --- gdb/tui/tui-winsource.c~5	2019-03-12 16:51:50.000000000 +0200
> +++ gdb/tui/tui-winsource.c	2019-03-13 08:21:02.303443600 +0200
> @@ -382,12 +387,30 @@ tui_set_is_exec_point_at (struct tui_lin
>          {
>            changed++;
>            content[i]->which_element.source.is_exec_point = new_state;
> -          tui_show_source_line (win_info, i + 1);
>          }
>        i++;
>      }
>    if (changed)
> -    tui_refresh_win (&win_info->generic);
> +    {
> +      struct gdbarch *gdbarch = win_info->detail.source_info.gdbarch;
> +      struct symtab *s = NULL;
> +
> +      if (win_info->generic.type == SRC_WIN)
> +	{
> +	  struct symtab_and_line cursal
> +	    = get_current_source_symtab_and_line ();
> +
> +	  if (cursal.symtab == NULL)
> +	    s = find_pc_line_symtab (get_frame_pc (get_selected_frame (NULL)));
> +	  else
> +	    s = cursal.symtab;
> +	}
> +      tui_update_source_window_as_is (win_info, gdbarch, s,
> +				      win_info->generic.content[0]
> +					->which_element.source.line_or_addr,
> +				      FALSE);
> +      tui_refresh_win (&win_info->generic);
> +    }

The substance of the patch LGTM.

But all this code above looks very much like the new tui_refill_source_window
function, added today by 6f11e6824e15.  I think you can replace all
the above with:

  -  tui_refresh_win (&win_info->generic);
  +  tui_refill_source_window (win_info);

Thanks,
Pedro Alves

Re: "next" into line longer than the source window-width (Re: [RFC 8.3 0/3] Some style fixes)

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 14 Mar 2019 20:25:25 +0000
> 
> The substance of the patch LGTM.
> 
> But all this code above looks very much like the new tui_refill_source_window
> function, added today by 6f11e6824e15.  I think you can replace all
> the above with:
> 
>   -  tui_refresh_win (&win_info->generic);
>   +  tui_refill_source_window (win_info);

You are right; I pushed the above to both branches.

[PATCH] Fix scrolling right in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

[Pedro Alves]

On 03/13/2019 03:44 PM, Eli Zaretskii wrote:
>> Date: Tue, 12 Mar 2019 17:08:33 +0000 (UTC)
>> From: "Hannes Domani via gdb-patches" <gdb-patches@sourceware.org>
>>
>> In copy_source_line() it checks if (column < first_col), and because of the ++column directly
>> before, it basically starts with 1 instead of 0.
>>
>> Attached is a patch that fixes most of 2) and 3), but I ignored the handling of escaped
>> characters, because I just don't have them in any of my sources.
> I can confirm that this patch fixes the problems with horizontal
> scrolling, with or without TAB characters in the sources.  I think we
> should push it.

I agree.  Hannes, I've written a git commit log entry, as well
as a ChangeLog entry for you.  Please double-check whether
I didn't make some mistake.

I've made one change in the patch, here:

On 03/12/2019 05:08 PM, Hannes Domani via gdb-patches wrote:
> +      if (column <= first_col || column > first_col + line_width)
> +	{
> +	  if (c == '\t')
> +	    {
> +	      int j, max_tab_len = tui_tab_width;
> +
> +	      --column;
> +	      for (j = column % max_tab_len;
> +		   j < max_tab_len && column < first_col + line_width;
> +		   column++, j++)
> +		if (column >= first_col)
> +		  result.push_back (' ');
> +	    }
> +	  continue;
> +	}

instead of duplicating that code, I'd put it in a lambda
and use it in both places.

Tromey, WDYT?  Would you prefer the version without the lambda?

From 3b88b2cebb2d8697fbbcf957e587440ee2a4968c Mon Sep 17 00:00:00 2001
From: Hannes Domani <ssbssa@yahoo.de>
Date: Thu, 14 Mar 2019 20:46:27 +0000
Subject: [PATCH] Fix scrolling right in the TUI

This commit fixes two issues in scrolling right in the TUI:

#1 - Scrolling right with the arrow keys, the first keypress doesn't
do anything.  The problem is that copy_source_line() checks if
(column < first_col), and because of the ++column directly before, it
basically starts with 1 instead of 0.

#2 - Scrolling right handles TABS and escaped characters as single
characters, which just looks weird.  The problem is that there's a
spot that misses handling TABS.

gdb/ChangeLog:
yyyy-mm-dd  Hannes Domani  <ssbssa@yahoo.de>

	* tui/tui-source.c (copy_source_line): Fix handling of 'column'.
	Handle tabs.
---
 gdb/tui/tui-source.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
index 7cc3c00069..1fa0e986e0 100644
--- a/gdb/tui/tui-source.c
+++ b/gdb/tui/tui-source.c
@@ -71,10 +71,27 @@ copy_source_line (const char **ptr, int line_no, int first_col,
 
       ++lineptr;
       ++column;
+
+      auto process_tab = [&] ()
+	{
+	  int max_tab_len = tui_tab_width;
+
+	  --column;
+	  for (int j = column % max_tab_len;
+	       j < max_tab_len && column < first_col + line_width;
+	       column++, j++)
+	    if (column >= first_col)
+	      result.push_back (' ');
+	};
+
       /* We have to process all the text in order to pick up all the
 	 escapes.  */
-      if (column < first_col || column > first_col + line_width)
-	continue;
+      if (column <= first_col || column > first_col + line_width)
+	{
+	  if (c == '\t')
+	    process_tab ();
+	  continue;
+	}
 
       if (c == '\n' || c == '\r' || c == '\0')
 	{
@@ -91,14 +108,7 @@ copy_source_line (const char **ptr, int line_no, int first_col,
 	  result.push_back ('?');
 	}
       else if (c == '\t')
-	{
-	  int j, max_tab_len = tui_tab_width;
-
-	  for (j = column - ((column / max_tab_len) * max_tab_len);
-	       j < max_tab_len && column < first_col + line_width;
-	       column++, j++)
-	    result.push_back (' ');
-	}
+	process_tab ();
       else
 	result.push_back (c);
     }
-- 
2.14.4

Re: [PATCH] Fix scrolling right in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

[Hannes Domani via gdb-patches]

 Am Donnerstag, 14. März 2019, 21:58:40 MEZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben: 

> I agree.  Hannes, I've written a git commit log entry, as well
> as a ChangeLog entry for you.  Please double-check whether
> I didn't make some mistake.
>
> I've made one change in the patch, here:
>
> On 03/12/2019 05:08 PM, Hannes Domani via gdb-patches wrote:
> > +      if (column <= first_col || column > first_col + line_width)
> > +    {
> > +      if (c == '\t')
> > +        {
> > +          int j, max_tab_len = tui_tab_width;
> > +
> > +          --column;
> > +          for (j = column % max_tab_len;
> > +          j < max_tab_len && column < first_col + line_width;
> > +          column++, j++)
> > +        if (column >= first_col)
> > +          result.push_back (' ');
> > +        }
> > +      continue;
> > +    }
>
> instead of duplicating that code, I'd put it in a lambda
> and use it in both places.

Since that duplicate code actually did bother me, I prefer your changes.
And I just tested it, still works fine.


Regards
Hannes Domani

Re: [PATCH] Fix scrolling right in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Tromey, WDYT?  Would you prefer the version without the lambda?

I think it's fine with them.  Thank you.

Pedro> gdb/ChangeLog:
Pedro> yyyy-mm-dd  Hannes Domani  <ssbssa@yahoo.de>

Pedro> 	* tui/tui-source.c (copy_source_line): Fix handling of 'column'.
Pedro> 	Handle tabs.

Tom

Re: [PATCH] Fix scrolling right in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

[Pedro Alves]

On 03/15/2019 09:51 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Tromey, WDYT?  Would you prefer the version without the lambda?
> 
> I think it's fine with them.  Thank you.

Merged to master and branch.

Thanks,
Pedro Alves

Re: [PATCH] Fix scrolling right in the TUI (Re: [RFC 8.3 0/3] Some style fixes)

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 14 Mar 2019 20:58:36 +0000
> 
> On 03/12/2019 05:08 PM, Hannes Domani via gdb-patches wrote:
> > +      if (column <= first_col || column > first_col + line_width)
> > +	{
> > +	  if (c == '\t')
> > +	    {
> > +	      int j, max_tab_len = tui_tab_width;
> > +
> > +	      --column;
> > +	      for (j = column % max_tab_len;
> > +		   j < max_tab_len && column < first_col + line_width;
> > +		   column++, j++)
> > +		if (column >= first_col)
> > +		  result.push_back (' ');
> > +	    }
> > +	  continue;
> > +	}
> 
> instead of duplicating that code, I'd put it in a lambda
> and use it in both places.
> 
> Tromey, WDYT?  Would you prefer the version without the lambda?

Works for me, with the initialization fixed to be 'false'.

Thanks.

Re: [RFC 8.3 0/3] Some style fixes

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Cc: Hannes Domani <ssbssa@yahoo.de>
> Date: Tue, 12 Mar 2019 10:48:08 -0600
> 
> I suspect #2 is the nonl problem.  Could you try the patch I sent in
> another branch of this thread?

No, it doesn't have anything to do with nonl.

Re: [RFC 8.3 0/3] Some style fixes

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Cc: Hannes Domani <ssbssa@yahoo.de>
> Date: Tue, 12 Mar 2019 10:48:08 -0600
> 
> Hannes> 1) Outside of TUI, escaped characters (< 040 and 0177) aren't handled correctly any more when
> Hannes> list'ing some source code, resulting in an endless loop.
> Hannes> See print_source_lines_base(), I just added "++iter" in the last 2 if() blocks, that fixed it for me.
> 
> Hannes> 2) In TUI, scrolling right with the arrow keys, the first keypress doesn't do anything.
> Hannes> (that's just a very minor nuisance)
> 
> Hannes> 3) Again in TUI, scrolling right handles TABS and escaped characters as single characters,
> Hannes> which just looks weird.
> 
> Thanks, I will take a look at these.

I'm working on 2) and 3), and have already solved 90% of the problem
(the last 10% are some tricky corner cases with TABs).  Hope to have a
tested patch tomorrow.

Re: [RFC 8.3 0/3] Some style fixes

[Eli Zaretskii]

> Date: Tue, 12 Mar 2019 19:29:01 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: gdb-patches@sourceware.org, ssbssa@yahoo.de
> 
> I'm working on 2) and 3), and have already solved 90% of the problem
> (the last 10% are some tricky corner cases with TABs).  Hope to have a
> tested patch tomorrow.

OTOH, feel free to use what Hannes just sent.

Re: [RFC 8.3 0/3] Some style fixes

[Pedro Franco de Carvalho]

Hello,

It seems that issue 1) that Hannes reported is still present, and
affects both master and the release branch.  Control characters like
form feed are being printed infinitely in the CLI when listing a source
file (for form feed lots of "^L"s are being printed).

Thanks!
--
Pedro Franco de Carvalho

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Hannes" == Hannes Domani via gdb-patches <gdb-patches@sourceware.org> writes:
>
> Hannes> 1) Outside of TUI, escaped characters (< 040 and 0177) aren't handled correctly any more when
> Hannes> list'ing some source code, resulting in an endless loop.
> Hannes> See print_source_lines_base(), I just added "++iter" in the last 2 if() blocks, that fixed it for me.
>
> Hannes> 2) In TUI, scrolling right with the arrow keys, the first keypress doesn't do anything.
> Hannes> (that's just a very minor nuisance)
>
> Hannes> 3) Again in TUI, scrolling right handles TABS and escaped characters as single characters,
> Hannes> which just looks weird.
>
> Thanks, I will take a look at these.
>
> I suspect #2 is the nonl problem.  Could you try the patch I sent in
> another branch of this thread?
>
> Tom

Re: [RFC 8.3 0/3] Some style fixes

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> This series fixes some of the TUI/styling regressions noted by Pedro.
Tom> I would like to push this to the 8.3 branch as well.

I'm pushing the updated versions to both branches now.

Tom