Re: [PATCH v3 5/5] gdb: better handling of 'S' packets

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-01-09 4:26 p.m., Pedro Alves wrote:
> On 08/01/21 04:17, Simon Marchi wrote:
> 
>> @@ -7796,75 +7799,117 @@ remote_notif_get_pending_events (remote_target *remote, notif_client *nc)
>>    remote->remote_notif_get_pending_events (nc);
>>  }
>>  
>> -/* Called when it is decided that STOP_REPLY holds the info of the
>> -   event that is to be returned to the core.  This function always
>> -   destroys STOP_REPLY.  */
>> +/* Called from process_stop_reply when the stop packet we are responding
>> +   to didn't include a process-id or thread-id.  STATUS is the stop event
>> +   we are responding to.
>> +
>> +   It is the task of this function to select a suitable thread (or process)
>> +   and return its ptid, this is the thread (or process) we will assume the
>> +   stop event came from.
>> +
>> +   In some cases there isn't really any choice about which thread (or
>> +   process) is selected, a basic remote with a single process containing a
>> +   single thread might choose not to send any process-id or thread-id in
>> +   its stop packets, this function will select and return the one and only
>> +   thread.
>> +
>> +   However, if a target supports multiple threads (or processes) and still
>> +   doesn't include a thread-id (or process-id) in its stop packet then
>> +   first, this is a badly behaving target, and second, we're going to have
>> +   to select a thread (or process) at random and use that.  This function
>> +   will print a warning to the user if it detects that there is the
>> +   possibility that GDB is guessing which thread (or process) to
>> +   report.  */
>>  
>>  ptid_t
>> -remote_target::process_stop_reply (struct stop_reply *stop_reply,
>> -				   struct target_waitstatus *status)
>> +remote_target::select_thread_for_ambiguous_stop_reply
>> +  (const struct target_waitstatus *status)
> 
> Note that this is called before gdb fetches the updated thread list,
> so the stop reply may be ambiguous without gdb realizing, if
> the inferior spawned new threads, but the stop is for the thread
> that was resumed.  Maybe the comment should mention that.
> 
> For this reason, I see this patch more as being lenient to the stub,
> than fixing a GDB bug with misimplementing the remote protocol.

I don't really understand this.

> 
>>  {
>> -  ptid_t ptid;
>> +  /* Some stop events apply to all threads in an inferior, while others
>> +     only apply to a single thread.  */
>> +  bool is_stop_for_all_threads
>> +    = (status->kind == TARGET_WAITKIND_EXITED
>> +       || status->kind == TARGET_WAITKIND_SIGNALLED);
> 
> I didn't mention this before, but I keep having the same thought, so I'd
> better speak up.  :-)  I find "stop is for all threads" ambiguous with
> all-stop vs non-stop.  I'd suggest something like "process_wide_stop",
> I think it would work.

Agreed, will fix.

> 
>>  
>> -  *status = stop_reply->ws;
>> -  ptid = stop_reply->ptid;
>> +  thread_info *first_resumed_thread = nullptr;
>> +  bool multiple_resumed_thread = false;
>>  
>> -  /* If no thread/process was reported by the stub then use the first
>> -     non-exited thread in the current target.  */
>> -  if (ptid == null_ptid)
>> +  /* Consider all non-exited threads of the target, find the first resumed
>> +     one.  */
>> +  for (thread_info *thr : all_non_exited_threads (this))
>>      {
>> -      /* Some stop events apply to all threads in an inferior, while others
>> -	 only apply to a single thread.  */
>> -      bool is_stop_for_all_threads
>> -	= (status->kind == TARGET_WAITKIND_EXITED
>> -	   || status->kind == TARGET_WAITKIND_SIGNALLED);
>> +      remote_thread_info *remote_thr =get_remote_thread_info (thr);
>> +
>> +      if (remote_thr->resume_state () != resume_state::RESUMED)
>> +	continue;
>> +
>> +      if (first_resumed_thread == nullptr)
>> +	first_resumed_thread = thr;
> 
> 
>> +      else if (!is_stop_for_all_threads
>> +	       || first_resumed_thread->ptid.pid () != thr->ptid.pid ())
>> +	multiple_resumed_thread = true;
> 
> The connection between the condition and whether there are multiple
> resumed threads seems mysterious and distracting to me.  For a variable
> called multiple_resumed_thread(s), I would have expected instead:
> 
>       if (first_resumed_thread == nullptr)
> 	first_resumed_thread = thr;
>       else
>         multiple_resumed_threads = true;
> 
> maybe something like "bool ambiguous;" would be more to the point?

Makes sense.

> 
>> +    }
>>  
>> -      for (thread_info *thr : all_non_exited_threads (this))
>> +  gdb_assert (first_resumed_thread != nullptr);
>> +
>> +  /* Warn if the remote target is sending ambiguous stop replies.  */
>> +  if (multiple_resumed_thread)
>> +    {
>> +      static bool warned = false;
>> +
> 
> 
>> +    # Single step thread 2.  Only the one thread will step.  When the
>> +    # thread stops, if the stop packet doesn't include a thread-id
>> +    # then GDB should still understand which thread stopped.
>> +    gdb_test_multiple "stepi" "" {
>> +	-re "Thread 1 received signal SIGTRAP" {
>> +	    fail $gdb_test_name
>> +	}
> 
> This is still missing consuming the prompt.  I'll leave deciding whether
> this -re need to be here to Andrew, but it is kept, but should consume
> the problem, since otherwise we will leave the prompt in the expect
> buffer and confuse the next gdb_test.  Just adding -wrap would do, I think.


> Otherwise this LGTM.

Thanks, I'll address the comments and push patches 1, 2 and 5.

Simon