* Re: [PATCH] PR15693 - Fix spurious *running events, thread state, dprintf-style call
2014-05-16 0:56 ` [PATCH] PR15693 - Fix spurious *running events, thread state, dprintf-style call Pedro Alves
@ 2014-05-16 8:28 ` Hui Zhu
2014-05-29 11:45 ` [pushed] " Pedro Alves
1 sibling, 0 replies; 6+ messages in thread
From: Hui Zhu @ 2014-05-16 8:28 UTC (permalink / raw)
To: Pedro Alves; +Cc: Tom Tromey, gdb-patches ml, Marc Khouzam
Hi Pedro,
Thanks for your work.
Best,
Hui
On Fri, May 16, 2014 at 8:56 AM, Pedro Alves <palves@redhat.com> wrote:
> On 07/31/2013 09:15 AM, Hui Zhu wrote:
>> On Tue, Jul 30, 2013 at 3:24 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:
>>>
>>> Hui> 2013-07-08 Hui Zhu <hui@codesourcery.com>
>>> 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 <hui@codesourcery.com>
>>
>> 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 <palves@redhat.com>
>
> 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 <palves@redhat.com>
> Hui Zhu <hui@codesourcery.com>
>
> 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 <http://www.gnu.org/licenses/>. */
> +
> +/* This is the multi-threaded driver for the real test. */
> +
> +#include <stdlib.h>
> +#include <pthread.h>
> +
> +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 <http://www.gnu.org/licenses/>. */
> +
> +/* 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 <http://www.gnu.org/licenses/>. */
> +
> +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 <http://www.gnu.org/licenses/>.
> +
> +# 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"
> --
> 1.9.0
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* [pushed] Re: [PATCH] PR15693 - Fix spurious *running events, thread state, dprintf-style call
2014-05-16 0:56 ` [PATCH] PR15693 - Fix spurious *running events, thread state, dprintf-style call Pedro Alves
2014-05-16 8:28 ` Hui Zhu
@ 2014-05-29 11:45 ` Pedro Alves
1 sibling, 0 replies; 6+ messages in thread
From: Pedro Alves @ 2014-05-29 11:45 UTC (permalink / raw)
To: Hui Zhu; +Cc: Tom Tromey, gdb-patches ml, Marc Khouzam
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 <palves@redhat.com>
>
> 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 <palves@redhat.com>
> Hui Zhu <hui@codesourcery.com>
>
> 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 <http://www.gnu.org/licenses/>. */
> +
> +/* This is the multi-threaded driver for the real test. */
> +
> +#include <stdlib.h>
> +#include <pthread.h>
> +
> +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 <http://www.gnu.org/licenses/>. */
> +
> +/* 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 <http://www.gnu.org/licenses/>. */
> +
> +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 <http://www.gnu.org/licenses/>.
> +
> +# 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"
>
^ permalink raw reply [flat|nested] 6+ messages in thread