Re: [PATCHv2] gdb: better handling of 'S' packets

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

On 2020-12-23 6:09 p.m., Andrew Burgess wrote:
> In V2 I've reduced the patch to focus only on the minimum changes
> required to fix the original bug (PR gdb/26819).
> 
> I think that this new patch is much more obviously the right thing to
> do.
> 
> Once this is merged I'll put together a new patch with the extra
> functionality from V1, but that can come later.
> 
> All feedback welcome.
> 
> Thanks,
> Andrew
> 
> ---
> 
> commit 00190f2b3958e2e822b3d6b078148175f995486c
> Author: Andrew Burgess <andrew.burgess@embecosm.com>
> Date:   Tue Nov 10 17:54:11 2020 +0000
> 
>     gdb: better handling of 'S' packets
>     
>     This commit builds on work started in the following two commits:
>     
>       commit 24ed6739b699f329c2c45aedee5f8c7d2f54e493
>       Date:   Thu Jan 30 14:35:40 2020 +0000
>     
>           gdb/remote: Restore support for 'S' stop reply packet
>     
>       commit cada5fc921e39a1945c422eea055c8b326d8d353
>       Date:   Wed Mar 11 12:30:13 2020 +0000
>     
>           gdb: Handle W and X remote packets without giving a warning
>     
>     This is related to how GDB handles remote targets that send back 'S'
>     packets.
>     
>     In the first of the above commits we fixed GDB's ability to handle a
>     single process, single threaded target that sends back 'S' packets.
>     Although the 'T' packet would always be preferred to 'S' these days,
>     there's nothing really wrong with 'S' for this situation.
>     
>     The second commit above fixed an oversight in the first commit, a
>     single-process, multi-threaded target can send back a process wide
>     event, for example the process exited event 'W' without including a
>     process-id, this also is fine as there is no ambiguity in this case.
>     
>     In PR gdb/26819 we run into yet another problem with the above
>     commits.  In this case we have a single process with two threads, GDB
>     hits a breakpoint in thread 2 and then performs a stepi:
>     
>       (gdb) b main
>       Breakpoint 1 at 0x1212340830: file infinite_loop.S, line 10.
>       (gdb) c
>       Continuing.
>     
>       Thread 2 hit Breakpoint 1, main () at infinite_loop.S:10
>       10    in infinite_loop.S
>       (gdb) set debug remote 1
>       (gdb) stepi
>       Sending packet: $vCont;s:2#24...Packet received: S05
>       ../binutils-gdb/gdb/infrun.c:5807: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.
>     
>     What happens in this case is that on the RISC-V target displaced
>     stepping is not supported, so when the stepi is issued GDB steps just
>     thread 2.  As only a single thread was set running the target decides
>     that is can get away with sending back an 'S' packet without a
>     thread-id.  GDB then associates the stop with thread 1 (the first
>     non-exited thread), but as thread 1 was not previously set executing
>     the assertion seen above triggers.
>     
>     As an aside I am surprised that the target sends pack 'S' in this
>     situation.  The target is happy to send back 'T' (including thread-id)
>     when multiple threads are set running, so (to me) it would seem easier
>     to just always use the 'T' packet when multiple threads are in use.
>     However, the target only uses 'T' when multiple threads are actually
>     executing, otherwise an 'S' packet it used.
>     
>     Still, when looking at the above situation we can see that GDB should
>     be able to understand which thread the 'S' reply is referring too.
>     
>     The problem is that is that in commit 24ed6739b699 (above) when a stop
>     reply comes in with no thread-id we look for the first non-exited
>     thread and select that as the thread the stop applies too.
>     
>     What we should really do is check the threads executing flag too, and
>     so find the first non-exited, executing thread, and associate the stop
>     event with this thread.  In the above example both thread 1 and 2 are
>     non-exited, but only thread 2 is executing, so this is what we should
>     use.
>     
>     Initially I planned to always look for the first non-exited, executing
>     thread, however, this doesn't always work.
>     
>     When GDB initially connects to a target it queries the target for a
>     list of threads.  These threads are created within GDB in the
>     non-executing state.  GDB then asks the target for the last stop
>     reason with the '?'  packet.  If the reply to '?' doesn't include a
>     thread-id then GDB needs to look through all the threads and find a
>     suitable candidate.  At this point no threads will be marked
>     executing, so all we can do is find the first non-exited thread (as we
>     currently do).

I'm thinking about how this could work with my patch that makes the remote
target track its own resume state:

https://sourceware.org/pipermail/gdb-patches/2020-December/174274.html

I'm trying to update that patch to make the remote target track the resume
state even in all-stop, as discussed in that thread.  In all-stop, when we
receive a stop-reply indicating that a thread stopped, all the threads of
the target are assumed to be stopped, so I marked all of them as NOT_RESUMED.

I'm thinking that the remote target could assume that all threads are initially
RESUMED.  In all-stop, we process the stop reply we got in response to '?', and
that will mark all the threads as NOT_RESUMED.

So I think your code could use that new state and implement what you initially
wanted: look for the first non-exited, resumed thread (resumed from the
point of view of the remote target).  And that would simplify things a bit.

>  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)
>  {
> -  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);
>  
> -  *status = stop_reply->ws;
> -  ptid = stop_reply->ptid;
> +  struct remote_state *rs = get_remote_state ();
>  
> -  /* If no thread/process was reported by the stub then use the first
> -     non-exited thread in the current target.  */
> -  if (ptid == null_ptid)
> +  /* Track the possible threads in this structure.  */
> +  struct thread_choices
> +  {
> +    /* Constructor.  */
> +    thread_choices (struct remote_state *rs, bool is_stop_for_all_threads)
> +      : m_rs (rs),
> +	m_is_stop_for_all_threads (is_stop_for_all_threads)
> +    { /* Nothing.  */ }
> +
> +    /* Disable/delete these.  */
> +    thread_choices () = delete;

I don't think it's necessary to delete the default constructor if you have
defined a non-default constructor.

> +    DISABLE_COPY_AND_ASSIGN (thread_choices);
> +
> +    /* Consider thread THR setting the internal thread tracking variables
> +       as appropriate.  */
> +    void consider_thread (thread_info *thr)
> +    {
> +      /* Record the first non-exited thread as a fall-back response.  This
> +	 is only every used during the initial connection to the target.  */

every -> ever

Simon