`qThreadExtraInfo` support

[thefaxman]

While porting probe-rs to use gdbstub I found two packets that would be helpful to respond to:

qC - this queries the current thread ID. WIthout responding GDB works correctly, but the client emits a message that may be confusing to end users. Edit: qC is no longer needed after debugging an issue with ? in the thread.

qThreadExtraInfo - this allows the stub to provide a string of extra data about a thread. It's a free form string that is displayed on the client and can be useful for adding extra detailed status or thread names, for example.

Both of these would produce a better experience for the probe-rs logic and I'd be glad to take a shot at producing a PR that adds the logic. Before I do I wanted some feedback on how to approach this.

Both packets feel like appropriate functions on the MultiThreadBase trait. To avoid a break I could add them with default implementations - qC could return the first value from list_active_threads and qThreadExtraInfo could return a 0 byte string by default. This avoids a break, but it puts a bit more burden on the implementor to realize these are options for them to extend.

Any thoughts / feedback on this?

[daniel5151]

Yep, both of these seems like useful packets to support!

As you've identified, both of these packets are not required to get a GDB session up and running, and only serve to enhance the GDB experience. As such, they should be added to gdbstub in both a non-breaking, and a non-bloating manner, using the typical IDET approach.

To elaborate on what I mean by "non-bloating": gdbstub is a no_std first project, and tries to keep the binary footprint of the library in minimal configurations as small as possible. As such, any new optional features are checked to make sure that they are optimized away on targets that don't end up using them.

In terms of implementation: it should be pretty straightforward to support these as sub-IDETs of the MultiThreadBase trait. That is to say - I wouldn't include any of those suggested fallback implementations. Rather, if the user doesn't opt-in to these extensions, the code should just fall back to what it currently does (i.e: nothing).

[daniel5151]

Also, could you elaborate on what you mean by "the client emits a message that may be confusing to end users"?

[thefaxman]

Also, could you elaborate on what you mean by "the client emits a message that may be confusing to end users"?

In my local testing (gdb 11.2) I get the following warning upon connecting:

warning: multi-threaded target stopped without sending a thread-id, using first non-exited thread

When looking at the gdbstub logs this seems to correlate to not responding to qC. Here's the gdbstub logging for packets not responded to:

 INFO  gdbstub::stub::core_impl    > Unknown command: Ok("vMustReplyEmpty")
 INFO  gdbstub::stub::core_impl    > Unknown command: Ok("qTStatus")
 INFO  gdbstub::stub::core_impl    > Unknown command: Ok("qC")
 INFO  gdbstub::stub::core_impl    > Unknown command: Ok("qOffsets")
 INFO  gdbstub::stub::core_impl    > Unknown command: Ok("qSymbol::")

My read is the client is expecting qC to be supported if threads are, and is recovering from the stub not responding "correctly".

[thefaxman]

Actually, I'm not 100% certain the qC issue is the cause of that warning. I'll debug more when I get a response flowing but it may be unrelated.

Here's the gdb logs that are leading to this warning:

[remote] start_remote: enter
  [remote] Sending packet: $qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;fork-events+;vfork-events+;exec-events+;vContSupported+;QThreadEvents+;no-resumed+;memory-tagging+#ec
  [remote] Received Ack
  [remote] Packet received: PacketSize=1000;vContSupported+;multiprocess+;QStartNoAckMode+;swbreak+;hwbreak+;qXfer:features:read+
  [remote] packet_ok: Packet qSupported (supported-packets) is supported
  [remote] Sending packet: $vMustReplyEmpty#3a
  [remote] Received Ack
  [remote] Packet received: 
  [remote] Sending packet: $QStartNoAckMode#b0
  [remote] Received Ack
  [remote] Packet received: OK
  [remote] Sending packet: $Hgp0.0#ad
  [remote] Packet received: OK
  [remote] Sending packet: $qXfer:features:read:target.xml:0,ffb#79
  [remote] Packet received: m<target version="1.0"><architecture>armv4t</architecture></target>
  [remote] Sending packet: $qXfer:features:read:target.xml:42,ffb#af
  [remote] Packet received: l
  [remote] Sending packet: $qTStatus#49
  [remote] Packet received: 
  [remote] packet_ok: Packet qTStatus (trace-status) is NOT supported
  [remote] Sending packet: $?#3f
  [remote] Packet received: S05
  [remote] Sending packet: $qfThreadInfo#bb
  [remote] Packet received: mp01.01,p01.02
  [remote] Sending packet: $qsThreadInfo#c8
  [remote] Packet received: l
  [remote] Sending packet: $qAttached:1#fa
  [remote] Packet received: 1
  [remote] packet_ok: Packet qAttached (query-attached) is supported
  [remote] Sending packet: $Hc-1#09
  [remote] Packet received: OK
  [remote] Sending packet: $qC#b4
  [remote] Packet received: 
  [remote] start_remote: warning: couldn't determine remote current thread; picking first in list.
  [remote] Sending packet: $qOffsets#4b
  [remote] Packet received: 
  [remote] wait: enter
    [remote] select_thread_for_ambiguous_stop_reply: enter
      [remote] select_thread_for_ambiguous_stop_reply: process_wide_stop = 0
      [remote] select_thread_for_ambiguous_stop_reply: first resumed thread is Thread 1.1
      [remote] select_thread_for_ambiguous_stop_reply: is this guess ambiguous? = 1
warning: multi-threaded target stopped without sending a thread-id, using first non-exited thread

[daniel5151]

Gotcha.

Without digging into the GDB source code, I have a feeling that this error message is actually a result of gdbstub hardcoding the response to the ? packet as S05, rather than qC not being implemented. If so, then the solution here might be to update the gdbstub API to support passing an initial stop reason into the event loop...

EDIT: You can run a quick-and-dirty test to see if that's the case by modifying the gdbstub implementation to return a hardcoded string that includes a thread-id, and see if that makes the message go away.

[thefaxman]

EDIT: You can run a quick-and-dirty test to see if that's the case by modifying the gdbstub implementation to return a hardcoded string that includes a thread-id, and see if that makes the message go away.

Yeah, I think that's it. Here's a relevant comment from the GDB code right before the warning is written out:

	  /* If you are seeing this warning then the remote target has
	     stopped without specifying a thread-id, but the target
	     does have multiple threads (or inferiors), and so GDB is
	     having to guess which thread stopped.

	     Examples of what might cause this are the target sending
	     and 'S' stop packet, or a 'T' stop packet and not
	     including a thread-id.

	     Additionally, the target might send a 'W' or 'X packet
	     without including a process-id, when the target has
	     multiple running inferiors.  */

It sounds like S05 should be replaced with a T packet if the stub is thread aware.

[daniel5151]

Yup, that's probably it.

Updating the gdbstub API to accept an initial stop reason still seems like the most robust solution here, but I suppose a reasonable short-term fix would be to go in and change the ? handler to return the value of get_sane_any_tid.

[thefaxman]

Confirmed - a response of T05thread:p1.1; makes the client happy

[daniel5151]

Awesome.

In that case, would you be interested in landing these features in two separate PRs? One for updating the ? response, and another implementing qThreadExtraInfo?

[daniel5151]

I guess there's technically no harm in also implementing support for qC as well, though given that gdbstub always includes the thread-id as part of its T stop-replies, it's not like GDB will ever need a non-default response to qC during steady-state operation...

[thefaxman]

The client no longer sends qC once I reply with a T packet w/ thread ID, so no reason wasting time on that. I'll get a first PR for the ? packet shortly after a few more test cases, but it's looking good on both the single and multi threaded armv4t examples.