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

[Pedro Alves <palves@redhat.com>]

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.

> 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,
Pedro Alves