[PATCH] gdb: Preserve gdb_std{out, err, ...} across interpreter-exec mi

[PATCH] gdb: Preserve gdb_std{out, err, ...} across interpreter-exec mi

[Peter Waller]

Hi,

This is my first attempt at submitting a patch to GDB.
Please fill me in if I'm missing any process.

Calls through interpreter_exec_cmd can cause the output state to be 
modified in
a way which doesn't get back after the execution.

It looks like the intent is that interp::resume should put things back 
how they
should be, however, mi_interp::resume modifies gdb_stdout and nothing 
currently
restores it to the previous state.

To see the broken behaviour:

   # (backtrace -> use MI -> backtrace).
   gdb -ex starti -ex bt -ex 'interpreter-exec mi echo' -ex bt -ex q 
echo <<<''

Prior to this patch, on a terminal environment, the first backtrace is
coloured, and the second backtrace is not. The reason is that
stdio_file::can_emit_style_escape becomes false, because the gdb_stdout gets
overwritten in mi_interp::resume and not replaced.

This fix feels like plastering over some underlying issues, so I'm 
submitting
it as a straw man in the hopes of provoking a better fix.

The fix is motivated because it manifests in cgdb, which uses 
interpreter-exec
to retrieve details of the process state for displaying useful 
information to
the user. I've filed a bug there to bring attention to the issue:
https://github.com/cgdb/cgdb/issues/242

Signed-off-by: Peter Waller <p@pwaller.net>
---
  gdb/interps.c | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/gdb/interps.c b/gdb/interps.c
index 4b2e3fd37b0..fd7c4c587f1 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -370,6 +370,14 @@ interpreter_exec_cmd (const char *args, int from_tty)
    unsigned int nrules;
    unsigned int i;

+  /* interpreters may clobber stdout/stderr (e.g. in mi_interp::resume 
at time
+     of writing), preserve their state across interpreter-exec. */
+  scoped_restore save_stdout = make_scoped_restore (&gdb_stdout);
+  scoped_restore save_stderr = make_scoped_restore (&gdb_stderr);
+  scoped_restore save_stdlog = make_scoped_restore (&gdb_stdlog);
+  scoped_restore save_stdtarg = make_scoped_restore (&gdb_stdtarg);
+  scoped_restore save_stdtargerr = make_scoped_restore (&gdb_stdtargerr);
+
    if (args == NULL)
      error_no_arg (_("interpreter-exec command"));

-- 
2.25.1

Re: [PATCH] gdb: Preserve gdb_std{out, err, ...} across interpreter-exec mi

[Simon Marchi]

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

On 2020-10-16 5:51 p.m., Peter Waller via Gdb-patches wrote:

Hi Peter,

> Hi,
>
> This is my first attempt at submitting a patch to GDB.
> Please fill me in if I'm missing any process.

My first suggestion will be to send your patch using git-send-email, as
I am not able to apply your patch.  If you send it using an email client
(which I presume you did), it is almost guaranteed that is will get
mangled in the process and not possible to apply.  Note that this patch
in particular is small enough that I can apply it by hand, but for the
future.

Also, it's a bit of a nit, but your message contains a mix of
non-breaking spaces (UTF-8 0xc2a0) and regular spaces (0x20).  Can you
make sure to use regular spaces for your commit message?

>
> Calls through interpreter_exec_cmd can cause the output state to be
> modified in
> a way which doesn't get back after the execution.
>
> It looks like the intent is that interp::resume should put things back
> how they
> should be, however, mi_interp::resume modifies gdb_stdout and nothing
> currently
> restores it to the previous state.
>
> To see the broken behaviour:
>
>    # (backtrace -> use MI -> backtrace).
>    gdb -ex starti -ex bt -ex 'interpreter-exec mi echo' -ex bt -ex q
> echo <<<''
>
> Prior to this patch, on a terminal environment, the first backtrace is
> coloured, and the second backtrace is not. The reason is that
> stdio_file::can_emit_style_escape becomes false, because the gdb_stdout gets
> overwritten in mi_interp::resume and not replaced.

I can reproduce it.  Interestingly, the output from the "print"
command is styled (e.g when doing "print main").

> ---
>   gdb/interps.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/gdb/interps.c b/gdb/interps.c
> index 4b2e3fd37b0..fd7c4c587f1 100644
> --- a/gdb/interps.c
> +++ b/gdb/interps.c
> @@ -370,6 +370,14 @@ interpreter_exec_cmd (const char *args, int from_tty)
>     unsigned int nrules;
>     unsigned int i;
>
> +  /* interpreters may clobber stdout/stderr (e.g. in mi_interp::resume
> at time
> +     of writing), preserve their state across interpreter-exec. */

GDB-style comment: use a capital letter at the beginning, and two spaces
after periods (including the final one).

> +  scoped_restore save_stdout = make_scoped_restore (&gdb_stdout);
> +  scoped_restore save_stderr = make_scoped_restore (&gdb_stderr);
> +  scoped_restore save_stdlog = make_scoped_restore (&gdb_stdlog);
> +  scoped_restore save_stdtarg = make_scoped_restore (&gdb_stdtarg);
> +  scoped_restore save_stdtargerr = make_scoped_restore (&gdb_stdtargerr);

This is one way of doing it.  Another way would be: since
mi_interp::resume sets up gdb_std* to the stream it wants, perhaps
cli_interp::resume and tui_interp::resume should just do the same.  I
have no idea what would be better, this area is a bit blurry to me.

I've written and attached a test that reproduces the issue, you can add
it to your patch.

Simon

[-- Attachment #2: 0001-Test-for-interpreter-exec-mi-breaks-style.patch --]
[-- Type: text/x-patch, Size: 3410 bytes --]

From cd905dab02376123bc375afeb3d7a2114998acb5 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Fri, 16 Oct 2020 22:54:14 -0400
Subject: [PATCH] Test for "interpreter-exec mi breaks style"

Change-Id: Ic5350cbeb0f48092d882577af1805330d13ba789
---
 gdb/testsuite/gdb.base/style-interp-exec-mi.c | 22 +++++++++
 .../gdb.base/style-interp-exec-mi.exp         | 46 +++++++++++++++++++
 2 files changed, 68 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/style-interp-exec-mi.c
 create mode 100644 gdb/testsuite/gdb.base/style-interp-exec-mi.exp

diff --git a/gdb/testsuite/gdb.base/style-interp-exec-mi.c b/gdb/testsuite/gdb.base/style-interp-exec-mi.c
new file mode 100644
index 000000000000..9d7b2f1a4c28
--- /dev/null
+++ b/gdb/testsuite/gdb.base/style-interp-exec-mi.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 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/>.  */
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/style-interp-exec-mi.exp b/gdb/testsuite/gdb.base/style-interp-exec-mi.exp
new file mode 100644
index 000000000000..f3f010e24a45
--- /dev/null
+++ b/gdb/testsuite/gdb.base/style-interp-exec-mi.exp
@@ -0,0 +1,46 @@
+# Copyright 2020 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/>.
+
+# It was observed that when doing:
+#
+#   - backtrace
+#   - an "interpreter-exec mi" command
+#   - backtrace again
+#
+# The second backtrace would not be styled.  This test tests that.
+
+standard_testfile
+
+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}] } {
+	return
+    }
+
+    if { ![runto_main] } {
+	untested "could not run to main"
+	return
+    }
+
+    gdb_test_no_output "set style enabled on"
+    set main_expr [style main function]
+    gdb_test "backtrace" ".* ${main_expr} .*" "backtrace before"
+    gdb_test "interpreter-exec mi \"-data-evaluate-expression 1\"" \
+	"\\^done,value=\"1\""
+    gdb_test "backtrace" ".* ${main_expr} .*" "backtrace after"
+}
-- 
2.28.0

Re: [PATCH] gdb: Preserve gdb_std{out, err, ...} across interpreter-exec mi

[Peter Waller]

Thanks very much for the quick reply. Not at my machine to improve the patch but quick comment below.

On 17 October 2020 04:19:18 BST, Simon Marchi <simark@simark.ca> wrote:
>On 2020-10-16 5:51 p.m., Peter Waller via Gdb-patches wrote:
>> +  scoped_restore save_stdout = make_scoped_restore (&gdb_stdout);
>> +  scoped_restore save_stderr = make_scoped_restore (&gdb_stderr);
>> +  scoped_restore save_stdlog = make_scoped_restore (&gdb_stdlog);
>> +  scoped_restore save_stdtarg = make_scoped_restore (&gdb_stdtarg);
>> +  scoped_restore save_stdtargerr = make_scoped_restore
>(&gdb_stdtargerr);
>
>This is one way of doing it.  Another way would be: since
>mi_interp::resume sets up gdb_std* to the stream it wants, perhaps
>cli_interp::resume and tui_interp::resume should just do the same.  I
>have no idea what would be better, this area is a bit blurry to me.

I thought the same. The other interpreters don't appear to retain such streams. Additionally, gdb_setup_readline messes with the streams in a way which also appears to cause this problem. Therefore I landed on scoped restore to force the invariant that the state should be the same across the call to hold.

I think there might also be a leak in gdb_setup_readline, because gdb_stdout is assigned a new stream each time which looks odd. It doesn't obviously look like it is ever freed.

>I've written and attached a test that reproduces the issue, you can add
>it to your patch.

Thanks very much for this!

I intend to combine our patches and fix the niggles you identified. I'll see if I can reply using get send email with --in-reply-to. If that fails I'll attach the patch in thunderbird.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[PATCH 1/1] Preserve gdb_std{out, err, log, targ, targerr} across interpreter_exec_cmd

[Peter Waller]

Calls through interpreter_exec_cmd can cause the output state to be modified in
a way which doesn't get back after the execution.

It looks like the intent is that interp::resume should put things back how they
should be, however, mi_interp::resume modifies gdb_stdout and nothing currently
restores it to the previous state.

To see the broken behaviour:

  gdb -ex starti -ex bt -ex 'interpreter-exec mi echo' -ex bt -ex q echo <<<''

Prior to this patch, on a terminal environment, the first backtrace is
coloured, and the second backtrace is not. The reason is that
stdio_file::can_emit_style_escape becomes false, because the gdb_stdout gets
overwritten in mi_interp::resume and not replaced.

Signed-off-by: Peter Waller <p@pwaller.net>
---
 gdb/interps.c                                 |  8 ++++
 gdb/testsuite/gdb.base/style-interp-exec-mi.c | 22 +++++++++
 .../gdb.base/style-interp-exec-mi.exp         | 46 +++++++++++++++++++
 3 files changed, 76 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/style-interp-exec-mi.c
 create mode 100644 gdb/testsuite/gdb.base/style-interp-exec-mi.exp

diff --git a/gdb/interps.c b/gdb/interps.c
index 4b2e3fd37b0..e186a736786 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -370,6 +370,14 @@ interpreter_exec_cmd (const char *args, int from_tty)
   unsigned int nrules;
   unsigned int i;
 
+  /* Interpreters may clobber stdout/stderr (e.g.  in mi_interp::resume at time
+     of writing), preserve their state here.  */
+  scoped_restore save_stdout = make_scoped_restore (&gdb_stdout);
+  scoped_restore save_stderr = make_scoped_restore (&gdb_stderr);
+  scoped_restore save_stdlog = make_scoped_restore (&gdb_stdlog);
+  scoped_restore save_stdtarg = make_scoped_restore (&gdb_stdtarg);
+  scoped_restore save_stdtargerr = make_scoped_restore (&gdb_stdtargerr);
+
   if (args == NULL)
     error_no_arg (_("interpreter-exec command"));
 
diff --git a/gdb/testsuite/gdb.base/style-interp-exec-mi.c b/gdb/testsuite/gdb.base/style-interp-exec-mi.c
new file mode 100644
index 00000000000..9d7b2f1a4c2
--- /dev/null
+++ b/gdb/testsuite/gdb.base/style-interp-exec-mi.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 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/>.  */
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/style-interp-exec-mi.exp b/gdb/testsuite/gdb.base/style-interp-exec-mi.exp
new file mode 100644
index 00000000000..f3f010e24a4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/style-interp-exec-mi.exp
@@ -0,0 +1,46 @@
+# Copyright 2020 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/>.
+
+# It was observed that when doing:
+#
+#   - backtrace
+#   - an "interpreter-exec mi" command
+#   - backtrace again
+#
+# The second backtrace would not be styled.  This test tests that.
+
+standard_testfile
+
+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}] } {
+	return
+    }
+
+    if { ![runto_main] } {
+	untested "could not run to main"
+	return
+    }
+
+    gdb_test_no_output "set style enabled on"
+    set main_expr [style main function]
+    gdb_test "backtrace" ".* ${main_expr} .*" "backtrace before"
+    gdb_test "interpreter-exec mi \"-data-evaluate-expression 1\"" \
+	"\\^done,value=\"1\""
+    gdb_test "backtrace" ".* ${main_expr} .*" "backtrace after"
+}
-- 
2.25.1

Re: [PATCH 1/1] Preserve gdb_std{out, err, log, targ, targerr} across interpreter_exec_cmd

[Simon Marchi]

On 2020-10-17 6:06 a.m., Peter Waller via Gdb-patches wrote:
> Calls through interpreter_exec_cmd can cause the output state to be modified in
> a way which doesn't get back after the execution.
> 
> It looks like the intent is that interp::resume should put things back how they
> should be, however, mi_interp::resume modifies gdb_stdout and nothing currently
> restores it to the previous state.
> 
> To see the broken behaviour:
> 
>   gdb -ex starti -ex bt -ex 'interpreter-exec mi echo' -ex bt -ex q echo <<<''
> 
> Prior to this patch, on a terminal environment, the first backtrace is
> coloured, and the second backtrace is not. The reason is that
> stdio_file::can_emit_style_escape becomes false, because the gdb_stdout gets
> overwritten in mi_interp::resume and not replaced.
> 
> Signed-off-by: Peter Waller <p@pwaller.net>

I just stumbled on this patch by searching for something else and realized it
was never followed-up on.  Feel free to ping regularly if you have no answer,
because things tend to fall through the cracks otherwise.

I'll run the testsuite see if that causes any regressions, and if not I'll
probably merge it after giving it a second look.

Simon

Re: [PATCH 1/1] Preserve gdb_std{out, err, log, targ, targerr} across interpreter_exec_cmd

[Simon Marchi]

On 2020-12-20 9:51 p.m., Simon Marchi via Gdb-patches wrote:
> On 2020-10-17 6:06 a.m., Peter Waller via Gdb-patches wrote:
>> Calls through interpreter_exec_cmd can cause the output state to be modified in
>> a way which doesn't get back after the execution.
>>
>> It looks like the intent is that interp::resume should put things back how they
>> should be, however, mi_interp::resume modifies gdb_stdout and nothing currently
>> restores it to the previous state.
>>
>> To see the broken behaviour:
>>
>>   gdb -ex starti -ex bt -ex 'interpreter-exec mi echo' -ex bt -ex q echo <<<''
>>
>> Prior to this patch, on a terminal environment, the first backtrace is
>> coloured, and the second backtrace is not. The reason is that
>> stdio_file::can_emit_style_escape becomes false, because the gdb_stdout gets
>> overwritten in mi_interp::resume and not replaced.
>>
>> Signed-off-by: Peter Waller <p@pwaller.net>
> 
> I just stumbled on this patch by searching for something else and realized it
> was never followed-up on.  Feel free to ping regularly if you have no answer,
> because things tend to fall through the cracks otherwise.
> 
> I'll run the testsuite see if that causes any regressions, and if not I'll
> probably merge it after giving it a second look.

Ok, I just pushed this patch, thanks again for sending it.

Simon