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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-01-06 16:19:54 -0500]:

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

OK, I'll wait for a while to see if you post a revised version of the
above patch, then update this to match.

Thanks,
Andrew


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