[RFC] Don't show "display"s twice in MI

  If you run "gdb -i=mi2" and set a "display", then when "next"ing the
displays will be shown twice:

    ~"1: x = 23\n"
    ~"7\t  printf(\"%d\\n\", x);\n"
    ~"1: x = 23\n"
    *stopped,reason="end-stepping-range",frame={addr="0x0000000000400565",func="main",args=[],file="q.c",fullname="/tmp/q.c",line="7"},thread-id="1",stopped-threads="all",core="1"

The immediate cause of this is this code in mi_on_normal_stop_1:

      print_stop_event (mi_uiout);

      console_interp = interp_lookup (current_ui, INTERP_CONSOLE);
      if (should_print_stop_to_console (console_interp, tp))
	print_stop_event (mi->cli_uiout);

... which obviously prints the stop twice.

However, I think the first call to print_stop_event is intended just
to emit the MI *stopped notification, which explains why the source
line does not show up two times.

This patch fixes the bug by changing print_stop_event to only call
do_displays for non-MI-like ui-outs.

Tested on x86-64 Fedora 29.

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

	* infrun.c (print_stop_event): Don't call do_displays for MI-like
	ui-outs.

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

	* gdb.mi/mi2-cli-display.c: New file.
	* gdb.mi/mi2-cli-display.exp: New file.
---
 gdb/ChangeLog                            |  5 ++
 gdb/infrun.c                             |  3 +-
 gdb/testsuite/ChangeLog                  |  5 ++
 gdb/testsuite/gdb.mi/mi2-cli-display.c   | 30 ++++++++++++
 gdb/testsuite/gdb.mi/mi2-cli-display.exp | 62 ++++++++++++++++++++++++
 5 files changed, 104 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi2-cli-display.c
 create mode 100644 gdb/testsuite/gdb.mi/mi2-cli-display.exp
  

On 2019-03-12 15:03, Tom Tromey wrote:
> If you run "gdb -i=mi2" and set a "display", then when "next"ing the
> displays will be shown twice:
> 
>     ~"1: x = 23\n"
>     ~"7\t  printf(\"%d\\n\", x);\n"
>     ~"1: x = 23\n"
> 
> *stopped,reason="end-stepping-range",frame={addr="0x0000000000400565",func="main",args=[],file="q.c",fullname="/tmp/q.c",line="7"},thread-id="1",stopped-threads="all",core="1"
> 
> The immediate cause of this is this code in mi_on_normal_stop_1:
> 
>       print_stop_event (mi_uiout);
> 
>       console_interp = interp_lookup (current_ui, INTERP_CONSOLE);
>       if (should_print_stop_to_console (console_interp, tp))
> 	print_stop_event (mi->cli_uiout);
> 
> ... which obviously prints the stop twice.
> 
> However, I think the first call to print_stop_event is intended just
> to emit the MI *stopped notification, which explains why the source
> line does not show up two times.
> 
> This patch fixes the bug by changing print_stop_event to only call
> do_displays for non-MI-like ui-outs.

FWIW, this is fine with me.  Maybe just format the C file of the test 
case according to GNU style.

Simon
  

On 03/12/2019 07:03 PM, Tom Tromey wrote:
> If you run "gdb -i=mi2" and set a "display", then when "next"ing the
> displays will be shown twice:
> 
>     ~"1: x = 23\n"
>     ~"7\t  printf(\"%d\\n\", x);\n"
>     ~"1: x = 23\n"
>     *stopped,reason="end-stepping-range",frame={addr="0x0000000000400565",func="main",args=[],file="q.c",fullname="/tmp/q.c",line="7"},thread-id="1",stopped-threads="all",core="1"
> 
> The immediate cause of this is this code in mi_on_normal_stop_1:
> 
>       print_stop_event (mi_uiout);
> 
>       console_interp = interp_lookup (current_ui, INTERP_CONSOLE);
>       if (should_print_stop_to_console (console_interp, tp))
> 	print_stop_event (mi->cli_uiout);
> 
> ... which obviously prints the stop twice.
> 
> However, I think the first call to print_stop_event is intended just
> to emit the MI *stopped notification, which explains why the source
> line does not show up two times.
> 
> This patch fixes the bug by changing print_stop_event to only call
> do_displays for non-MI-like ui-outs.

Yeah, this was previously discussed here:

  https://sourceware.org/ml/gdb/2018-06/msg00006.html

See my comment there:

> Fixing this probably needs to take in consideration whether
> we print the displays on the CLI uiout even if the stop
> isn't printed there according to should_print_stop_to_console.
> I.e., what makes more sense for a user that enabled some display,
> but then stepped with the frontend's "step" buttons instead of
> typing "next" or "step".  Probably we shouldn't print the
> displays in that case, just to keep things simple, respecting
> should_print_stop_to_console, but not 100% sure.

So your patch makes GDB not do the displays in the
-exec-step/-exec-next case, which is the solution I was
leaning to above too, even though I'm not 100% sure about it.
I think that change should be covered by the testcase too, though.
I.e., check that displays aren't printed in the -exec-step/-exec-next
cases (they are without your patch), and that they are in the
-exec-continue case (I believe they are with your patch).

> 
> Tested on x86-64 Fedora 29.
> 
> gdb/ChangeLog
> 2019-03-12  Tom Tromey  <tromey@adacore.com>
> 
> 	* infrun.c (print_stop_event): Don't call do_displays for MI-like
> 	ui-outs.
> 
> gdb/testsuite/ChangeLog
> 2019-03-12  Tom Tromey  <tromey@adacore.com>
> 
> 	* gdb.mi/mi2-cli-display.c: New file.
> 	* gdb.mi/mi2-cli-display.exp: New file.
> ---
>  gdb/ChangeLog                            |  5 ++
>  gdb/infrun.c                             |  3 +-
>  gdb/testsuite/ChangeLog                  |  5 ++
>  gdb/testsuite/gdb.mi/mi2-cli-display.c   | 30 ++++++++++++
>  gdb/testsuite/gdb.mi/mi2-cli-display.exp | 62 ++++++++++++++++++++++++
>  5 files changed, 104 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/testsuite/gdb.mi/mi2-cli-display.c
>  create mode 100644 gdb/testsuite/gdb.mi/mi2-cli-display.exp
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 33e5d434b35..9261588db32 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -7867,7 +7867,8 @@ print_stop_event (struct ui_out *uiout)
>      print_stop_location (&last);
>  
>      /* Display the auto-display expressions.  */
> -    do_displays ();
> +    if (!uiout->is_mi_like_p ())
> +      do_displays ();
>    }
>  
>    tp = inferior_thread ();
> diff --git a/gdb/testsuite/gdb.mi/mi2-cli-display.c b/gdb/testsuite/gdb.mi/mi2-cli-display.c
> new file mode 100644
> index 00000000000..813fda47cfc
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi2-cli-display.c
> @@ -0,0 +1,30 @@
> +/* Copyright 2019 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   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 do_tests (int x)

Like break after return type?

> +{
> +  ++x;
> +  ++x;
> +  ++x;
> +  ++x;
> +  return x;
> +}
> +
> +int main (void)

Ditto.

> +{
> +  return do_tests (23);
> +}
> diff --git a/gdb/testsuite/gdb.mi/mi2-cli-display.exp b/gdb/testsuite/gdb.mi/mi2-cli-display.exp
> new file mode 100644
> index 00000000000..b11306a51cd
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi2-cli-display.exp
> @@ -0,0 +1,62 @@
> +# 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/>.
> +
> +# Ensure that CLI "display"s aren't double-emitted in MI mode.
> +
> +load_lib mi-support.exp
> +set MIFLAGS "-i=mi2"
> +
> +if {[mi_gdb_start]} {
> +    continue
> +}
> +
> +standard_testfile
> +
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
> +    untested "failed to compile"
> +    return -1
> +}
> +
> +mi_delete_breakpoints
> +mi_gdb_reinitialize_dir $srcdir/$subdir
> +mi_gdb_load ${binfile}
> +
> +mi_runto do_tests
> +
> +mi_gdb_test "display x" \
> +    "&\"display x\\\\n\"\r\n~\"1: x = 23\\\\n\"\r\n\\^done" \
> +    "display x"
> +
> +mi_send_resuming_command "interpreter-exec console next" next
> +
> +# Now check for the display and the source line.  Note we don't check
> +# the source line too closely, since it's not really important here.
> +gdb_expect {
> +    -re "~\"1: x = 24\\\\n\"\r\n~\"\[0-9\]+.*\\\\n\"\r\n~\"1: x = 24\\\\n\"\r\n" {
> +	# This case is the bug: the display is shown twice.
> +	fail "check display and source line"
> +    }
> +    -re "~\"\[0-9\]+.*\\\\n\"\r\n~\"1: x = 24\\\\n\"\r\n" {
> +	pass "check display and source line"

Hard to tell, but just to double-check -- is the pass regex a
subset of the fail regex case above?  I'm just wondering whether
this could issue a false pass if expect consumes just the right amount
of the bad case's output.  A comment indicating otherwise here would
be good, IMO.

> +    }
> +    -re ".*\r\n$mi_gdb_prompt$" {
> +	fail "check display and source line"
> +    }
> +    timeout {
> +	fail "check display and source line"

Add " (timeout)" ?

> +    }
> +}
> +
> +mi_expect_stop end-stepping-range {.*} {.*} {.*} {.*} {.*} "stop for next"
> 

Thanks,
Pedro Alves
  

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

>> Probably we shouldn't print the displays in that case, just to keep
>> things simple, respecting should_print_stop_to_console, but not 100%
>> sure.

Pedro> So your patch makes GDB not do the displays in the
Pedro> -exec-step/-exec-next case, which is the solution I was
Pedro> leaning to above too, even though I'm not 100% sure about it.

I'm not 100% sure either.

We could have a more complicated patch that arranges for do_displays to
be called just once, no matter what decision is made.  Maybe this would
be better?

I originally thought it was somewhat odd to deal with displays in an MI
stepping situation -- MI clients presumably would use varobj.  But,
really the scenario is that the MI client provides a console, the user
types "display ...", and then debugs some more.  I suppose the way that
the "next" is done wouldn't matter to the user?

Tom
  

On 03/13/2019 03:17 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
>>> Probably we shouldn't print the displays in that case, just to keep
>>> things simple, respecting should_print_stop_to_console, but not 100%
>>> sure.
> 
> Pedro> So your patch makes GDB not do the displays in the
> Pedro> -exec-step/-exec-next case, which is the solution I was
> Pedro> leaning to above too, even though I'm not 100% sure about it.
> 
> I'm not 100% sure either.
> 
> We could have a more complicated patch that arranges for do_displays to
> be called just once, no matter what decision is made.  Maybe this would
> be better?

Maybe we could simply move the do_display call elsewhere?

> I originally thought it was somewhat odd to deal with displays in an MI
> stepping situation -- MI clients presumably would use varobj.  

Yeah, that's not the way to look at it, IMO.  

> But, really the scenario is that the MI client provides a console, the user
> types "display ...", and then debugs some more.  I suppose the way that
> the "next" is done wouldn't matter to the user?

Yeah.  Thinking a bit more, I think I'd be surprised if my console-created "display"
wasn't redisplayed in the console regardless of whether I pressed a "next" button,
or typed "next" in the console.  Kind of the mirror view of creating a
"watch expression" thingy in Eclipse (or whatever), which is implemented with
varobjs, and then that widget not updating with "next" in the console.

Thanks,
Pedro Alves
  

On Wed, Mar 13, 2019 at 09:17:48AM -0600, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> >> Probably we shouldn't print the displays in that case, just to keep
> >> things simple, respecting should_print_stop_to_console, but not 100%
> >> sure.
> 
> Pedro> So your patch makes GDB not do the displays in the
> Pedro> -exec-step/-exec-next case, which is the solution I was
> Pedro> leaning to above too, even though I'm not 100% sure about it.
> 
> I'm not 100% sure either.
> 
> We could have a more complicated patch that arranges for do_displays to
> be called just once, no matter what decision is made.  Maybe this would
> be better?
> 
> I originally thought it was somewhat odd to deal with displays in an MI
> stepping situation -- MI clients presumably would use varobj. 

That's possibly a bit too general: As counterexample, I'd call Qt Creator
an "MI client" but it doesn't use varobj.

On the other hand, I would not use "display" in that setup either (there's a
separate window to evaluate expression that gets updated after each stop)
so any number of copies of the value in the output is fine in that
situation.

> But, really the scenario is that the MI client provides a console, the
> user types "display ...", and then debugs some more.  I suppose the way
> that the "next" is done wouldn't matter to the user?

Probably not, indeed.

Andre'