From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.freebsd.org (mx2.freebsd.org [96.47.72.81]) by sourceware.org (Postfix) with ESMTPS id 52B7B3858D35 for ; Sat, 1 Aug 2020 19:32:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 52B7B3858D35 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=FreeBSD.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=jhb@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits)) (Client CN "mx1.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx2.freebsd.org (Postfix) with ESMTPS id DAEBC812B6; Sat, 1 Aug 2020 19:32:27 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4BJvRv4ptGz4cnx; Sat, 1 Aug 2020 19:32:27 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from John-Baldwins-MacBook-Pro-274.local (unknown [IPv6:2601:648:8203:2990:1c16:59cd:a25c:a7a3]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 0C1EE17009; Sat, 1 Aug 2020 19:32:26 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Subject: Re: [PATCH 02/23] Don't rely on inferior_ptid in record_full_wait To: Simon Marchi , Tom Tromey , Pedro Alves Cc: gdb-patches@sourceware.org References: <20190906232807.6191-1-palves@redhat.com> <20190906232807.6191-3-palves@redhat.com> <87h7tope4d.fsf@tromey.com> <9d4243a6-3b43-802b-b1d6-4cd0497205af@simark.ca> From: John Baldwin Autocrypt: addr=jhb@FreeBSD.org; keydata= mQGiBETQ+XcRBADMFybiq69u+fJRy/0wzqTNS8jFfWaBTs5/OfcV7wWezVmf9sgwn8TW0Dk0 c9MBl0pz+H01dA2ZSGZ5fXlmFIsee1WEzqeJzpiwd/pejPgSzXB9ijbLHZ2/E0jhGBcVy5Yo /Tw5+U/+laeYKu2xb0XPvM0zMNls1ah5OnP9a6Ql6wCgupaoMySb7DXm2LHD1Z9jTsHcAQMD /1jzh2BoHriy/Q2s4KzzjVp/mQO5DSm2z14BvbQRcXU48oAosHA1u3Wrov6LfPY+0U1tG47X 1BGfnQH+rNAaH0livoSBQ0IPI/8WfIW7ub4qV6HYwWKVqkDkqwcpmGNDbz3gfaDht6nsie5Z pcuCcul4M9CW7Md6zzyvktjnbz61BADGDCopfZC4of0Z3Ka0u8Wik6UJOuqShBt1WcFS8ya1 oB4rc4tXfSHyMF63aPUBMxHR5DXeH+EO2edoSwViDMqWk1jTnYza51rbGY+pebLQOVOxAY7k do5Ordl3wklBPMVEPWoZ61SdbcjhHVwaC5zfiskcxj5wwXd2E9qYlBqRg7QeSm9obiBCYWxk d2luIDxqaGJARnJlZUJTRC5vcmc+iGAEExECACAFAkTQ+awCGwMGCwkIBwMCBBUCCAMEFgID AQIeAQIXgAAKCRBy3lIGd+N/BI6RAJ9S97fvbME+3hxzE3JUyUZ6vTewDACdE1stFuSfqMvM jomvZdYxIYyTUpC5Ag0ERND5ghAIAPwsO0B7BL+bz8sLlLoQktGxXwXQfS5cInvL17Dsgnr3 1AKa94j9EnXQyPEj7u0d+LmEe6CGEGDh1OcGFTMVrof2ZzkSy4+FkZwMKJpTiqeaShMh+Goj XlwIMDxyADYvBIg3eN5YdFKaPQpfgSqhT+7El7w+wSZZD8pPQuLAnie5iz9C8iKy4/cMSOrH YUK/tO+Nhw8Jjlw94Ik0T80iEhI2t+XBVjwdfjbq3HrJ0ehqdBwukyeJRYKmbn298KOFQVHO EVbHA4rF/37jzaMadK43FgJ0SAhPPF5l4l89z5oPu0b/+5e2inA3b8J3iGZxywjM+Csq1tqz hltEc7Q+E08AAwUIAL+15XH8bPbjNJdVyg2CMl10JNW2wWg2Q6qdljeaRqeR6zFus7EZTwtX sNzs5bP8y51PSUDJbeiy2RNCNKWFMndM22TZnk3GNG45nQd4OwYK0RZVrikalmJY5Q6m7Z16 4yrZgIXFdKj2t8F+x613/SJW1lIr9/bDp4U9tw0V1g3l2dFtD3p3ZrQ3hpoDtoK70ioIAjjH aIXIAcm3FGZFXy503DOA0KaTWwvOVdYCFLm3zWuSOmrX/GsEc7ovasOWwjPn878qVjbUKWwx Q4QkF4OhUV9zPtf9tDSAZ3x7QSwoKbCoRCZ/xbyTUPyQ1VvNy/mYrBcYlzHodsaqUDjHuW+I SQQYEQIACQUCRND5ggIbDAAKCRBy3lIGd+N/BCO8AJ9j1dWVQWxw/YdTbEyrRKOY8YZNwwCf afMAg8QvmOWnHx3wl8WslCaXaE8= Message-ID: <93dfb190-2d7d-8337-b2b0-df160d1a0bf6@FreeBSD.org> Date: Sat, 1 Aug 2020 12:32:25 -0700 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <9d4243a6-3b43-802b-b1d6-4cd0497205af@simark.ca> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_NONE, 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: Sat, 01 Aug 2020 19:32:29 -0000 On 8/1/20 9:14 AM, Simon Marchi wrote: > On 2020-07-30 11:17 p.m., Tom Tromey wrote: >>>>>>> "Pedro" == Pedro Alves writes: >> >> Pedro> The multi-target patch sets inferior_ptid to null_ptid before handling >> Pedro> a target event, and thus before calling target_wait, in order to catch >> Pedro> places in target_ops::wait implementations that are incorrectly >> Pedro> relying on inferior_ptid (which could otherwise be a ptid of a >> Pedro> different target, for example). That caught this instance in >> Pedro> record-full.c. >> >> I found a few target_ops::wait implementations doing "return >> inferior_ptid" in error cases. Based on this comment, and the comment >> for target_wait, I suspect these should actually return minus_one_ptid >> instead. >> >> I've appended the patch so you can see what it looks like. I haven't >> tried it at all. Does this seem correct? > > I think you are right that it's incorrect, but... > >> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c >> index ae0b0f7ff0d..2cae87023f9 100644 >> --- a/gdb/inf-ptrace.c >> +++ b/gdb/inf-ptrace.c >> @@ -343,7 +343,7 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus, >> /* Claim it exited with unknown signal. */ >> ourstatus->kind = TARGET_WAITKIND_SIGNALLED; >> ourstatus->value.sig = GDB_SIGNAL_UNKNOWN; >> - return inferior_ptid; >> + return minus_one_ptid; >> } > > I don't know if this makes sense: you tell the core of GDB that some > process was signalled (terminated by a signal), but you don't tell > what process it is. I am not sure what the core of GDB can do with > this information. > > The contract that the `wait` implementations must respect is not > very well documented I believe, so it's not clear which value is > valid with which event. But I expect that tying a > TARGET_WAITKIND_SIGNALLED event with a minus_one_ptid (or null_ptid > for that matter) is invalid, and will just break down later in > handle_inferior_event, where we handle this kind of event: > > case TARGET_WAITKIND_EXITED: > case TARGET_WAITKIND_SIGNALLED: > { > /* Depending on the system, ecs->ptid may point to a thread or > to a process. On some targets, target_mourn_inferior may > need to have access to the just-exited thread. That is the > case of GNU/Linux's "checkpoint" support, for example. > Call the switch_to_xxx routine as appropriate. */ > thread_info *thr = find_thread_ptid (ecs->target, ecs->ptid); > if (thr != nullptr) > switch_to_thread (thr); > else > { > inferior *inf = find_inferior_ptid (ecs->target, ecs->ptid); > switch_to_inferior_no_thread (inf); > } > > find_thread_ptid will return nullptr, so we'll go in the else. > find_inferior_ptid will return nullptr, which we'll pass to > switch_to_inferior_no_thread, and it will assert somewhere in there. Note that prior to my fix to inf-ptrace.c in f37e5866aa, I was tripping over this case and it did indeed result in an assertion failure as wait was returning null_ptid. I kind of think an error like an assertion failure is the right behavior as it's really an indication of a bug. Arguably though inf-ptrace.c should not return null_ptid that later triggers an obscure assertion failure, but instead it should probably just die with an explicit error() or the like. It's not really clear to me that we should be trying to do anything to gracefully handle an ENOCHILD error. -- John Baldwin