This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 05/11 v5] Add target/target.h
- From: Gary Benson <gbenson at redhat dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: Doug Evans <dje at google dot com>, gdb-patches at sourceware dot org
- Date: Wed, 20 Aug 2014 16:01:10 +0100
- Subject: Re: [PATCH 05/11 v5] Add target/target.h
- Authentication-results: sourceware.org; auth=none
- References: <1406888377-25795-1-git-send-email-gbenson at redhat dot com> <1406888377-25795-6-git-send-email-gbenson at redhat dot com> <21474 dot 27284 dot 80140 dot 944680 at ruffy dot mtv dot corp dot google dot com> <20140807134840 dot GC19737 at blade dot nx> <53F4B55B dot 3020207 at redhat dot 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/