Re: [PATCH v3 5/5] gdb: better handling of 'S' packets

Simon Marchi via Gdb-patches
2021-01-11T20:37:05Z

On 2021-01-09 4:26 p.m., Pedro Alves wrote:
> On 08/01/21 04:17, Simon Marchi wrote:
>
>> @@ -7796,75 +7799,117 @@ remote_notif_get_pending_events (remote_target *remote, notif_client *nc)
>> remote->remote_notif_get_pending_events (nc);
>> }
>>
>> -/* Called when it is decided that STOP_REPLY holds the info of the
>> - event that is to be returned to the core. This function always
>> - destroys STOP_REPLY. */
>> +/* Called from process_stop_reply when the stop packet we are responding
>> + to didn't include a process-id or thread-id. STATUS is the stop event
>> + we are responding to.
>> +
>> + It is the task of this function to select a suitable thread (or process)
>> + and return its ptid, this is the thread (or process) we will assume the
>> + stop event came from.
>> +
>> + In some cases there isn't really any choice about which thread (or
>> + process) is selected, a basic remote with a single process containing a
>> + single thread might choose not to send any process-id or thread-id in
>> + its stop packets, this function will select and return the one and only
>> + thread.
>> +
>> + However, if a target supports multiple threads (or processes) and still
>> + doesn't include a thread-id (or process-id) in its stop packet then
>> + first, this is a badly behaving target, and second, we're going to have
>> + to select a thread (or process) at random and use that. This function
>> + will print a warning to the user if it detects that there is the
>> + possibility that GDB is guessing which thread (or process) to
>> + report. */
>>
>> 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)
>
> Note that this is called before gdb fetches the updated thread list,
> so the stop reply may be ambiguous without gdb realizing, if
> the inferior spawned new threads, but the stop is for the thread
> that was resumed. Maybe the comment should mention that.
>
> For this reason, I see this patch more as being lenient to the stub,
> than fixing a GDB bug with misimplementing the remote protocol.

I don't really understand this.

>
>> {
>> - 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);
>
> I didn't mention this before, but I keep having the same thought, so I'd
> better speak up. :-) I find "stop is for all threads" ambiguous with
> all-stop vs non-stop. I'd suggest something like "process_wide_stop",
> I think it would work.

Agreed, will fix.

>
>>
>> - *status = stop_reply->ws;
>> - ptid = stop_reply->ptid;
>> + thread_info *first_resumed_thread = nullptr;
>> + bool multiple_resumed_thread = false;
>>
>> - /* If no thread/process was reported by the stub then use the first
>> - non-exited thread in the current target. */
>> - if (ptid == null_ptid)
>> + /* Consider all non-exited threads of the target, find the first resumed
>> + one. */
>> + for (thread_info *thr : all_non_exited_threads (this))
>> {
>> - /* 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);
>> + remote_thread_info *remote_thr =get_remote_thread_info (thr);
>> +
>> + if (remote_thr->resume_state () != resume_state::RESUMED)
>> + continue;
>> +
>> + if (first_resumed_thread == nullptr)
>> + first_resumed_thread = thr;
>
>
>> + else if (!is_stop_for_all_threads
>> + || first_resumed_thread->ptid.pid () != thr->ptid.pid ())
>> + multiple_resumed_thread = true;
>
> The connection between the condition and whether there are multiple
> resumed threads seems mysterious and distracting to me. For a variable
> called multiple_resumed_thread(s), I would have expected instead:
>
> if (first_resumed_thread == nullptr)
> first_resumed_thread = thr;
> else
> multiple_resumed_threads = true;
>
> maybe something like "bool ambiguous;" would be more to the point?

Makes sense.

>
>> + }
>>
>> - for (thread_info *thr : all_non_exited_threads (this))
>> + gdb_assert (first_resumed_thread != nullptr);
>> +
>> + /* Warn if the remote target is sending ambiguous stop replies. */
>> + if (multiple_resumed_thread)
>> + {
>> + static bool warned = false;
>> +
>
>
>> + # Single step thread 2. Only the one thread will step. When the
>> + # thread stops, if the stop packet doesn't include a thread-id
>> + # then GDB should still understand which thread stopped.
>> + gdb_test_multiple "stepi" "" {
>> + -re "Thread 1 received signal SIGTRAP" {
>> + fail $gdb_test_name
>> + }
>
> This is still missing consuming the prompt. I'll leave deciding whether
> this -re need to be here to Andrew, but it is kept, but should consume
> the problem, since otherwise we will leave the prompt in the expect
> buffer and confuse the next gdb_test. Just adding -wrap would do, I think.

> Otherwise this LGTM.

Thanks, I'll address the comments and push patches 1, 2 and 5.

Simon