From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id E259D383F417 for ; Tue, 22 Jun 2021 21:55:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E259D383F417 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 15MLtIIl024251 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 22 Jun 2021 17:55:23 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 15MLtIIl024251 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 6C3CB1E54D; Tue, 22 Jun 2021 17:55:18 -0400 (EDT) Subject: Re: [PING] [PATCH v2] gdb: Fix deleted thread when issuing next command To: "Paunovic, Aleksandar" , "gdb-patches@sourceware.org" References: From: Simon Marchi Message-ID: <2c3ec10f-b49f-fd04-0948-000f0ea32d5e@polymtl.ca> Date: Tue, 22 Jun 2021 17:55:18 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Tue, 22 Jun 2021 21:55:18 +0000 X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org 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: , X-List-Received-Date: Tue, 22 Jun 2021 21:55:29 -0000 On 2021-06-22 6:23 a.m., Paunovic, Aleksandar via Gdb-patches wrote: > When issuing "next" command the thread got deleted even though it was still alive and running. This happened because the thread was examined under a wrong inferior. > > The fixed scenario: > ~~~ > $ gdb -q breakpoint-running-inferior-1-exe > (gdb) set schedule-multiple off > (gdb) break breakpoint-running-inferior-1.c:26 > (gdb) run > (gdb) add-inferior -no-connection > (gdb) inferior 2 > (gdb) spawn gdbserver :2346 breakpoint-running-inferior-2-exe > (gdb) target remote :2346 > (gdb) break breakpoint-running-inferior-2.c:26 > (gdb) continue > (gdb) thread 1.1 > (gdb) clear breakpoint-running-inferior-2.c:26 > (gdb) set schedule-multiple on > (gdb) next > (gdb) thread 1.1 > ~~~ > > Before: > ~~~ > Thread ID 1.1 has terminated. > ~~~ > > Now: > ~~~ > Switching to thread 1.1 > ~~~ > > gdb/ChangeLog: > 2021-04-30 Aleksandar Paunovic > > * infrun.c (keep_going_stepped_thread): Switch to correct > inferior and check if thread is executing. > > gdb/testsuite/ChangeLog: > 2021-04-30 Aleksandar Paunovic > > * gdb.base/breakpoint-running-inferior-1.c: New file. > * gdb.base/breakpoint-running-inferior-2.c: New file. > * gdb.base/breakpoint-running-inferior.exp: New file. > > 2021-06-14 Aleksandar Paunovic > --- > gdb/infrun.c | 2 + > .../gdb.base/breakpoint-running-inferior-1.c | 39 ++++++++ .../gdb.base/breakpoint-running-inferior-2.c | 39 ++++++++ .../gdb.base/breakpoint-running-inferior.exp | 89 +++++++++++++++++++ > 4 files changed, 169 insertions(+) > create mode 100644 gdb/testsuite/gdb.base/breakpoint-running-inferior-1.c > create mode 100644 gdb/testsuite/gdb.base/breakpoint-running-inferior-2.c > create mode 100644 gdb/testsuite/gdb.base/breakpoint-running-inferior.exp > > diff --git a/gdb/infrun.c b/gdb/infrun.c index 4bd21fde59..abe4db31c5 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -7561,6 +7561,8 @@ keep_going_stepped_thread (struct thread_info *tp) > stepping thread is still alive. For that reason, we need to > synchronously query the target now. */ > > + /* Make sure that we are within a correct inferior. */ > + switch_to_inferior_no_thread (tp->inf); > if (tp->state == THREAD_EXITED || !target_thread_alive (tp->ptid)) So, if I understand correctly, the important point is that target_thread_alive is called on the wrong target stack, so it erroneously returns that the thread is not alive? If so, the commit message should mention that, as a step that leads to the problem. Otherwise, the fix makes sense to me. keep_going_stepped_thread is called with loops that iterate over all threads, so it will necessarily be called with threads of different inferiors / target, it makes sense that we have to switch to the correct inferior. > { > infrun_debug_printf ("not resuming previously stepped thread, it has " > diff --git a/gdb/testsuite/gdb.base/breakpoint-running-inferior-1.c b/gdb/testsuite/gdb.base/breakpoint-running-inferior-1.c > new file mode 100644 > index 0000000000..475b2c4126 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/breakpoint-running-inferior-1.c > @@ -0,0 +1,39 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2021 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see > + . */ > + > +#include > +#include > + > +void * > +forever () > +{ > + /* Wait for alarm. */ > + while (1) > + sleep (1); /* break here */ > +} > + > +int > +main () > +{ > + alarm (30); > + > + pthread_t forever_thread; > + pthread_create (&forever_thread, NULL, *forever, NULL); pthread_join > + (forever_thread, NULL); > + > + return 0; > +} > diff --git a/gdb/testsuite/gdb.base/breakpoint-running-inferior-2.c b/gdb/testsuite/gdb.base/breakpoint-running-inferior-2.c > new file mode 100644 > index 0000000000..475b2c4126 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/breakpoint-running-inferior-2.c > @@ -0,0 +1,39 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2021 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see > + . */ > + > +#include > +#include > + > +void * > +forever () > +{ > + /* Wait for alarm. */ > + while (1) > + sleep (1); /* break here */ > +} > + > +int > +main () > +{ > + alarm (30); > + > + pthread_t forever_thread; > + pthread_create (&forever_thread, NULL, *forever, NULL); pthread_join > + (forever_thread, NULL); > + > + return 0; > +} > diff --git a/gdb/testsuite/gdb.base/breakpoint-running-inferior.exp b/gdb/testsuite/gdb.base/breakpoint-running-inferior.exp > new file mode 100644 > index 0000000000..932a885e49 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/breakpoint-running-inferior.exp > @@ -0,0 +1,89 @@ > +# Copyright 2021 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or # > +(at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, # but > +WITHOUT ANY WARRANTY; without even the implied warranty of # > +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the # GNU > +General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License # > +along with this program. If not, see . > + > +# Create two inferiors with different targets. The first runs on top > +of # a native target while the second on a remote target. Both > +inferiors # use the copy of the same source code. The copy is done in > +order to make # sure that a breakpoint is only in inferior 2. While in > +inferior 1, do # a "next" which should break in a thread in inferior 2. > +# Both executables will run total of 4 threads (2 per executable) and # > +we will put a breakpoint only in the second executable to achieve this. The test would probably belong to gdb.multi, this is where this kind of interaction between multiple targets are tested. I have the feeling that "breakpoint-running-inferior" is not a very accurate name for this test. The test steps a thread in an inferior while a thread in another inferior is expected to hit a breakpoint, so I guess it should be something along those lines. When running the test, I get: DUPLICATE: gdb.base/breakpoint-running-inferior.exp: successfully compiled posix threads test case Please get rid of that. If it's not possible to pass an explicit test name to gdb_compile_pthreads, you can probably use with_test_prefix. When running your test with the native-extended-gdbserver board: make check TESTS="gdb.base/breakpoint-running-inferior.exp" RUNTESTFLAGS="--target_board=native-extended-gdbserver" It passes, but also says: WARNING: Timed out waiting for EOF in server after monitor exit This is not ideal, as the test hangs for $timeout at the end, which makes the testsuite run for longer unnecessarily. I checked other multi-target tests, and they seem to be skipped with that board: make check TESTS="gdb.multi/multi-target-no-resumed.exp" RUNTESTFLAGS="--target_board=native-extended-gdbserver" This is the code in gdb.multi/multi-target.exp.tcl that likely causes it to be skipped: # The plain remote target can't do multiple inferiors. if {[target_info gdb_protocol] != ""} { return 0 } The comment looks wrong to me, because the code causes the test to be skipped even with gdb_protocol is extended-remote. But the resulting behavior makes sense to me: in this test we explicitly set up our targets, native and remote, so it's a bit pointless to run with a board that defaults to a remote target. Some other tests use the approach of doing a "disconnect" in case we are in this situation, for example: https://gitlab.com/gnutools/gdb/-/blob/224506e95d2d44aa6583cbcda9f4b7305f834ab3/gdb/testsuite/gdb.server/server-exec-info.exp#L30-32 But I find it a bit pointless, because we end up testing exactly the same thing as the default unix board then. So we might as well just skip it. Simon