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

Andrew Burgess
2021-01-07T09:57:40Z

* 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