From patchwork Fri May 16 00:56:20 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 956 Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx20.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id EB80A36006F for ; Thu, 15 May 2014 17:56:33 -0700 (PDT) Received: by homiemail-mx20.g.dreamhost.com (Postfix, from userid 14314964) id A1BA541DC3D38; Thu, 15 May 2014 17:56:33 -0700 (PDT) X-Original-To: gdb@patchwork.siddhesh.in Delivered-To: x14314964@homiemail-mx20.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx20.g.dreamhost.com (Postfix) with ESMTPS id 774A341DC3D82 for ; Thu, 15 May 2014 17:56:33 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; q=dns; s=default; b=dp/KTw5/c8swTauv lmOVEgUVySKlECeFtcuWViVEfXQgrVnu9CEFc4M9iqwkrvlP+975rwLF6GVdIJyS MEcCu233HQYHs90agKedX0a5LBP/pNAEnT2kQgNh4Wm74IKa8+EHxAhqmJkgp/nC aMWXoCoaPOrL2CDBrEehkFgVPbA= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; s=default; bh=8qPVQhKJNV0AxVoT5TYi2j eMlEs=; b=wjYKxxr3dfN1gke+FbX0oz+p5qWaeITRRfUUzWyih9AUqz3wOAFsns lDNxW3FYft7NeW29ZEiGI/Nq4DxNUAsertPsyHyMHCOsJxGq1POnWb28zDMqi7CB 2neSzSEBtS+nn2g4SsyoHuihMrYBta94Vbj865uY37VnHK8Av6a8E= Received: (qmail 11036 invoked by alias); 16 May 2014 00:56:30 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 11008 invoked by uid 89); 16 May 2014 00:56:29 -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; Fri, 16 May 2014 00:56:26 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s4G0uMe9003474 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 15 May 2014 20:56:22 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s4G0uK2X009211; Thu, 15 May 2014 20:56:21 -0400 Message-ID: <53756234.7070007@redhat.com> Date: Fri, 16 May 2014 01:56:20 +0100 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: [PATCH] PR15693 - Fix spurious *running events, thread state, dprintf-style call References: <87ehahnp93.fsf@fleche.redhat.com> In-Reply-To: X-DH-Original-To: gdb@patchwork.siddhesh.in On 07/31/2013 09:15 AM, Hui Zhu wrote: > On Tue, Jul 30, 2013 at 3:24 AM, Tom Tromey wrote: >>>>>>> "Hui" == Hui Zhu writes: >> >> Hui> 2013-07-08 Hui Zhu >> Hui> PR gdb/15693 >> Hui> infcall.c (run_inferior_call): Save value of call_thread->state >> Hui> and set it back. >> >> I didn't see a response to this. >> I think this needs a test case. >> >> Tom > > Hi Tom, > > I make a test for it. Sorry that it took _so_ long to get this reviewed/fixed. But better late than never... I was going to approve this this morning, with a few tweaks (the test as is is racy and doesn't actually fail with an unfixed GDB), but then I realized that the fix isn't right in case the inferior has multiple threads. If I run the test in the patch below against your patch, I get this ... -exec-continue ^running *running,thread-id="all" (gdb) *running,thread-id="all" *running,thread-id="all" *running,thread-id="all" *running,thread-id="all" *running,thread-id="all" *running,thread-id="all" *running,thread-id="all" *running,thread-id="all" *running,thread-id="all" *running,thread-id="all" =breakpoint-modified,bkpt={number="3",type="breakpoint",disp="keep",enabled="y",addr="0x000000000040086a",func="test",file="../../../src/gdb/testsuite/gdb.mi/mi-condbreak-c all-thr-state.c",fullname="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state.c",line="32",thread-groups=["i1"],times="1",original-location="mi-cond break-call-thr-state.c:32"} *stopped,reason="breakpoint-hit",disp="keep",bkptno="3",frame={addr="0x000000000040086a",func="test",args=[],file="../../../src/gdb/testsuite/gdb.mi/mi-condbreak-call-thr-s tate.c",fullname="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/mi-condbreak-call-thr-state.c",line="32"},thread-id="1",stopped-threads="all",core="2" FAIL: gdb.mi/mi-condbreak-call-thr-state.exp: mt: no spurious *running notifications > 2013-07-31 Hui Zhu > > PR gdb/15693 > * infcall.c (run_inferior_call): Save value of call_thread->state > and set it back. ... because this only saves/restores the state of _one_ thread. So even though the current thread's state is reset to "running", the other threads end up marked as stopped, and so the next resume (because the breakpoint doesn't cause a stop) emits *running events anyway... So I thought about this a lot today, and come up with the patch below. Let me know what you think. 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"