From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 597 invoked by alias); 29 May 2014 11:45:19 -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 563 invoked by uid 89); 29 May 2014 11:45:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 29 May 2014 11:45:15 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s4TBiu69014504 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 29 May 2014 07:44:56 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s4TBirRI010665; Thu, 29 May 2014 07:44:54 -0400 Message-ID: <53871DB5.10001@redhat.com> Date: Thu, 29 May 2014 11:45:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: Hui Zhu CC: Tom Tromey , gdb-patches ml , Marc Khouzam Subject: [pushed] Re: [PATCH] PR15693 - Fix spurious *running events, thread state, dprintf-style call References: <87ehahnp93.fsf@fleche.redhat.com> <53756234.7070007@redhat.com> In-Reply-To: <53756234.7070007@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-05/txt/msg00724.txt.bz2 I've pushed this in now. Pedro Alves wrote: > 8<------- > Subject: [PATCH] PR15693 - Fix spurious *running events, thread state, > dprintf-style call > > If one sets a breakpoint with a condition that involves calling a > function in the inferior, and then the condition evaluates false, GDB > outputs one *running event for each time the program hits the > breakpoint. E.g., > > $ gdb return-false -i=mi > > (gdb) > start > ... > (gdb) > b 14 if return_false () > &"b 14 if return_false ()\n" > ~"Breakpoint 2 at 0x4004eb: file return-false.c, line 14.\n" > ... > ^done > (gdb) > c > &"c\n" > ~"Continuing.\n" > ^running > *running,thread-id="1" > (gdb) > *running,thread-id="1" > *running,thread-id="1" > *running,thread-id="1" > *running,thread-id="1" > *running,thread-id="1" > *running,thread-id="1" > ... repeat forever ... > > An easy way a user can trip on this is with a dprintf with > dprintf-style set to call. In that case, dprintf calls a function in > the inferior, and the resumes, just like the case above. > > If the breakpoint/dprintf is set in a loop, then these spurious events > can potentially slows down a frontend much, if it decides to refresh > its GUI whenever it sees this event. > > When we run an infcall, we pretend we don't actually run the inferior. > This is already handled for the usual case of calling a function > directly from the CLI: > > (gdb) > p return_false () > &"p return_false ()\n" > ~"$1 = 0" > ~"\n" > ^done > (gdb) > > Note no *running, not *stopped events. That's handled by: > > static void > mi_on_resume (ptid_t ptid) > { > ... > /* Suppress output while calling an inferior function. */ > if (tp->control.in_infcall) > return; > > and equivalent code on normal_stop. > > However, in the cases of the PR, after finishing the infcall there's > one more resume, and mi_on_resume doesn't know that that should be > suppressed too, somehow. > > The "running/stopped" state is a high level user/frontend state. > Internal stops are invisible to the frontend. If follows from that > that we should be setting the thread to running at a higher level > where we still know the set of threads the user _intends_ to resume. > > Currently we mark a thread as running from within target_resume, a low > level target operation. As consequence, today, if we resume a > multi-threaded program while stopped at a breakpoint, we see this: > > -exec-continue > ^running > *running,thread-id="1" > (gdb) > *running,thread-id="all" > > The first *running was GDB stepping over the breakpoint, and the > second is GDB finally resuming everything. > > Between those two *running's, threads other than "1" have bogus still > have their state set to stopped. That bogus -- in async mode, this > opens a tiny window between both resumes where the user might try to > run another execution command to threads other than thread 1, and very > much confuse GDB. > > That is, the "step" below should fail the "step", complaining that the > thread is running: > > (gdb) c -a & > (gdb) thread 2 > (gdb) step > > IOW, threads that GDB happens to not resume immediately (say, because > it needs to step over a breakpoint) shall still be marked as running. > > Then, if we move marking threads as running to a higher layer, > decoupled from target_resume , and also skip marking threads as > running when running an infcall, the spurious *running events > disappear. > > I think we might end up adding a new thread state -- THREAD_INFCALL or > some such, however since infcalls are always synchronous today, I > didn't find a need. There's no way to execute a CLI/MI command > directly from the prompt if some thread is running an infcall. > > Tested on x86_64 Fedora 20. > > gdb/ > 2014-05-16 Pedro Alves > > PR PR15693 > * infrun.c (resume): Determine how much to resume depending on > whether the caller wanted a step, not whether we can hardware step > the target. Mark all threads that we intend to run as running, > unless we're calling an inferior function. > (normal_stop): If the thread is running an infcall, don't finish > thread state. > * target.c (target_resume): Don't mark threads as running here. > > gdb/testsuite/ > 2014-05-16 Pedro Alves > Hui Zhu > > PR PR15693 > * gdb.mi/mi-condbreak-call-thr-state-mt.c: New file. > * gdb.mi/mi-condbreak-call-thr-state-st.c: New file. > * gdb.mi/mi-condbreak-call-thr-state.c: New file. > * gdb.mi/mi-condbreak-call-thr-state.exp: New file. > --- > gdb/infrun.c | 44 ++++++-- > gdb/target.c | 3 +- > .../gdb.mi/mi-condbreak-call-thr-state-mt.c | 61 +++++++++++ > .../gdb.mi/mi-condbreak-call-thr-state-st.c | 26 +++++ > gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state.c | 33 ++++++ > .../gdb.mi/mi-condbreak-call-thr-state.exp | 116 +++++++++++++++++++++ > 6 files changed, 271 insertions(+), 12 deletions(-) > create mode 100644 gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state-mt.c > create mode 100644 gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state-st.c > create mode 100644 gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state.c > create mode 100644 gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state.exp > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 597a188..2e44fef 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -1775,6 +1775,7 @@ resume (int step, enum gdb_signal sig) > CORE_ADDR pc = regcache_read_pc (regcache); > struct address_space *aspace = get_regcache_aspace (regcache); > ptid_t resume_ptid; > + int hw_step = step; > > QUIT; > > @@ -1794,7 +1795,7 @@ resume (int step, enum gdb_signal sig) > if (debug_infrun) > fprintf_unfiltered (gdb_stdlog, > "infrun: resume : clear step\n"); > - step = 0; > + hw_step = 0; > } > > if (debug_infrun) > @@ -1839,7 +1840,7 @@ a command like `return' or `jump' to continue execution.")); > step software breakpoint. */ > if (use_displaced_stepping (gdbarch) > && (tp->control.trap_expected > - || (step && gdbarch_software_single_step_p (gdbarch))) > + || (hw_step && gdbarch_software_single_step_p (gdbarch))) > && sig == GDB_SIGNAL_0 > && !current_inferior ()->waiting_for_vfork_done) > { > @@ -1849,11 +1850,14 @@ a command like `return' or `jump' to continue execution.")); > { > /* Got placed in displaced stepping queue. Will be resumed > later when all the currently queued displaced stepping > - requests finish. The thread is not executing at this point, > - and the call to set_executing will be made later. But we > - need to call set_running here, since from frontend point of view, > - the thread is running. */ > - set_running (inferior_ptid, 1); > + requests finish. The thread is not executing at this > + point, and the call to set_executing will be made later. > + But we need to call set_running here, since from frontend > + point of view, threads were set running. Unless we're > + calling an inferior function, as in that case pretend we > + inferior doesn't run at all. */ > + if (!tp->control.in_infcall) > + set_running (user_visible_resume_ptid (step), 1); > discard_cleanups (old_cleanups); > return; > } > @@ -1863,8 +1867,8 @@ a command like `return' or `jump' to continue execution.")); > pc = regcache_read_pc (get_thread_regcache (inferior_ptid)); > > displaced = get_displaced_stepping_state (ptid_get_pid (inferior_ptid)); > - step = gdbarch_displaced_step_hw_singlestep (gdbarch, > - displaced->step_closure); > + hw_step = gdbarch_displaced_step_hw_singlestep (gdbarch, > + displaced->step_closure); > } > > /* Do we need to do it the hard way, w/temp breakpoints? */ > @@ -1928,6 +1932,14 @@ a command like `return' or `jump' to continue execution.")); > by applying increasingly restricting conditions. */ > resume_ptid = user_visible_resume_ptid (step); > > + /* Even if RESUME_PTID is a wildcard, and we end up resuming less > + (e.g., we might need to step over a breakpoint), from the > + user/frontend's point of view, all threads in RESUME_PTID are now > + running. Unless we're calling an inferior function, as in that > + case pretend we inferior doesn't run at all. */ > + if (!tp->control.in_infcall) > + set_running (resume_ptid, 1); > + > /* Maybe resume a single thread after all. */ > if ((step || singlestep_breakpoints_inserted_p) > && tp->control.trap_expected) > @@ -6172,8 +6184,18 @@ normal_stop (void) > if (has_stack_frames () && !stop_stack_dummy) > set_current_sal_from_frame (get_current_frame (), 1); > > - /* Let the user/frontend see the threads as stopped. */ > - do_cleanups (old_chain); > + /* Let the user/frontend see the threads as stopped, but do nothing > + if the thread was running an infcall. We may be e.g., evaluating > + a breakpoint condition. In that case, the thread had state > + THREAD_RUNNING before the infcall, and shall remain set to > + running, all without informing the user/frontend about state > + transition changes. If this is actually a call command, then the > + thread was originally already stopped, so there's no state to > + finish either. */ > + if (target_has_execution && inferior_thread ()->control.in_infcall) > + discard_cleanups (old_chain); > + else > + do_cleanups (old_chain); > > /* Look up the hook_stop and run it (CLI internally handles problem > of stop_command's pre-hook not existing). */ > diff --git a/gdb/target.c b/gdb/target.c > index 1b48f79..c99b9c7 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -2149,8 +2149,9 @@ target_resume (ptid_t ptid, int step, enum gdb_signal signal) > gdb_signal_to_name (signal)); > > registers_changed_ptid (ptid); > + /* We only set the internal executing state here. The user/frontend > + running state is set at a higher level. */ > set_executing (ptid, 1); > - set_running (ptid, 1); > clear_inline_frame_state (ptid); > } > > diff --git a/gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state-mt.c b/gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state-mt.c > new file mode 100644 > index 0000000..112a5cb > --- /dev/null > +++ b/gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state-mt.c > @@ -0,0 +1,61 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright (C) 2014 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 . */ > + > +/* This is the multi-threaded driver for the real test. */ > + > +#include > +#include > + > +extern int test (void); > + > +#define NTHREADS 5 > +pthread_barrier_t barrier; > + > +void * > +thread_func (void *arg) > +{ > + pthread_barrier_wait (&barrier); > + > + while (1) > + sleep (1); > +} > + > +void > +create_thread (void) > +{ > + pthread_t tid; > + > + if (pthread_create (&tid, NULL, thread_func, NULL)) > + { > + perror ("pthread_create"); > + exit (1); > + } > +} > + > +int > +main (int argc, char *argv[]) > +{ > + int i; > + > + pthread_barrier_init (&barrier, NULL, NTHREADS + 1); > + > + for (i = 0; i < NTHREADS; i++) > + create_thread (); > + pthread_barrier_wait (&barrier); > + > + return test (); > +} > diff --git a/gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state-st.c b/gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state-st.c > new file mode 100644 > index 0000000..66335dd > --- /dev/null > +++ b/gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state-st.c > @@ -0,0 +1,26 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright (C) 2014 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 . */ > + > +/* This is single-threaded driver for the real test. */ > + > +extern int test (void); > + > +int > +main () > +{ > + return test (); > +} > diff --git a/gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state.c b/gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state.c > new file mode 100644 > index 0000000..75d5601 > --- /dev/null > +++ b/gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state.c > @@ -0,0 +1,33 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright (C) 2013-2014 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 . */ > + > +int > +return_false (void) > +{ > + return 0; > +} > + > +int > +test (void) > +{ > + int a = 0; > + > + while (a < 10) > + a++; /* set breakpoint here */ > + > + return 0; /* set end breakpoint here */ > +} > diff --git a/gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state.exp b/gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state.exp > new file mode 100644 > index 0000000..82ca6cb > --- /dev/null > +++ b/gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state.exp > @@ -0,0 +1,116 @@ > +# Copyright (C) 2013-2014 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 . > + > +# Regression test for PR15693. A breakpoint with a condition that > +# calls a function that evaluates false would result in a spurious > +# *running event sent to the frontend each time the breakpoint is hit > +# (and the target re-resumed). Like: > +# > +# -exec-continue > +# ^running > +# *running,thread-id="all" > +# (gdb) > +# *running,thread-id="1" > +# *running,thread-id="1" > +# *running,thread-id="1" > +# *running,thread-id="1" > +# *running,thread-id="1" > +# ... > + > +load_lib mi-support.exp > +set MIFLAGS "-i=mi" > + > +# Run either the multi-threaded or the single-threaded variant of the > +# test, as determined by VARIANT. > +proc test { variant } { > + global gdb_test_file_name > + global testfile srcdir subdir srcfile srcfile2 binfile > + global mi_gdb_prompt async > + > + with_test_prefix "$variant" { > + gdb_exit > + if [mi_gdb_start] { > + continue > + } > + > + set options {debug} > + if {$variant == "mt" } { > + lappend options "pthreads" > + } > + > + # Don't use standard_testfile as we need a different binary > + # for each variant. > + set testfile $gdb_test_file_name-$variant > + set binfile [standard_output_file ${testfile}] > + set srcfile $testfile.c > + set srcfile2 $gdb_test_file_name.c > + > + if {[build_executable "failed to prepare" \ > + $testfile \ > + "${srcfile} ${srcfile2}" \ > + $options] == -1} { > + return -1 > + } > + > + mi_delete_breakpoints > + mi_gdb_reinitialize_dir $srcdir/$subdir > + mi_gdb_reinitialize_dir $srcdir/$subdir > + mi_gdb_load ${binfile} > + > + mi_runto test > + > + # Leave the breakpoint at test set, on purpose. The next > + # resume shall emit a single '*running,thread-id="all"', even > + # if GDB needs to step over a breakpoint (that is, even if GDB > + # needs to run only one thread for a little bit). > + > + set bp_location [gdb_get_line_number "set breakpoint here" $srcfile2] > + set bp_location_end [gdb_get_line_number "set end breakpoint here" $srcfile2] > + > + mi_gdb_test "-break-insert -c return_false() $srcfile2:$bp_location" ".*" \ > + "insert conditional breakpoint" > + > + mi_gdb_test "-break-insert $srcfile2:$bp_location_end" ".*" \ > + "insert end breakpoint" > + > + set msg "no spurious *running notifications" > + send_gdb "-exec-continue\n" > + gdb_expect { > + -re "\\*running.*\\*running.*\\*stopped" { > + fail $msg > + } > + -re "\\^running\r\n\\*running,thread-id=\"all\"\r\n${mi_gdb_prompt}.*\\*stopped" { > + pass $msg > + } > + timeout { > + fail "$msg (timeout)" > + } > + } > + > + # In sync mode, there's an extra prompt after *stopped. Consume it. > + if {!$async} { > + gdb_expect { > + -re "$mi_gdb_prompt" { > + } > + } > + } > + } > +} > + > +# Single-threaded. > +test "st" > + > +# Multi-threaded. > +test "mt" >