Re: [PATCH] PR15693 - Fix spurious *running events, thread state, dprintf-style call

[Hui Zhu <teawater at gmail dot com>]

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
>
>