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 AB2FA3861817 for ; Tue, 13 Oct 2020 15:31:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org AB2FA3861817 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 09DFV46Z003302 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Tue, 13 Oct 2020 11:31:09 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 09DFV46Z003302 Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (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 5421F1E58E for ; Tue, 13 Oct 2020 11:31:04 -0400 (EDT) Subject: Re: [PATCH v2] gdb: don't pass TARGET_WNOHANG to targets that can't async (PR 26642) To: gdb-patches@sourceware.org References: <20201001025656.2561757-1-simon.marchi@polymtl.ca> <20201013152705.36523-1-simon.marchi@polymtl.ca> From: Simon Marchi Message-ID: Date: Tue, 13 Oct 2020 11:31:03 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <20201013152705.36523-1-simon.marchi@polymtl.ca> Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Tue, 13 Oct 2020 15:31:04 +0000 X-Spam-Status: No, score=-9.7 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-DCC: Etherboy: antispam2020.polymtl.ca 1002; Body=1 Fuz1=1 Fuz2=1 X-Spam-Languages: en 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, 13 Oct 2020 15:31:16 -0000 Sorry, I forgot the "v2" in the title. On 2020-10-13 11:27 a.m., Simon Marchi wrote: > New in v2: > > - omit change in linux-nat.c > - set target-async off using command-line flag in the test > - use target local variable in target_wait > > --8<--- > > Debugging with "maintenance set target-async off" on Linux has been > broken since 5b6d1e4fa4f ("Multi-target support"). > > The issue is easy to reproduce: > > $ ./gdb -q --data-directory=data-directory -nx ./test > Reading symbols from ./test... > (gdb) maintenance set target-async off > (gdb) start > Temporary breakpoint 1 at 0x1151: file test.c, line 5. > Starting program: /home/simark/build/binutils-gdb/gdb/test > > ... and it hangs there. > > The difference between pre-5b6d1e4fa4f and 5b6d1e4fa4f is that > fetch_inferior_event now calls target_wait with TARGET_WNOHANG for > non-async-capable targets, whereas it didn't before. > > For non-async-capable targets, this is how it's expected to work when > resuming execution: > > 1. we call resume > 2. the infrun async handler is marked in prepare_to_wait, to immediately > wake up the event loop when we get back to it > 3. fetch_inferior_event calls the target's wait method without > TARGET_WNOHANG, effectively blocking until the target has something > to report > > However, since we call the target's wait method with TARGET_WNOHANG, > this happens: > > 1. we call resume > 2. the infrun async handler is marked in prepare_to_wait, to immediately > wake up the event loop when we get back to it > 3. fetch_inferior_event calls the target's wait method with > TARGET_WNOHANG, the target has nothing to report yet > 4. we go back to blocking on the event loop > 5. SIGCHLD finally arrives, but the event loop is not woken up, because > we are not in async mode. Normally, we should have been stuck in > waitpid the SIGCHLD would have unblocked us. > > We end up in this situation because these two necessary conditions are > met: > > 1. GDB uses the TARGET_WNOHANG option with a target that can't do async. > I don't think this makes sense. I mean, it's technically possible, > the doc for TARGET_WNOHANG is: > > /* Return immediately if there's no event already queued. If this > options is not requested, target_wait blocks waiting for an > event. */ > TARGET_WNOHANG = 1, > > ... which isn't in itself necessarily incompatible with synchronous > targets. It could be possible for a target to support non-blocking > polls, while not having a way to asynchronously wake up the event > loop, which is also necessary to support async. But as of today, > we don't expect GDB and sync targets to work this way. > > 2. The linux-nat target, even in the mode where it emulates a > synchronous target (with "maintenance set target-async off") respects > TARGET_WNOHANG. Other non-async targets, such as windows_nat_target, > simply don't check / support TARGET_WNOHANG, so their wait method is > always blocking. > > Fix the first issue by avoiding using TARGET_WNOHANG on non-async > targets, in do_target_wait_1. Add an assert in target_wait to verify it > doesn't happen. > > The new test gdb.base/maint-target-async-off.exp is a simple test that > just tries running to main and then to the end of the program, with > "maintenance set target-async off". > > gdb/ChangeLog: > > PR gdb/26642 > * infrun.c (do_target_wait_1): Clear TARGET_WNOHANG if the > target can't do async. > * target.c (target_wait): Assert that we don't pass > TARGET_WNOHANG to a target that can't async. > > gdb/testsuite/ChangeLog: > > PR gdb/26642 > * gdb.base/maint-target-async-off.c: New test. > * gdb.base/maint-target-async-off.exp: New test. > > Change-Id: I69ad3a14598863d21338a8c4e78700a58ce7ad86 > --- > gdb/infrun.c | 5 +++ > gdb/target.c | 7 +++- > .../gdb.base/maint-target-async-off.c | 22 ++++++++++ > .../gdb.base/maint-target-async-off.exp | 41 +++++++++++++++++++ > 4 files changed, 74 insertions(+), 1 deletion(-) > create mode 100644 gdb/testsuite/gdb.base/maint-target-async-off.c > create mode 100644 gdb/testsuite/gdb.base/maint-target-async-off.exp > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index d552fb3adb26..8ae39a2877b3 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -3533,6 +3533,11 @@ do_target_wait_1 (inferior *inf, ptid_t ptid, > > /* But if we don't find one, we'll have to wait. */ > > + /* We can't ask a non-async target to do a non-blocking wait, so this will be > + a blocking wait. */ > + if (!target_can_async_p ()) > + options &= ~TARGET_WNOHANG; > + > if (deprecated_target_wait_hook) > event_ptid = deprecated_target_wait_hook (ptid, status, options); > else > diff --git a/gdb/target.c b/gdb/target.c > index 531858a3333f..a111ea3c3336 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -1992,7 +1992,12 @@ ptid_t > target_wait (ptid_t ptid, struct target_waitstatus *status, > target_wait_flags options) > { > - return current_top_target ()->wait (ptid, status, options); > + target_ops *target = current_top_target (); > + > + if (!target->can_async_p ()) > + gdb_assert ((options & TARGET_WNOHANG) == 0); > + > + return target->wait (ptid, status, options); > } > > /* See target.h. */ > diff --git a/gdb/testsuite/gdb.base/maint-target-async-off.c b/gdb/testsuite/gdb.base/maint-target-async-off.c > new file mode 100644 > index 000000000000..9d7b2f1a4c28 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/maint-target-async-off.c > @@ -0,0 +1,22 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2020 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 . */ > + > +int > +main (void) > +{ > + return 0; > +} > diff --git a/gdb/testsuite/gdb.base/maint-target-async-off.exp b/gdb/testsuite/gdb.base/maint-target-async-off.exp > new file mode 100644 > index 000000000000..dc205f533c53 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/maint-target-async-off.exp > @@ -0,0 +1,41 @@ > +# Copyright 2020 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 . > + > +# Verify that debugging with "maintenance target-async off" works somewhat. At > +# least running to main and to the end of the program. > + > +standard_testfile > + > +save_vars { GDBFLAGS } { > + # Enable target-async off this way, because with board > + # native-extended-gdbserver, the remote target is already open when > + # returning from prepare_for_testing, and that would be too late to toggle > + # it. > + append GDBFLAGS { -ex "maintenance set target-async off" } > + > + if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } { > + return > + } > + > + # Make sure our command-line flag worked. > + gdb_test "maintenance show target-async" "Controlling the inferior in asynchronous mode is off\\." > + > + if { ![runto_main] } { > + fail "can't run to main" > + return > + } > + > + gdb_continue_to_end > +} > -- > 2.28.0 >