From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from barracuda.ebox.ca (barracuda.ebox.ca [96.127.255.19]) by sourceware.org (Postfix) with ESMTPS id 57B0E3952C01 for ; Thu, 1 Oct 2020 02:57:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 57B0E3952C01 X-ASG-Debug-ID: 1601521017-0c856e1c44c7220001-fS2M51 Received: from smtp.ebox.ca (smtp.ebox.ca [96.127.255.82]) by barracuda.ebox.ca with ESMTP id YDUOEBYdFmsyQK28 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 30 Sep 2020 22:56:58 -0400 (EDT) X-Barracuda-Envelope-From: simon.marchi@polymtl.ca X-Barracuda-RBL-Trusted-Forwarder: 96.127.255.82 Received: from simark.localdomain (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) by smtp.ebox.ca (Postfix) with ESMTP id CE2B9441B21; Wed, 30 Sep 2020 22:56:57 -0400 (EDT) From: Simon Marchi X-Barracuda-RBL-IP: 173.246.6.90 X-Barracuda-Effective-Source-IP: 173-246-6-90.qc.cable.ebox.net[173.246.6.90] X-Barracuda-Apparent-Source-IP: 173.246.6.90 To: gdb-patches@sourceware.org Subject: [PATCH] gdb: don't pass TARGET_WNOHANG to targets that can't async (PR 26642) Date: Wed, 30 Sep 2020 22:56:56 -0400 X-ASG-Orig-Subj: [PATCH] gdb: don't pass TARGET_WNOHANG to targets that can't async (PR 26642) Message-Id: <20201001025656.2561757-1-simon.marchi@polymtl.ca> X-Mailer: git-send-email 2.28.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Barracuda-Connect: smtp.ebox.ca[96.127.255.82] X-Barracuda-Start-Time: 1601521018 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://96.127.255.19:443/cgi-mod/mark.cgi X-Virus-Scanned: by bsmtpd at ebox.ca X-Barracuda-Scan-Msg-Size: 9355 X-Barracuda-BRTS-Status: 1 X-Barracuda-Spam-Score: 0.00 X-Barracuda-Spam-Status: No, SCORE=0.00 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=8.0 tests= X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.84992 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_QUARANTINE, KAM_DMARC_STATUS, KAM_SHORT, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_SOFTFAIL, 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: Thu, 01 Oct 2020 02:57:10 -0000 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 immediatly wake up the event loop 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 immediatly wake up the event loop 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. But I don't see when it would be useful to ask a sync target to do a non-blocking wait. Pass TARGET_WNOHANG to sync targets also poses the risk of passing TARGET_WNOHANG to a target that doesn't implement it. The caller of target_wait would expect the call not to block, but the call may indeed block. Since supporting TARGET_WNOHANG is a requirement for targets that can do async (I believe), I propose (as implemented in this patch) that we add an assertion in target_wait to make sure we don't ask a target that can't do async to handle TARGET_WNOHANG. A question in my mind is: could we bind the blocking or non-blocking behavior of wait with can_async_p? In other words: - Do we ever need to do a blocking wait on a target that supports async? - Do we ever need to do a non-blocking wait on a target that does not support async? So, could we make it so that if target's can_async_p method returns true, its wait method is necessarily non-blocking? And if can_async_p returns false, its wait method is necessarily blocking? It sounds to me like it would simplify the semantic of target_ops::wait a little bit. 2. The linux-nat target, even in the mode where it emulates a synchronous target (with "maintenance set target-async off") respects TARGET_WNOHANG. Non-async targets, such as windows_nat_target, simply don't check / support TARGET_WNOHANG. Their wait method is always blocking. So, to properly emulate a non-async target, I believe that linux-nat should also ignore it when in "maintenance set target-async off" mode. Its behavior would be closer to a "true" non-async target. The problem disappears if we simply fix either of these issues, but I think it wouldn't hurt to fix both. 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. * linux-nat.c (linux_nat_wait_1): Ignore TARGET_WNOHANG if target_async_permitted is false. * 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/linux-nat.c | 6 +++- gdb/target.c | 5 +++ .../gdb.base/maint-target-async-off.c | 22 +++++++++++++ .../gdb.base/maint-target-async-off.exp | 32 +++++++++++++++++++ 5 files changed, 69 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 daf10417842f..1b88514c9cc7 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/linux-nat.c b/gdb/linux-nat.c index fbb538859429..1064fc4f8c72 100644 --- a/gdb/linux-nat.c +++ b/gdb/linux-nat.c @@ -3238,7 +3238,11 @@ linux_nat_wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus, /* No interesting event to report to the core. */ - if (target_options & TARGET_WNOHANG) + /* If target_async_permitted is false (maintenance set target-async off + is used), pretend that we don't know about TARGET_WNOHANG and go block + in wait_for_signal. */ + if (target_options & TARGET_WNOHANG + && target_async_permitted) { linux_nat_debug_printf ("exit (ignore)"); diff --git a/gdb/target.c b/gdb/target.c index dd78a848caec..6f340678b7ca 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -1997,6 +1997,11 @@ ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status, target_wait_flags options) { + target_ops *target = current_top_target (); + + if (!target->can_async_p ()) + gdb_assert ((options & TARGET_WNOHANG) == 0); + return current_top_target ()->wait (ptid, status, options); } 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..e77bc79a21e1 --- /dev/null +++ b/gdb/testsuite/gdb.base/maint-target-async-off.exp @@ -0,0 +1,32 @@ +# 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 + +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } { + return +} + +gdb_test_no_output "maintenance set target-async off" + +if { ![runto_main] } { + fail "can't run to main" + return +} + +gdb_continue_to_end -- 2.28.0