Re: [PATCH v2] gdb: infrun: consume multiple events at each pass in stop_all_threads

[Pedro Alves <palves@redhat.com>]

On 5/15/20 5:06 PM, Simon Marchi wrote:
> On 2020-05-14 2:14 p.m., Pedro Alves wrote:
>> On 5/14/20 7:02 PM, Simon Marchi wrote:
>>
>>> Here's a rebased version.
>>
>> Thanks Simon.  Please go ahead and merge.
>>
>> BTW, did you consider coming up with a mechanism similar to
>> make_scoped_defer_target_commit_resume()/target_commit_resume()
>> for target_stop, so that we could coalesce the multiple vCont;t
>> requests in a single remote packet?  That could also cut down
>> on latency.
>>
>> (gdbserver's resume interface is better for that, in that
>> a stop, continues and steps all go via the same interface:
>>
>>  static void
>>  resume (struct thread_resume *actions, size_t num_actions)
>>  {
>>
>> I've pondered adjusting GDB's resume interface in a similar
>> way.  That would make target_commit_resume Just Work for
>> stops too.
>> )
> 
> No, I haven't considered it, but I think I see what you mean.  To illustrate this case using
> the remote target, I had to set "maintenance set target-non-stop on", while using
> "target non-stop off". 

I think you meant "set non-stop off" in the last command.

> Am I missing something, is there a more common scenario where
> it gets called, using the remote target?

Nope, not missing it -- ideally "maintenance set target-non-stop" would
default to "on", but we're not there yet, unfortunately.

> 
> With the above, when hitting a breakpoint, I do see the stops sent in sequence as part
> of stop_all_threads:
> 
> Sending packet: $vCont;t:p25a703.25a71c#86...Packet received: OK
> Sending packet: $vCont;t:p25a703.25a71d#87...Packet received: OK
> Sending packet: $vCont;t:p25a703.25a71e#88...Packet received: OK
> Sending packet: $vCont;t:p25a703.25a71f#89...Packet received: OK
> Sending packet: $vCont;t:p25a703.25a720#54...Packet received: OK

Exactly.

> 
> which could easily be coalesced.  I think a `target_commit_stop` approach that mimics
> `target_commit_resume` would work, without being too invasive.  But maybe changing
> the `target_stop` interface to accept multiple ptids would be a better approach for
> the future, since it's more of a step towards the gdbserver-style interface that you
> talked about.  In stop_all_threads, it would be quite easy to use: build a vector of
> ptid in this loop:
> 
> 4815           /* Go through all threads looking for threads that we need
> 4816              to tell the target to stop.  */
> 4817           for (thread_info *t : all_non_exited_threads ())
> 
> and call target_stop once after the loop.

I actually forgot you're looking at a native-only target.  But even then,
it may help if the debug API level can aggregate stop requests.  Otherwise,
it probably wouldn't help you.

Thanks,
Pedro Alves