Re: [PATCH 05/11 v5] Add target/target.h

[Gary Benson <gbenson@redhat.com>]

Pedro Alves wrote:
> On 08/07/2014 02:48 PM, Gary Benson wrote:
> > Doug Evans wrote:
> > > Gary Benson writes:
> > > > @@ -284,37 +253,18 @@ agent_run_command (int pid, const char *cmd,
> > > >        int was_non_stop = non_stop;
> > > >        /* Stop thread PTID.  */
> > > >        DEBUG_AGENT ("agent: stop helper thread\n");
> > > > -#ifdef GDBSERVER
> > > > -      {
> > > > -        struct thread_resume resume_info;
> > > > -
> > > > -        resume_info.thread = ptid;
> > > > -        resume_info.kind = resume_stop;
> > > > -        resume_info.sig = GDB_SIGNAL_0;
> > > > -        (*the_target->resume) (&resume_info, 1);
> > > > -      }
> > > > -
> > > > -      non_stop = 1;
> > > > -      mywait (ptid, &status, 0, 0);
> > > > -#else
> > > >        non_stop = 1;
> > > >        target_stop (ptid);
> > > >  
> > > >        memset (&status, 0, sizeof (status));
> > > >        target_wait (ptid, &status, 0);
> > > > -#endif
> > > >        non_stop = was_non_stop;
> > >
> > > The old gdbserver code set non_stop = 1 *after* asking the target to
> > > stop, whereas now it'll be done before (right?).  Just checking that
> > > that's ok.
> > > E.g., I see a test for non_stop in linux_resume (which feels weird to be
> > > using in this context given that we're talking about target_stop :-)).
> > 
> > Good catch!  I did not notice that change.  I also don't know if it's
> > ok.
> > 
> > In the gdbserver case forcing non_stop to 1 causes need_step_over
> > in linux_resume to become maybe set.  
> 
> > If non_stop had been 0
> > need_step_over would definitely be NULL. 
> 
> That isn't really true, see:
> 
>   any_pending = 0;
>   if (!non_stop)
>     find_inferior (&all_threads, resume_status_pending_p, &any_pending); #1
> ...
>   if (!any_pending && supports_breakpoints ())
>     need_step_over
>       = (struct thread_info *) find_inferior (&all_threads,
> 					      need_step_over_p, NULL);
> 
> If non_stop is 0, then we execute #1 above, true.  But, that may well
> return with ANY_PENDING still clear/0, and so 'need_step_over' may end
> up set anyway.
> 
> So looks fine to me.
> 
> > So forcing non_stop to 1
> > beforehand like this patch does means a step over might take place
> > that would otherwise not have.
> 
> See above.
> 
> > In the GDB case forcing non_stop to 1 before target_stop forces GDB
> > to send a SIGSTOP to each LWP. 
> 
> Note we're just really just stopping one LWP here, the agent helper
> thread, specified in PTID, not all threads.
> 
> > If non_stop had been 0 linux_nat_stop
> > would have fallen back to inf_ptrace_stop which sends one SIGINT to
> > the process group.
> 
> Yeah, we definitely want SIGSTOP, not SIGINT here.  Really, GDB_SIGNAL_0:
> SIGSTOP is how we implement "quiesce with no signal" on Linux -- the
> SIGSTOP is not visible to the target_wait caller.  Unfortunately we have
> a mixup of "interrupt/ctrl-c" vs "quiesce" in the interface.

Pedro, to mirror your new 'target_continue_ptid (ptid_t ptid)' function
suggestion, I thought I might make a new 'target_stop_ptid (ptid_t ptid)'
that would handle the stop/wait combination and the non_stop fiddling.
That way everything will stay exactly as it is.  Does that sound ok to
you?

Thanks,
Gary

-- 
http://gbenson.net/