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

[Pedro Alves <pedro@palves.net>]

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.

>  {
> -  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.

>  
> -  *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?

> +    }
>  
> -      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.


> +	-re -wrap "$hex.*$decimal.*while \\(worker_blocked\\).*" {
> +	    pass $gdb_test_name
> +	}
> +    }
> +