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

[Gary Benson <gbenson@redhat.com>]

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.  So forcing non_stop to 1
beforehand like this patch does means a step over might take place
that would otherwise not have.

In the GDB case forcing non_stop to 1 before target_stop forces GDB
to send a SIGSTOP to each LWP.  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.

I don't know enough about this to know which is the best solution.
Pedro would know, but he's away for the next two weeks.  If what is
happening currently is correct in both cases then maybe a new target
method "target_stop_all" is required to encompass the whole block of
code.

In the interests of keeping things moving would you be ok for me to
commit the following until Pedro is back and able to advise?

      /* FIXME: This conditional code needs removing, either by
         setting non_stop in the same place for both cases or by
	 implementing a new client method for this whole block
	 (less the printing code) from "int was_non_stop = non_stop;"
	 to "non_stop = was_non_stop;".  */
#ifdef GDBSERVER
      target_stop (ptid);
      non_stop = 1;
#else
      non_stop = 1;
      target_stop (ptid);
#endif

Thanks,
Gary

-- 
http://gbenson.net/