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

[Simon Marchi]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Test-for-interpreter-exec-mi-breaks-style.patch
Type: text/x-patch
Size: 3411 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/gdb-patches/attachments/20201016/a0d1ac3f/attachment.bin>