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

Simon Marchi via Gdb-patches
2021-01-06T21:20:05Z

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