From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id ywTBI5Yf+l+JXgAAWB0awg (envelope-from ) for ; Sat, 09 Jan 2021 16:26:46 -0500 Received: by simark.ca (Postfix, from userid 112) id 7B2801EE11; Sat, 9 Jan 2021 16:26:46 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 3FECB1E940 for ; Sat, 9 Jan 2021 16:26:45 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id F14953858018; Sat, 9 Jan 2021 21:26:44 +0000 (GMT) Received: from mail-wm1-f54.google.com (mail-wm1-f54.google.com [209.85.128.54]) by sourceware.org (Postfix) with ESMTPS id 82DCC3858018 for ; Sat, 9 Jan 2021 21:26:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 82DCC3858018 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=alves.ped@gmail.com Received: by mail-wm1-f54.google.com with SMTP id e25so11441631wme.0 for ; Sat, 09 Jan 2021 13:26:41 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=fkyu0/fCYWT+R2kZjqNffokHfiinP21a6tTvbc1/kcs=; b=p+i5AsJjfsdLIhOf40niJOuj/9DdpOkZPO5iENBhTkibaRqqCZsfM/FPdQIp0i9irl cXTOaCCfe0XPTgV8UGukKYK+2Tj6YAaDCmjEZYvFGznY3euXDZShK2Omm4Vxw1aQ5GBG gJnhm5T9hJ2BdL1h1ekVIDd8D+LaQR1zXTxjsaSYMc5NGPy05CzSKg41vzvo7MrI/yIp wUD11udP40djF4N28jYU2gVlKiRuGD5aV/8PTPbRw9tlvPnSTDnsMdWoD72kG1JKfJi/ B/zauKacID1oSuhzWTJkCZHpXuhdXoEtr8bTvQFrqUwKfB9GHG5SInOz4zdAG8Sxr+x7 mGBg== X-Gm-Message-State: AOAM533rIKtSRgf0UffwepLkpVvVYIeGl2WA+ickntOtQoKZJDYF8ckh ARAGHZMrhq4io3+XSOCjVvQ= X-Google-Smtp-Source: ABdhPJwnD7E0CFOl06W1JSx1qFQ6ok5wRF8ou0zW+gnbu0L01A2ESpHTZoG8+TNA1tTu6dcSTrRxSA== X-Received: by 2002:a1c:41c5:: with SMTP id o188mr8453444wma.18.1610227600527; Sat, 09 Jan 2021 13:26:40 -0800 (PST) Received: from ?IPv6:2001:8a0:f91f:e900:1d90:d745:3c32:c159? ([2001:8a0:f91f:e900:1d90:d745:3c32:c159]) by smtp.gmail.com with ESMTPSA id s1sm17441215wrv.97.2021.01.09.13.26.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 09 Jan 2021 13:26:39 -0800 (PST) Subject: Re: [PATCH v3 5/5] gdb: better handling of 'S' packets From: Pedro Alves To: Simon Marchi , gdb-patches@sourceware.org References: <20210108041734.3873826-1-simon.marchi@polymtl.ca> <20210108041734.3873826-6-simon.marchi@polymtl.ca> Message-ID: Date: Sat, 9 Jan 2021 21:26:38 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <20210108041734.3873826-6-simon.marchi@polymtl.ca> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" 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. > { > - 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. > > - *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? > + } > > - 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. > + -re -wrap "$hex.*$decimal.*while \\(worker_blocked\\).*" { > + pass $gdb_test_name > + } > + } > +