From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 66001 invoked by alias); 13 Mar 2019 15:04:30 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 65494 invoked by uid 89); 13 Mar 2019 15:04:30 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=frontend's, displays, H*M:9c80, HX-Languages-Length:6971 X-HELO: mail-wr1-f43.google.com Received: from mail-wr1-f43.google.com (HELO mail-wr1-f43.google.com) (209.85.221.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 13 Mar 2019 15:04:27 +0000 Received: by mail-wr1-f43.google.com with SMTP id g12so2393216wrm.5 for ; Wed, 13 Mar 2019 08:04:27 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id x13sm1721248wrw.14.2019.03.13.08.04.24 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 13 Mar 2019 08:04:24 -0700 (PDT) Subject: Re: [RFC] Don't show "display"s twice in MI To: Tom Tromey , gdb-patches@sourceware.org References: <20190312190320.19645-1-tromey@adacore.com> From: Pedro Alves Message-ID: <0a23b883-9c80-a9e2-1e3e-3aa8c0b0ce13@redhat.com> Date: Wed, 13 Mar 2019 15:04:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190312190320.19645-1-tromey@adacore.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-03/txt/msg00267.txt.bz2 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 > > * infrun.c (print_stop_event): Don't call do_displays for MI-like > ui-outs. > > gdb/testsuite/ChangeLog > 2019-03-12 Tom Tromey > > * 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 . */ > + > +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 . > + > +# 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