Do not emit style escape sequences to log file

Message ID 20190606190539.11462-1-tromey@adacore.com
State New
Headers show
Series
  • Do not emit style escape sequences to log file
Related show

Commit Message

Tom Tromey June 6, 2019, 7:05 p.m.
PR gdb/24502 requests that the "set logging" log file not contain
style escape sequences emitted by gdb.

This seemed like a reasonable request to me, so this patch implements
filtering for the log file.

This also updates a comment in ui-style.h that I noticed while writing
the patch.

Tested on x86-64 Fedora 29.

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

	PR gdb/24502:
	* ui-style.h (skip_ansi_escape): Update comment.
	* ui-file.h (class no_terminal_escape_file): New class.
	* ui-file.c (no_terminal_escape_file::write)
	(no_terminal_escape_file::puts): New methods.
	* cli/cli-logging.c (handle_redirections): Use
	no_terminal_escape_file.

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

	PR gdb/24502:
	* gdb.base/style-logging.exp: New file.
---
 gdb/ChangeLog                            | 10 ++++
 gdb/cli/cli-logging.c                    |  2 +-
 gdb/testsuite/ChangeLog                  |  5 ++
 gdb/testsuite/gdb.base/style-logging.exp | 70 ++++++++++++++++++++++++
 gdb/ui-file.c                            | 32 +++++++++++
 gdb/ui-file.h                            | 16 ++++++
 gdb/ui-style.h                           |  4 +-
 7 files changed, 136 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/style-logging.exp

-- 
2.20.1

Comments

Sergio Durigan Junior June 7, 2019, 1:56 p.m. | #1
On Thursday, June 06 2019, Tom Tromey wrote:

> PR gdb/24502 requests that the "set logging" log file not contain

> style escape sequences emitted by gdb.

>

> This seemed like a reasonable request to me, so this patch implements

> filtering for the log file.

>

> This also updates a comment in ui-style.h that I noticed while writing

> the patch.

>

> Tested on x86-64 Fedora 29.


Thanks for the patch.

I looked at the code, tried my best to understand what it does
(specially how the "skip_ansi_escape" function interacts with the new
"no_terminal_escape_file::puts" method), and everything looks good to
me.

Thanks.

> gdb/ChangeLog

> 2019-06-06  Tom Tromey  <tromey@adacore.com>

>

> 	PR gdb/24502:

> 	* ui-style.h (skip_ansi_escape): Update comment.

> 	* ui-file.h (class no_terminal_escape_file): New class.

> 	* ui-file.c (no_terminal_escape_file::write)

> 	(no_terminal_escape_file::puts): New methods.

> 	* cli/cli-logging.c (handle_redirections): Use

> 	no_terminal_escape_file.

>

> gdb/testsuite/ChangeLog

> 2019-06-06  Tom Tromey  <tromey@adacore.com>

>

> 	PR gdb/24502:

> 	* gdb.base/style-logging.exp: New file.

> ---

>  gdb/ChangeLog                            | 10 ++++

>  gdb/cli/cli-logging.c                    |  2 +-

>  gdb/testsuite/ChangeLog                  |  5 ++

>  gdb/testsuite/gdb.base/style-logging.exp | 70 ++++++++++++++++++++++++

>  gdb/ui-file.c                            | 32 +++++++++++

>  gdb/ui-file.h                            | 16 ++++++

>  gdb/ui-style.h                           |  4 +-

>  7 files changed, 136 insertions(+), 3 deletions(-)

>  create mode 100644 gdb/testsuite/gdb.base/style-logging.exp

>

> diff --git a/gdb/cli/cli-logging.c b/gdb/cli/cli-logging.c

> index bef5f3939bd..a8b9955c570 100644

> --- a/gdb/cli/cli-logging.c

> +++ b/gdb/cli/cli-logging.c

> @@ -100,7 +100,7 @@ handle_redirections (int from_tty)

>        return;

>      }

>  

> -  stdio_file_up log (new stdio_file ());

> +  stdio_file_up log (new no_terminal_escape_file ());

>    if (!log->open (logging_filename, logging_overwrite ? "w" : "a"))

>      perror_with_name (_("set logging"));

>  

> diff --git a/gdb/testsuite/gdb.base/style-logging.exp b/gdb/testsuite/gdb.base/style-logging.exp

> new file mode 100644

> index 00000000000..1d7c3052c38

> --- /dev/null

> +++ b/gdb/testsuite/gdb.base/style-logging.exp

> @@ -0,0 +1,70 @@

> +# Copyright 2019 Free Software Foundation, Inc.

> +

> +# This program is free software; you can redistribute it and/or modify

> +# it under the terms of the GNU General Public License as published by

> +# the Free Software Foundation; either version 3 of the License, or

> +# (at your option) any later version.

> +#

> +# This program is distributed in the hope that it will be useful,

> +# but WITHOUT ANY WARRANTY; without even the implied warranty of

> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

> +# GNU General Public License for more details.

> +#

> +# You should have received a copy of the GNU General Public License

> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.

> +

> +# Test that logging does not style.

> +

> +# Do not run if gdb debug is enabled as it will interfere with log redirect.

> +if {[gdb_debug_enabled]} {

> +    untested "debug is enabled"

> +    return 0

> +}

> +

> +if {[is_remote host]} {

> +    untested "does not work on remote host"

> +    return 0

> +}

> +

> +standard_testfile style.c

> +

> +save_vars { env(TERM) } {

> +    # We need an ANSI-capable terminal to get the output.

> +    setenv TERM ansi

> +

> +    if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {

> +	return -1

> +    }

> +

> +    if {![runto_main]} {

> +	fail "style tests failed"

> +	return

> +    }

> +

> +    gdb_test_no_output "set style enabled on"

> +

> +    set log_name [standard_output_file log.txt]

> +    gdb_test_no_output "set logging file $log_name"

> +    gdb_test_no_output "set logging overwrite on"

> +    gdb_test "set logging on" "Copying output to .*"

> +

> +    set main_expr [style main function]

> +    set base_file_expr [style ".*style\\.c" file]

> +    set file_expr "$base_file_expr:\[0-9\]"

> +    set arg_expr [style "arg." variable]

> +    gdb_test "frame" \

> +	"$main_expr.*$arg_expr.*$arg_expr.*$file_expr.*"

> +

> +    gdb_test "set logging off" "Done logging to .*"

> +

> +    set fd [open $log_name]

> +    set data [read -nonewline $fd]

> +    close $fd

> +

> +    set testname "log is escape-free"

> +    if {[regexp "\033" $data]} {

> +	fail $testname

> +    } else {

> +	pass $testname

> +    }

> +}

> diff --git a/gdb/ui-file.c b/gdb/ui-file.c

> index 24c914f442a..05d411aa392 100644

> --- a/gdb/ui-file.c

> +++ b/gdb/ui-file.c

> @@ -396,3 +396,35 @@ tee_file::can_emit_style_escape ()

>  	  && m_one->term_out ()

>  	  && term_cli_styling ());

>  }

> +

> +/* See ui-file.h.  */

> +

> +void

> +no_terminal_escape_file::write (const char *buf, long length_buf)

> +{

> +  std::string copy (buf, length_buf);

> +  this->puts (copy.c_str ());

> +}

> +

> +/* See ui-file.h.  */

> +

> +void

> +no_terminal_escape_file::puts (const char *buf)

> +{

> +  while (*buf != '\0')

> +    {

> +      const char *esc = strchr (buf, '\033');

> +      if (esc == nullptr)

> +	break;

> +

> +      int n_read = 0;

> +      if (!skip_ansi_escape (esc, &n_read))

> +	++esc;

> +

> +      this->stdio_file::write (buf, esc - buf);

> +      buf = esc + n_read;

> +    }

> +

> +  if (*buf != '\0')

> +    this->stdio_file::write (buf, strlen (buf));

> +}

> diff --git a/gdb/ui-file.h b/gdb/ui-file.h

> index 39f56d5ea42..3f6f38a68fb 100644

> --- a/gdb/ui-file.h

> +++ b/gdb/ui-file.h

> @@ -287,4 +287,20 @@ private:

>    ui_file_up m_two;

>  };

>  

> +/* A ui_file implementation that filters out terminal escape

> +   sequences.  */

> +

> +class no_terminal_escape_file : public stdio_file

> +{

> +public:

> +  no_terminal_escape_file ()

> +  {

> +  }

> +

> +  /* Like the stdio_file methods, but these filter out terminal escape

> +     sequences.  */

> +  void write (const char *buf, long length_buf) override;

> +  void puts (const char *linebuffer) override;

> +};

> +

>  #endif

> diff --git a/gdb/ui-style.h b/gdb/ui-style.h

> index 2a87fbe8012..24b4b59ed9f 100644

> --- a/gdb/ui-style.h

> +++ b/gdb/ui-style.h

> @@ -233,8 +233,8 @@ private:

>  

>  /* Skip an ANSI escape sequence in BUF.  BUF must begin with an ESC

>     character.  Return true if an escape sequence was successfully

> -   skipped; false otherwise.  In either case, N_READ is updated to

> -   reflect the number of chars read from BUF.  */

> +   skipped; false otherwise.  If an escape sequence was skipped,

> +   N_READ is updated to reflect the number of chars read from BUF.  */

>  

>  extern bool skip_ansi_escape (const char *buf, int *n_read);

>  

> -- 

> 2.20.1


-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Tom Tromey June 14, 2019, 8:12 p.m. | #2
>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:


>> PR gdb/24502 requests that the "set logging" log file not contain

>> style escape sequences emitted by gdb.

>> 

>> This seemed like a reasonable request to me, so this patch implements

>> filtering for the log file.

>> 

>> This also updates a comment in ui-style.h that I noticed while writing

>> the patch.

>> 

>> Tested on x86-64 Fedora 29.


Sergio> Thanks for the patch.

Sergio> I looked at the code, tried my best to understand what it does
Sergio> (specially how the "skip_ansi_escape" function interacts with the new
Sergio> "no_terminal_escape_file::puts" method), and everything looks good to
Sergio> me.

Thanks for taking a look.

I'm checking this in now.

Tom

Patch

diff --git a/gdb/cli/cli-logging.c b/gdb/cli/cli-logging.c
index bef5f3939bd..a8b9955c570 100644
--- a/gdb/cli/cli-logging.c
+++ b/gdb/cli/cli-logging.c
@@ -100,7 +100,7 @@  handle_redirections (int from_tty)
       return;
     }
 
-  stdio_file_up log (new stdio_file ());
+  stdio_file_up log (new no_terminal_escape_file ());
   if (!log->open (logging_filename, logging_overwrite ? "w" : "a"))
     perror_with_name (_("set logging"));
 
diff --git a/gdb/testsuite/gdb.base/style-logging.exp b/gdb/testsuite/gdb.base/style-logging.exp
new file mode 100644
index 00000000000..1d7c3052c38
--- /dev/null
+++ b/gdb/testsuite/gdb.base/style-logging.exp
@@ -0,0 +1,70 @@ 
+# Copyright 2019 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test that logging does not style.
+
+# Do not run if gdb debug is enabled as it will interfere with log redirect.
+if {[gdb_debug_enabled]} {
+    untested "debug is enabled"
+    return 0
+}
+
+if {[is_remote host]} {
+    untested "does not work on remote host"
+    return 0
+}
+
+standard_testfile style.c
+
+save_vars { env(TERM) } {
+    # We need an ANSI-capable terminal to get the output.
+    setenv TERM ansi
+
+    if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+	return -1
+    }
+
+    if {![runto_main]} {
+	fail "style tests failed"
+	return
+    }
+
+    gdb_test_no_output "set style enabled on"
+
+    set log_name [standard_output_file log.txt]
+    gdb_test_no_output "set logging file $log_name"
+    gdb_test_no_output "set logging overwrite on"
+    gdb_test "set logging on" "Copying output to .*"
+
+    set main_expr [style main function]
+    set base_file_expr [style ".*style\\.c" file]
+    set file_expr "$base_file_expr:\[0-9\]"
+    set arg_expr [style "arg." variable]
+    gdb_test "frame" \
+	"$main_expr.*$arg_expr.*$arg_expr.*$file_expr.*"
+
+    gdb_test "set logging off" "Done logging to .*"
+
+    set fd [open $log_name]
+    set data [read -nonewline $fd]
+    close $fd
+
+    set testname "log is escape-free"
+    if {[regexp "\033" $data]} {
+	fail $testname
+    } else {
+	pass $testname
+    }
+}
diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index 24c914f442a..05d411aa392 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -396,3 +396,35 @@  tee_file::can_emit_style_escape ()
 	  && m_one->term_out ()
 	  && term_cli_styling ());
 }
+
+/* See ui-file.h.  */
+
+void
+no_terminal_escape_file::write (const char *buf, long length_buf)
+{
+  std::string copy (buf, length_buf);
+  this->puts (copy.c_str ());
+}
+
+/* See ui-file.h.  */
+
+void
+no_terminal_escape_file::puts (const char *buf)
+{
+  while (*buf != '\0')
+    {
+      const char *esc = strchr (buf, '\033');
+      if (esc == nullptr)
+	break;
+
+      int n_read = 0;
+      if (!skip_ansi_escape (esc, &n_read))
+	++esc;
+
+      this->stdio_file::write (buf, esc - buf);
+      buf = esc + n_read;
+    }
+
+  if (*buf != '\0')
+    this->stdio_file::write (buf, strlen (buf));
+}
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index 39f56d5ea42..3f6f38a68fb 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -287,4 +287,20 @@  private:
   ui_file_up m_two;
 };
 
+/* A ui_file implementation that filters out terminal escape
+   sequences.  */
+
+class no_terminal_escape_file : public stdio_file
+{
+public:
+  no_terminal_escape_file ()
+  {
+  }
+
+  /* Like the stdio_file methods, but these filter out terminal escape
+     sequences.  */
+  void write (const char *buf, long length_buf) override;
+  void puts (const char *linebuffer) override;
+};
+
 #endif
diff --git a/gdb/ui-style.h b/gdb/ui-style.h
index 2a87fbe8012..24b4b59ed9f 100644
--- a/gdb/ui-style.h
+++ b/gdb/ui-style.h
@@ -233,8 +233,8 @@  private:
 
 /* Skip an ANSI escape sequence in BUF.  BUF must begin with an ESC
    character.  Return true if an escape sequence was successfully
-   skipped; false otherwise.  In either case, N_READ is updated to
-   reflect the number of chars read from BUF.  */
+   skipped; false otherwise.  If an escape sequence was skipped,
+   N_READ is updated to reflect the number of chars read from BUF.  */
 
 extern bool skip_ansi_escape (const char *buf, int *n_read);