Re: [PATCH v3 09/17] Teach non-stop to do in-line step-overs (stop all, step, restart)

[Doug Evans <dje@google.com>]

Pedro Alves writes:
  > Hi Doug,
  >
  > Thanks for your comments, and sorry for the delay in getting
  > back to answering them.
  >
  > On 04/27/2015 08:27 PM, Doug Evans wrote:
  >
  > >  > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
  > >  > index 31869f1..e23d223 100644
  > >  > --- a/gdb/breakpoint.c
  > >  > +++ b/gdb/breakpoint.c
  > >  > @@ -468,6 +468,8 @@ breakpoints_should_be_inserted_now (void)
  > >  >      }
  > >  >    else if (target_has_execution)
  > >  >      {
  > >  > +      struct thread_info *tp;
  > >  > +
  > >  >        if (always_inserted_mode)
  > >  >  	{
  > >  >  	  /* The user wants breakpoints inserted even if all threads
  > >  > @@ -477,6 +479,13 @@ breakpoints_should_be_inserted_now (void)
  > >  >
  > >  >        if (threads_are_executing ())
  > >  >  	return 1;
  > >
  > > Not a problem introduced by this patch, but as an fyi, the terminology
  > > employed here is a bit confusing.
  > > Why would we want to insert breakpoints into executing threads,
  > > or when threads are executing? That's what a simple reading of this
  > > code says is happening. This reading can't be correct of course. :-)
  >
  > Right.  :-)
  >
  > >
  > >  > +
  > >  > +      /* Don't remove breakpoints yet if, even though all threads  
are
  > >  > +	 stopped, we still have events to process.  */
  > >  > +      ALL_NON_EXITED_THREADS (tp)
  > >  > +	if (tp->resumed
  > >  > +	    && tp->suspend.waitstatus_pending_p)
  > >  > +	  return 1;
  > >
  > > Plus, this function is named "breakpoints_should_be_inserted_now"
  > > but the comment is talking about whether breakpoints should be removed.
  >
  > Yeah, because if breakpoints are inserted now, and the function returns
  > false (meaning, they shouldn't be inserted now", they'll be removed.

Ah. Though that's caller-specific: not all callers remove breakpoints
if this returns false.

  > > Can you elaborate on how to interpret the name of this function?
  > > Guessing at how I'm supposed to interpret what this function is for,
  > > is a better name
  >  
> "breakpoints_should_have_been_inserted_by_now_or_should_remain_inserted"?
  > > [Not that that's my recommendation :-). Just trying to understand how
  > > to read this function.]
  >
  > You got it right, but I'm afraid I lack the English skills to come
  > up with a better name.  "be inserted" is the state we want to reach, not
  > an action. Maybe "should_breakpoints_be_inserted_now" is little clearer,
  > but it still doesn't distinguish "state" vs "action".  Because
  > state("be inserted"=false) is clearly "no breakpoints on target",
  > while action("be inserted"=false) could mean that whatever
  > breakpoint is already inserted remains inserted.

I think I can manage with this change:

-      /* Don't remove breakpoints yet if, even though all threads are
-         stopped, we still have events to process.  */
+      /* We still need breakpoints, even though all threads are
+         stopped, if we still have events to process.  */

But I'll submit that separately to not interfere with
this patchset.

  > >  >  typedef struct value *value_ptr;
  > >  > @@ -183,6 +201,14 @@ struct thread_info
  > >  >       thread is off and running.  */
  > >  >    int executing;
  > >  >
  > >  > +  /* Non-zero if this thread will be/has been resumed.  Note that a
  > >  > +     thread can be marked both as not-executing and resumed at the
  > >  > +     same time.  This happens if we try to resume a thread that  
has a
  > >  > +     wait status pending.  We shouldn't let the thread run until  
that
  > >  > +     wait status has been processed, but we should not process that
  > >  > +     wait status if we didn't try to let the thread run.  */
  > >  > +  int resumed;
  > >
  > > I suspect this will be another source of confusion, but I don't have
  > > a good suggestion at the moment for how to improve it.
  > > The "will be" in the comment speaks in future tense, but the name
  > > "resumed" is past tense. Maybe (though I'd have to spend more time
  > > reading the code to be sure) it would make sense to have this be
  > > multi-state: not-resumed, to-be-resumed, and resumed; thus splitting up
  > > "will be" resumed from "has been" resumed.
  >
  > I've changed this to:
  >
  >   /* Non-zero if this thread is resumed from infrun's perspective.
  >      Note that a thread can be marked both as not-executing and
  >      resumed at the same time.  This happens if we try to resume a
  >      thread that has a wait status pending.  We shouldn't let the
  >      thread really run until that wait status has been processed, but
  >      we should not process that wait status if we didn't try to let
  >      the thread run.  */
  >   int resumed;

Thanks.

  > >  > +  if (tp->suspend.waitstatus_pending_p)
  > >  > +    {
  > >  > +      if (debug_infrun)
  > >  > +	{
  > >  > +	  char *statstr;
  > >  > +
  > >  > +	  statstr = target_waitstatus_to_string (&tp->suspend.waitstatus);
  > >  > +	  fprintf_unfiltered (gdb_stdlog,
  > >  > +			      "infrun: resume: thread %s has pending wait status %s "
  > >  > +			      "(currently_stepping=%d).\n",
  > >  > +			      target_pid_to_str (tp->ptid),  statstr,
  > >  > +			      currently_stepping (tp));
  > >  > +	  xfree (statstr);
  > >
  > > Not something that has to be done with this patch of course,
  > > but it's nice that we don't have to track the memory of  
target_pid_to_str;
  > > IWBN to be able to do the same for target_waitstatus_to_string.
  > > [In C++ it could just return a string, and we *could* just wait for  
C++.
  > > Just a thought.]
  >
  > I'm just going with the flow you yourself created.  ;-)

What's the point of saying something like that?
I'm going to assume you don't disagree with the improvement.

  > >
  > >  > +	}
  > >  > +
  > >  > +      tp->resumed = 1;
  > >  > +      /* Avoid confusing the next resume, if the next stop/resume
  > >  > +	 happens to apply to another thread.  */
  > >  > +      tp->suspend.stop_signal = GDB_SIGNAL_0;
  > >
  > > This is a bit confusing. How will the value of stop_signal in one
  > > thread affect stop/resume in another thread?
  >
  > The wonders of copy/paste led to that comment percolating all the way
  > up here.  I first added that comment in 2020b7abd8 elsewhere, when
  > stop_signal was converted to be per-thread instead of a global.
  > With all the 7.9 changes to make gdb pass signals to the right
  > threads, I think this comment no longer makes sense.  I'll remove it.
  >
  > > Plus, the reader is left worried that we just clobbered the signal
  > > that we need to resume thread tp with. Can you elaborate on what's
  > > happening here?
  >
  > Yeah good point.  I recall wanting to mention something about
  > this, but forgot.  This is the same situation we already have here
  > in linux-nat.c's target_resume:
  >
  >   /* If we have a pending wait status for this thread, there is no
  >      point in resuming the process.  But first make sure that
  >      linux_nat_wait won't preemptively handle the event - we
  >      should never take this short-circuit if we are going to
  >      leave LP running, since we have skipped resuming all the
  >      other threads.  This bit of code needs to be synchronized
  >      with linux_nat_wait.  */
  >
  >   if (lp->status && WIFSTOPPED (lp->status))
  >     {
  >       if (!lp->step
  > 	  && WSTOPSIG (lp->status)
  > 	  && sigismember (&pass_mask, WSTOPSIG (lp->status)))
  > 	{
  > 	  if (debug_linux_nat)
  > 	    fprintf_unfiltered (gdb_stdlog,
  > 				"LLR: Not short circuiting for ignored "
  > 				"status 0x%x\n", lp->status);
  >
  > 	  /* FIXME: What should we do if we are supposed to continue
  > 	     this thread with a signal?  */
  > 	  gdb_assert (signo == GDB_SIGNAL_0);
  > 	  signo = gdb_signal_from_host (WSTOPSIG (lp->status));
  > 	  lp->status = 0;
  > 	}
  >     }
  >
  > Probably the only thing we can do is queue the new signal
  > to deliver later, somehow.   gdbserver's Linux backend handles
  > that with linux-low.c's lwp->pending_signals list.
  >
  > Since Mark added that FIXME back in 2000 already, and this
  > is only enabled on native Linux for now, and this is only
  > reachable with gdbserver + non-stop on cases we're mishandle
  > before anyway, we're not losing anything by not handling
  > this yet.
  >
  > So I think we should be fine with adding a warning for now:
  >
  >       /* FIXME: What should we do if we are supposed to resume this
  > 	 thread with a signal?  Maybe we should maintain a queue of
  > 	 pending signals to deliver.  */
  >       if (sig != GDB_SIGNAL_0)
  > 	{
  > 	  warning (_("Couldn't deliver signal %s to %s.\n"),
  > 		   gdb_signal_to_name (sig), target_pid_to_str (tp->ptid));
  > 	}

"works for me"

  >
  > I ran this against the testsuite and nothing exercises this today,
  > as expected; but I know there's a PR about the assert above.
  >
  > >  > +      if (pc != tp->suspend.stop_pc)
  > >  > +	{
  > >  > +	  if (debug_infrun)
  > >  > +	    fprintf_unfiltered (gdb_stdlog,
  > >  > +				"infrun: PC of %s changed.  was=%s, now=%s\n",
  > >  > +				target_pid_to_str (tp->ptid),
  > >  > +				paddress (target_gdbarch (), tp->prev_pc),
  > >  > +				paddress (target_gdbarch (), pc));
  > >
  > > s/target_gdbarch ()/gdbarch/ ?
  >
  > Fixed.  Below as well.
  >
  > >  > +wait_one (struct target_waitstatus *ws)
  > >  > +{
  > >  > +  ptid_t event_ptid;
  > >  > +  ptid_t wait_ptid = minus_one_ptid;
  > >  > +
  > >  > +  overlay_cache_invalid = 1;
  > >  > +
  > >  > +  /* Flush target cache before starting to handle each event.
  > >  > +     Target was running and cache could be stale.  This is just a
  > >  > +     heuristic.  Running threads may modify target memory, but we
  > >  > +     don't get any event.  */
  > >  > +  target_dcache_invalidate ();
  > >  > +
  > >  > +  if (deprecated_target_wait_hook)
  > >  > +    event_ptid = deprecated_target_wait_hook (wait_ptid, ws, 0);
  > >  > +  else
  > >  > +    event_ptid = target_wait (wait_ptid, ws, 0);
  > >  > +
  > >  > +  if (debug_infrun)
  > >  > +    print_target_wait_results (wait_ptid, event_ptid, ws);
  > >  > +
  > >  > +  if (ws->kind == TARGET_WAITKIND_SYSCALL_ENTRY
  > >  > +      || ws->kind == TARGET_WAITKIND_SYSCALL_RETURN)
  > >  > +    ws->value.syscall_number = UNKNOWN_SYSCALL;
  > >
  > > IWBN to have a comment explaining why we set UNKNOWN_SYSCALL here.
  >
  > Good question.  I honestly don't recall.  I ran the testsuite now
  > with that removed, and nothing broke.  Dropped.
  >
  > >  > +/* Stop all threads.  */
  > >  > +
  > >  > +static void
  > >  > +stop_all_threads (void)
  > >  > +{
  > >  > +  /* We may need multiple passes to discover all threads.  */
  > >  > +  int pass;
  > >  > +  int iterations = 0;
  > >  > +  ptid_t entry_ptid;
  > >  > +  struct cleanup *old_chain;
  > >  > +
  > >  > +  gdb_assert (non_stop);
  > >  > +
  > >  > +  if (debug_infrun)
  > >  > +    fprintf_unfiltered (gdb_stdlog, "infrun: stop_all_threads\n");
  > >  > +
  > >  > +  entry_ptid = inferior_ptid;
  > >  > +  old_chain = make_cleanup (switch_to_thread_cleanup, &entry_ptid);
  > >  > +
  > >  > +  /* Stop threads in two passes since threads could be spawning as  
we
  > >  > +     go through the first pass.  In the second pass, we will stop  
such
  > >  > +     spawned threads.  */
  > >  > +  for (pass = 0; pass < 2; pass++, iterations++)
  > >
  > > Can you rephrase the "Stop threads in two passes ... In the second  
pass ..."
  > > comment? What's happening here is that we keep iterating until two  
passes
  > > find no new threads (IIUC).
  >
  > You're right.  Here's what I got now:
  >
  >   /* Request threads to stop, and then wait for the stops.  Because
  >      threads we already know about can spawn more threads while we're
  >      trying to stop them, and we only learn about new threads when we
  >      update the thread list, do this in a loop, and keep iterating
  >      until two passes find no threads that need to be stopped.  */
  >   for (pass = 0; pass < 2; pass++, iterations++)
  >     {
  >
  > Here's what I'm squashing to the original patch.  Let me know
  > what you think.
  >
  >  gdb/infrun.c | 30 ++++++++++++++++++------------
  >  1 file changed, 18 insertions(+), 12 deletions(-)
  >
  > diff --git a/gdb/infrun.c b/gdb/infrun.c
  > index 9f3da09..540ca87 100644
  > --- a/gdb/infrun.c
  > +++ b/gdb/infrun.c
  > @@ -2262,8 +2262,16 @@ resume (enum gdb_signal sig)
  >  	}
  >
  >        tp->resumed = 1;
  > -      /* Avoid confusing the next resume, if the next stop/resume
  > -	 happens to apply to another thread.  */
  > +
  > +      /* FIXME: What should we do if we are supposed to resume this
  > +	 thread with a signal?  Maybe we should maintain a queue of
  > +	 pending signals to deliver.  */
  > +      if (sig != GDB_SIGNAL_0)
  > +	{
  > +	  warning (_("Couldn't deliver signal %s to %s.\n"),
  > +		   gdb_signal_to_name (sig), target_pid_to_str (tp->ptid));
  > +	}
  > +
  >        tp->suspend.stop_signal = GDB_SIGNAL_0;
  >        discard_cleanups (old_cleanups);
  >
  > @@ -3313,8 +3321,8 @@ do_target_wait (ptid_t ptid, struct  
target_waitstatus *status, int options)
  >  	    fprintf_unfiltered (gdb_stdlog,
  >  				"infrun: PC of %s changed.  was=%s, now=%s\n",
  >  				target_pid_to_str (tp->ptid),
  > -				paddress (target_gdbarch (), tp->prev_pc),
  > -				paddress (target_gdbarch (), pc));
  > +				paddress (gdbarch, tp->prev_pc),
  > +				paddress (gdbarch, pc));
  >  	  discard = 1;
  >  	}
  >        else if (!breakpoint_inserted_here_p (get_regcache_aspace  
(regcache), pc))
  > @@ -3323,7 +3331,7 @@ do_target_wait (ptid_t ptid, struct  
target_waitstatus *status, int options)
  >  	    fprintf_unfiltered (gdb_stdlog,
  >  				"infrun: previous breakpoint of %s, at %s gone\n",
  >  				target_pid_to_str (tp->ptid),
  > -				paddress (target_gdbarch (), pc));
  > +				paddress (gdbarch, pc));
  >
  >  	  discard = 1;
  >  	}
  > @@ -4008,10 +4016,6 @@ wait_one (struct target_waitstatus *ws)
  >    if (debug_infrun)
  >      print_target_wait_results (wait_ptid, event_ptid, ws);
  >
  > -  if (ws->kind == TARGET_WAITKIND_SYSCALL_ENTRY
  > -      || ws->kind == TARGET_WAITKIND_SYSCALL_RETURN)
  > -    ws->value.syscall_number = UNKNOWN_SYSCALL;
  > -
  >    return event_ptid;
  >  }
  >
  > @@ -4146,9 +4150,11 @@ stop_all_threads (void)
  >    entry_ptid = inferior_ptid;
  >    old_chain = make_cleanup (switch_to_thread_cleanup, &entry_ptid);
  >
  > -  /* Stop threads in two passes since threads could be spawning as we
  > -     go through the first pass.  In the second pass, we will stop such
  > -     spawned threads.  */
  > +  /* Request threads to stop, and then wait for the stops.  Because
  > +     threads we already know about can spawn more threads while we're
  > +     trying to stop them, and we only learn about new threads when we
  > +     update the thread list, do this in a loop, and keep iterating
  > +     until two passes find no threads that need to be stopped.  */
  >    for (pass = 0; pass < 2; pass++, iterations++)
  >      {
  >        if (debug_infrun)
  > --
  > 1.9.3
  >
  >

Thanks.