From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 5/muL5j8dWrXPBEAWB0awg (envelope-from ) for ; Fri, 07 Aug 2026 11:41:12 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=O0yCnoXm; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id A31E81E166; Fri, 07 Aug 2026 11:41:12 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-6.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham autolearn_force=no version=4.0.1 Received: from vm01.sourceware.org (vm01.sourceware.org [IPv6:2620:52:6:3111::32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id D77101E09B for ; Fri, 07 Aug 2026 11:41:11 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id CE2724B9DB5C for ; Fri, 7 Aug 2026 15:41:10 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org CE2724B9DB5C Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=O0yCnoXm Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id 62A9B4BB58BE for ; Fri, 7 Aug 2026 15:40:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 62A9B4BB58BE Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 62A9B4BB58BE Authentication-Results: sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1786117244; cv=none; b=FiZD6Ns8iVF/ti9W+bpq5xqYOsiSoVOCAHKWZdYQfH+B9YkHfg/xKV6DudfZEsThUlfNfAYegCa2PaUj1A+tgxHzhK04//OwcAG6TpfTtAWB3MuLGbdzS8rrYfPChEHvt3jFHabD/qpyM61LCh8x7L9V76J1O1reMsGdm9+/kOo= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1786117244; c=relaxed/simple; bh=rRi/lrAed9vpcqy66/nsNqWG6LnZzDAKXybokmydnyQ=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=c44BoMfdKrdp1H1PDgxaxmVdyvzMEybjk5YgRPYhB/mr4c+BsuYJXgx6U5drPCXQHoONNTaUWcNNyyIc+Lq51Fc3fFZRaG2X2V1QwPudsgDruxmDZpcy4CidbpCQRLqqLZfFgM0h7JdUBewdQCTXySjf1OU45vLgReum7vEKmwo= ARC-Authentication-Results: i=1; sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=O0yCnoXm DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 62A9B4BB58BE DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1786117244; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=BW7TVEBpLV+ik/MykIxYj7SduOFM6Y3B6s/awPEGh6w=; b=O0yCnoXmxF87bDDY8JfurvEu0bLuXt14wc6/+FjuAZS/IymGeO07asT0kXA+/3WyeM4sg9 3RbwMJ32ziwg1zb5XVV5SdeqP1E2FJE4/icGA5iIZb3FWBULuGYCg9iSSqqllR2644XkS4 dHjDL21mHcaf9LWQMaoNxBwqu8VqVRg= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-691-rjtGtILrMJWYhtFQ0ClIGg-1; Fri, 07 Aug 2026 11:40:37 -0400 X-MC-Unique: rjtGtILrMJWYhtFQ0ClIGg-1 X-Mimecast-MFC-AGG-ID: rjtGtILrMJWYhtFQ0ClIGg_1786117236 Received: by mail-wr1-f72.google.com with SMTP id ffacd0b85a97d-47f726186c4so2005333f8f.2 for ; Fri, 07 Aug 2026 08:40:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1786117236; x=1786722036; h=content-type:mime-version:message-id:date:references:in-reply-to :subject:to:from:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to:content-type; bh=BW7TVEBpLV+ik/MykIxYj7SduOFM6Y3B6s/awPEGh6w=; b=eMsvY0voAn+a8Dd/oQEwXWlG2lfChndNWJMhArwUw36dXbnYGgPWXz+N5U83k1z6jA MS/pgDd1CpYPpiBfu1SP0ZVaQkoMkm4+wqv/ukgpsaTr8gLE8cKaojUXW+UIQo4pq9AE k5GLOCuOv+yhF62H1Kewm5c26NuCZwm/W0AybTMLNdOBcHpTnDBXDv5GN4AgPNUrKDbE F5KfiahPVTKYO8/ZhICdWZev/KWQI/es06XFSILMubORT/A0Ua2rnxoL/08FJ8p+p0QQ VZSe4DLLsQU+g2cSqn7GyCjqwEqUImLciy8L2+aF54W2ed/dcsisxUWDosGLEXxd+ZJ7 s6+Q== X-Forwarded-Encrypted: i=1; AHgh+Rq6s6nOPETbTLjCH8f10CqEprHUKxSGle5gYxXSqYwnn5HZ7jHJCXhLxQE6W60Zy6Ip8geO4bxoK/VRVw==@sourceware.org X-Gm-Message-State: AOJu0YzMhtSzgk6t8041jdqwx8909Z+n/TlV6P36i54oD+5Fv3+d3Ppi 5VCsOzSPyYKvQJf55cmYmvABty8JeHk325Feo0U2W9X5Liytxc//0UrZmLpSydQ2UwI/CjrBp+q Byaua9p4hfdv/QmOChloEgDe5Wth1WmtNaTkX5Z33myXAfzbB+IQMDaA88L/vvpdM0+k5qbY= X-Gm-Gg: AR+sD10GqY7NQKIxCvMQ89oTRfMcS7XpGkaVDzxn0TbA6mBIxncLrbI10uj0cb83GbE bC7ji6kLnEvWTts9MgGwJ7WyMlpxAqLLsBS7Cpw1+4LpFi0THxB1D5GKa5rzaOgF/Eu7iVoekFu Dfry2xsCSpifgEuc0fwAQjlxmDHG3zpzx5b9LX7Y4X5Uqb7uVZ+ZseJbsqlkg2o04wyRSJvzBMy vbqONyd/v9vEKMVRVTcE7qVlgTD7/xqDOkr4Tfici2h0vE+WGh+khFl5u2+3YfTX4FY5b7wJUQS SdUZedGwJDMVlCGsfr2zYlsNXQ5wVEbwdJWfdFcduu6AfpE0YGScnaJQ7TgXT12gLppU6H7KPg9 3YAsPIUzpbzYeqShTBJ1mjwUx X-Received: by 2002:adf:e19d:0:b0:47f:f138:226 with SMTP id ffacd0b85a97d-47ff138035cmr32998341f8f.17.1786117236217; Fri, 07 Aug 2026 08:40:36 -0700 (PDT) X-Received: by 2002:adf:e19d:0:b0:47f:f138:226 with SMTP id ffacd0b85a97d-47ff138035cmr32998241f8f.17.1786117235669; Fri, 07 Aug 2026 08:40:35 -0700 (PDT) Received: from localhost (227.114.208.46.dyn.plus.net. [46.208.114.227]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-480021f8f33sm7495015f8f.27.2026.08.07.08.40.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 07 Aug 2026 08:40:35 -0700 (PDT) From: Andrew Burgess To: Tom de Vries , gdb-patches@sourceware.org Subject: Re: [PATCH 2/2] gdb: share some thread proceed related code between CLI and MI In-Reply-To: References: <55e17e264f17f47680100684c9aa7bd9b88e377c.1786049312.git.aburgess@redhat.com> Date: Fri, 07 Aug 2026 16:40:34 +0100 Message-ID: <87fr0qkkgd.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: JtHLbHwj1O77yUphG6xlT0Xcw8S_SK4TjEYFIzufnB4_1786117236 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org Tom de Vries writes: > On 8/6/26 10:54 PM, Andrew Burgess wrote: >> I noticed that some code related to proceeding threads could be shared >> between CLI and MI. This fixes a bug as the CLI code contains a fix >> that the MI code is missing. >> >> In continue_1 (in infcmd.c) we have a loop that iterates over all >> threads looking for threads that are THREAD_STOPPED and are in an >> inferior that has_execution. For each thread found we then call: >> >> switch_to_thread (&thread); >> clear_proceed_status (0); >> proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT); >> >> In exec_continue (in mi/mi-main.c) we also have a loop over all >> threads that calls the proceed_thread helper function which skips >> threads that are not THREAD_STOPPED, does some PID related >> filtering (more on this later) and then calls the same three >> functions: switch_to_thread, clear_proceed_status, proceed. >> >> The PID filtering mentioned above allows proceed_thread to do two >> jobs, if the PID is zero then we proceed all threads. If PID is >> non-zero then we proceed only the threads in the inferior with that >> PID. >> >> You might also have spotted that in continue_1 we checked if the >> inferior has execution or not. This check was added in commit: >> >> commit 5b6d1e4fa4fc6827c7b3f0e99ff120dfa14d65d2 >> Date: Fri Jan 10 20:06:08 2020 +0000 >> >> Multi-target support >> >> A matching check was not added into the MI at this point, nor did the >> commit message mention why such a check was not added. I'm choosing >> to believe that this was an oversight in the 5b6d1e4fa4fc6827 commit. >> And this is the bug I mentioned above. >> >> If we call `proceed` with a thread that is part of an inferior that >> does not "has_execution" then the thread will be marked running even >> though it will never actually be set running. See the early return at >> the top of `proceed_resume_thread_checked` and the call to set_state >> in `proceed`. >> >> I do worry that there might be a bigger set of bugs here if proceed >> can set a thread's state to THREAD_RUNNING, but then never actually >> sets the underlying thread running. But in this case, just having the >> MI share code with the CLI means that we pick up the fix for this case >> basically for free. >> >> I propose adding a new global helper function `proceed_one_thread`, >> this will check if the thread is THREAD_STOPPED and is in an inferior >> which has_execution. If these conditions are met then the three >> functions mentioned above will be called to proceed the thread. >> >> I will then add a second new function `proceed_all_threads`, this will >> iterate over all threads and call proceed_one_thread. >> >> We can then use proceed_all_threads from continue_1, replacing the >> existing loop. >> >> In exec_continue we can move the PID (or rather inferior) check >> earlier, outside the loop. If we want to resume all threads (the old >> PID is zero path) then we call proceed_all_threads. If we only want >> to proceed threads with one PID then we loop over threads in the >> matching inferior and call proceed_one_thread on each. >> >> As the code I am factoring out is all within non_stop only paths I >> have added `gdb_assert (non_stop);` to each of the new helper >> functions, this will prevent these functions accidentally being called >> in the all_stop code path. >> >> While moving the two `for (...)` loops I have replaced 'auto' with >> 'thread_info' for additional type clarity. >> >> With the exception of the new has_execution check in the MI path there >> should be no other user visible changes with this commit. I've added >> a new test which exposes the missing has_execution check issue. > > Hi Andrew, > > thanks for finding and fixing this. > > The functional change is minimal (add one check), and uses a pattern > already used elsewhere in the code, so LGTM. > > Approved-By: Tom de Vries > > FWIW, while reviewing the patch I came to the hypothesis that there are > really three parts: > - refactoring in infcmd.c > - minimal fix > - refactoring in exec_continue > > To verify this, I split off the first two parts, and confirmed that this > minimal fix (not showing the part removing proceed_thread): > ... > diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c > index 8b6da41ffeb..a7e3845d6e3 100644 > --- a/gdb/mi/mi-main.c > +++ b/gdb/mi/mi-main.c > @@ -279,7 +265,12 @@ exec_continue (const char *const *argv, int argc) > } > > for (auto &thread : all_threads ()) > - proceed_thread (&thread, pid); > + { > + if (pid != 0 && thread.ptid.pid () != pid) > + continue; > + proceed_one_thread (thread); > + } > + > disable_commit_resumed.reset_and_commit (); > } > else > ... > fixes the test-case failure. > > I didn't like the escaping in the test-case much, so I wrote a patch > fixing this (attached). You could merge before committing, or I can do > a follow-up commit, as you like. Thanks, I merged everything except the quotemeta related changes. Not a huge fan, and in this case the imbalanced quotes was mucking up the syntax highlighting in my editor. I know, that's a "me" problem, but I really don't want to push a patch that makes my life harder. I'll hold off pushing this until we've discussed your other review email about this change. Thanks, Andrew > > Thanks, > - Tom > From a88acf26e0346c57fa8275af4d0b26514873aa8a Mon Sep 17 00:00:00 2001 > From: Tom de Vries > Date: Fri, 7 Aug 2026 09:44:12 +0200 > Subject: [PATCH] [gdb/testsuite] Make gdb.mi/mi-corefile-and-live.exp regexps > more readable > > --- > gdb/testsuite/gdb.mi/mi-corefile-and-live.exp | 38 ++++++++++--------- > 1 file changed, 21 insertions(+), 17 deletions(-) > > diff --git a/gdb/testsuite/gdb.mi/mi-corefile-and-live.exp b/gdb/testsuite/gdb.mi/mi-corefile-and-live.exp > index a3a57d3c81a..1a34029d087 100644 > --- a/gdb/testsuite/gdb.mi/mi-corefile-and-live.exp > +++ b/gdb/testsuite/gdb.mi/mi-corefile-and-live.exp > @@ -39,8 +39,8 @@ if {$corefile == ""} { > > # Start GDB in non-stop and schedule-multiple mode. > save_vars { GDBFLAGS } { > - append GDBFLAGS " -ex \"set non-stop on\"" > - append GDBFLAGS " -ex \"set schedule-multiple on\"" > + append GDBFLAGS { -ex "set non-stop on"} > + append GDBFLAGS { -ex "set schedule-multiple on"} > mi_clean_restart $::testfile > } > > @@ -56,42 +56,46 @@ mi_create_breakpoint "-g i1 foo" \ > > # Setup inferior 2, this will load the core file. > mi_gdb_test "-add-inferior" \ > - [multi_line "=thread-group-added,id=\"i2\"" \ > - "~\"\\\[New inferior 2\\\]\\\\n\"" \ > - "\~\"Added inferior 2\[^\r\n\]*\\\\n\"" \ > - "\\^done,inferior=\"\[^\"\]+\"(?:,connection={.*})?" ] \ > + [quotemeta \ > + [multi_line \ > + {=thread-group-added,id="i2"} \ > + {~"[New inferior 2]\n"} \ > + {~"Added inferior 2@/[^\r\n]*/\n"} \ > + {^done,inferior="@/[^"]+/"@/(?:,connection={.*})?/}]] \ > "add inferior 2" > > # Set the executable for inferior 2. > mi_gdb_test "-file-exec-and-symbols --thread-group i2 $::binfile" \ > - "\\^done" \ > + [string_to_regexp "^done"] \ > "set executable of inferior 2" > > # Load the core file into inferior 2. > mi_gdb_test \ > "-target-select --thread-group i2 core $::corefile" \ > - [multi_line \ > - "=thread-group-started,id=\"i2\",.*" \ > - "=thread-created,id=\"2\",group-id=\"i2\"" \ > - ".*\\^connected,frame=.*"] \ > + [quotemeta \ > + [multi_line \ > + {=thread-group-started,id="i2",@...} \ > + {=thread-created,id="2",group-id="i2"} \ > + {@...^connected,frame=@...}]] \ > "load core file in inferior 2" > > # Check the core file thread is initially shown as stopped. > -mi_gdb_test "-thread-info 2" ".*,state=\"stopped\".*" \ > +mi_gdb_test "-thread-info 2" {.*,state="stopped".*} \ > "core file thread is initially stopped" > > # Resume "all" threads. As the core target doesn't support execution > # this should not try to set the core target threads running. > mi_gdb_test "-exec-continue --all" \ > - [multi_line \ > - "\\^running" \ > - "\\*running,thread-id=\"1\""] \ > + [string_to_regexp \ > + [multi_line \ > + "^running" \ > + {*running,thread-id="1"}]] \ > "resume all" > > # Wait for the non-core target thread to stop. > mi_expect_stop "breakpoint-hit" \ > - "foo" ".*" ".*" ".*" {"" "disp=\"keep\""} "w1,i2 stop" > + "foo" ".*" ".*" ".*" {"" {disp="keep"}} "w1,i2 stop" > > # Check that the core target thread is still showing as stopped. > -mi_gdb_test "-thread-info 2" ".*,state=\"stopped\".*" \ > +mi_gdb_test "-thread-info 2" {.*,state="stopped".*} \ > "core file thread is still stopped" > > base-commit: 4df4712cb94f40463daf6a75c71a64eaf84e7901 > -- > 2.51.0