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

Message ID 20190312190320.19645-1-tromey@adacore.com
State New
Headers show
Series
  • [RFC] Don't show "display"s twice in MI
Related show

Commit Message

Tom Tromey March 12, 2019, 7:03 p.m.
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

-- 
2.20.1

Comments

Simon Marchi March 12, 2019, 9:26 p.m. | #1
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
Pedro Alves March 13, 2019, 3:04 p.m. | #2
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
Tom Tromey March 13, 2019, 3:17 p.m. | #3
>>>>> "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
Pedro Alves March 13, 2019, 3:50 p.m. | #4
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
André Pönitz March 13, 2019, 6:43 p.m. | #5
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'
Tom Tromey March 13, 2019, 9:02 p.m. | #6
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:


Pedro> Maybe we could simply move the do_display call elsewhere?

There are several spots that call print_stop_event, so instead of that,
I just added a way for the caller to suppress the displays.

I also addressed your other comments and tried to tighten the test case
a bit.

Let me know what you think.

Tom

commit 930a25c9826c18d0b0e64f2daf7ced7fb9edc3a7
Author: Tom Tromey <tromey@adacore.com>
Date:   Tue Mar 12 12:56:01 2019 -0600

    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-13  Tom Tromey  <tromey@adacore.com>
    
            * mi/mi-interp.c (mi_on_normal_stop_1): Only show displays once.
            * infrun.h (print_stop_event): Add "displays" parameter.
            * infrun.c (print_stop_event): Add "displays" parameter.
    
    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.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 24de651bf92..7273a778424 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-03-13  Tom Tromey  <tromey@adacore.com>
+
+	* mi/mi-interp.c (mi_on_normal_stop_1): Only show displays once.
+	* infrun.h (print_stop_event): Add "displays" parameter.
+	* infrun.c (print_stop_event): Add "displays" parameter.
+
 2019-03-13  Tom Tromey  <tromey@adacore.com>
 
 	* i386-gnu-nat.c (i386_gnu_nat_target::fetch_registers)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 3d7f36b4474..550fbe7f5b9 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -7856,7 +7856,7 @@ print_stop_location (struct target_waitstatus *ws)
 /* See infrun.h.  */
 
 void
-print_stop_event (struct ui_out *uiout)
+print_stop_event (struct ui_out *uiout, bool displays)
 {
   struct target_waitstatus last;
   ptid_t last_ptid;
@@ -7870,7 +7870,8 @@ print_stop_event (struct ui_out *uiout)
     print_stop_location (&last);
 
     /* Display the auto-display expressions.  */
-    do_displays ();
+    if (displays)
+      do_displays ();
   }
 
   tp = inferior_thread ();
diff --git a/gdb/infrun.h b/gdb/infrun.h
index 8f61b75c15b..e53fd81e716 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -167,9 +167,10 @@ extern void print_return_value (struct ui_out *uiout,
 
 /* Print current location without a level number, if we have changed
    functions or hit a breakpoint.  Print source line if we have one.
-   If the execution command captured a return value, print it.  */
+   If the execution command captured a return value, print it.  If
+   DISPLAYS is false, do not call 'do_displays'.  */
 
-extern void print_stop_event (struct ui_out *uiout);
+extern void print_stop_event (struct ui_out *uiout, bool displays = true);
 
 /* Pretty print the results of target_wait, for debugging purposes.  */
 
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index f17e09ff04f..3c5a0d8fb78 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -625,10 +625,15 @@ mi_on_normal_stop_1 (struct bpstats *bs, int print_frame)
 	  reason = tp->thread_fsm->async_reply_reason ();
 	  mi_uiout->field_string ("reason", async_reason_lookup (reason));
 	}
-      print_stop_event (mi_uiout);
 
       console_interp = interp_lookup (current_ui, INTERP_CONSOLE);
-      if (should_print_stop_to_console (console_interp, tp))
+      /* We only want to print the displays once, and we want it to
+	 look just how it would on the console, so we use this to
+	 decide whether the MI stop should include them.  */
+      bool console_print = should_print_stop_to_console (console_interp, tp);
+      print_stop_event (mi_uiout, !console_print);
+
+      if (console_print)
 	print_stop_event (mi->cli_uiout);
 
       mi_uiout->field_int ("thread-id", tp->global_num);
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 037f561595a..d71cc4b1924 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2019-03-12  Tom Tromey  <tromey@adacore.com>
+
+	* gdb.mi/mi2-cli-display.c: New file.
+	* gdb.mi/mi2-cli-display.exp: New file.
+
 2019-03-13  Simon Marchi  <simon.marchi@ericsson.com>
 
 	* mi-breakpoint-location-ena-dis.exp: Rename to ...
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..552612d9430
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi2-cli-display.c
@@ -0,0 +1,32 @@
+/* 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)
+{
+  ++x;
+  ++x;
+  ++x;
+  ++x;
+  return x;
+}
+
+int
+main (void)
+{
+  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..e12fe72964e
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi2-cli-display.exp
@@ -0,0 +1,86 @@
+# 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
+
+# A helper procedure that checks for the display and the line number,
+# and the following *stopped.  X is the expected value of "x" and is
+# also used in test names.
+proc check_cli_display {x show_source} {
+    global mi_gdb_prompt
+
+    # Now check for the display and the source line.  We don't check
+    # the source line too closely, since it's not really important
+    # here, but we do check that the stop happened.
+    set stop "\\*stopped,reason=.*\r\n$mi_gdb_prompt$"
+    if {$show_source} {
+	set src "~\"\[0-9\]+\[^\"\]*\\\\n\"\r\n"
+    } else {
+	set src ""
+    }
+    set display "~\"1: x = $x\\\\n\"\r\n"
+    gdb_expect {
+	-re "^${display}${src}${display}${stop}" {
+	    # This case is the bug: the display is shown twice.
+	    fail "check display and source line x=$x"
+	}
+	-re "^${src}${display}${stop}" {
+	    verbose -log "got <<<$expect_out(buffer)>>>"
+	    pass "check display and source line x=$x"
+	}
+	-re ".*\r\n$mi_gdb_prompt$" {
+	    verbose -log "got <<<$expect_out(buffer)>>>"
+	    fail "check display and source line x=$x (unexpected output)"
+	}
+	timeout {
+	    fail "check display and source line x=$x (timeout)"
+	}
+    }
+}
+
+mi_gdb_test "display x" \
+    "&\"display x\\\\n\"\r\n~\"1: x = 23\\\\n\"\r\n\\^done" \
+    "display x"
+
+if {![mi_send_resuming_command "interpreter-exec console next" next]} {
+    pass "next"
+}
+check_cli_display 24 1
+
+# Also check that displays are shown for -exec-next.
+if {![mi_send_resuming_command exec-next exec-next]} {
+    pass "-exec-next"
+}
+check_cli_display 25 0
Pedro Alves March 19, 2019, 5:46 p.m. | #7
On 03/13/2019 09:02 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

> 

> Pedro> Maybe we could simply move the do_display call elsewhere?

> 

> There are several spots that call print_stop_event, so instead of that,

> I just added a way for the caller to suppress the displays.

> 

> I also addressed your other comments and tried to tighten the test case

> a bit.

> 

> Let me know what you think.


I think it looks good, thanks.

Pedro Alves

Patch

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)
+{
+  ++x;
+  ++x;
+  ++x;
+  ++x;
+  return x;
+}
+
+int main (void)
+{
+  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"
+    }
+    -re ".*\r\n$mi_gdb_prompt$" {
+	fail "check display and source line"
+    }
+    timeout {
+	fail "check display and source line"
+    }
+}
+
+mi_expect_stop end-stepping-range {.*} {.*} {.*} {.*} {.*} "stop for next"