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

[Pedro Alves <palves at redhat dot com>]

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