This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 14.5/23] Avoid another inferior_ptid reference in gdb/remote.c (Re: [PATCH 14/23] Tweak handling of remote errors in response to resumption packet)
- From: "Aktemur, Tankut Baris" <tankut dot baris dot aktemur at intel dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Thu, 17 Oct 2019 10:45:37 +0000
- Subject: Re: [PATCH 14.5/23] Avoid another inferior_ptid reference in gdb/remote.c (Re: [PATCH 14/23] Tweak handling of remote errors in response to resumption packet)
- Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none
- Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=mH1+LMpOhsYYg6DAG+1PBDvdFruC+2deWosGIXubnYw=; b=NnZpiuivXoILb5SPv0r5UqWujX+9e9/7fhZPKax5ZPSPzdK5Gu6RxOtgO03t3GwD4Q659bSkNyN3MCrj+647RY5lLfbS7UWTGfAlv50rBWAJvt6lZ5Ulcu6+8xCPB3Fi+q6fehxLTz3wN86nU/7W7l+UnxqFUQMMQ6sCc9UGrVV3paxwBKepfPakkdxS3jkGH1aqeHot2z53H0CMjBgBl21MOMRJzH/21k7DMnNZLDpzW7JlbWXXYkOv8RxCFjcZAXL1qv4r9IWCO034YZ7iOsIcWTT5r3L+0aLcH8SIlPK9DtR6jCkFi0UWQ6K7m7d366gWHbfmDUYp56E/X+Jb5Q==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=PAlGvgXIxtO6zi1VlwYRZEXQ/ZkaxZcF+cYrwvSpOHNA4Zft2bt7kJcURT5E2ehh2UpFriJVtpaoptwe+rD+aASi/qOCEI/LJ/H91YZ7cq+lhz0ldlt0lCFraXP74k0gVRRzvqN4/TCmItwlzUF3hzKQK9IUPRyFP0AVn3JY6vZiQKt4KIu48g7Cak1kZWIc9alSCmAYbjvH9AWrsNiXyaILM1YEU5+VVg3lql5HJ52S2L0TNrqz8328HPRVGkmzQaHlxdulgn7a6opjUAZhM+0KsWLlBYRYSAFIcbmrX2AGSRF70o81bqfWMcuaP10RCOSrNobIIinR4qI1EVGV3Q==
- References: <20190906232807.6191-1-palves@redhat.com> <20190906232807.6191-15-palves@redhat.com> <B98F7326B8E238409968F562D326E1A90D07674D@IRSMSX103.ger.corp.intel.com> <a60eeb53-ddc2-fa21-8c73-d4fb266fc357@redhat.com>
* On Thursday, October 17, 2019 2:54 AM, Pedro Alves wrote:
>
> On 10/9/19 2:34 PM, Aktemur, Tankut Baris wrote:
> > * On September 7, 2019 1:28 AM, Pedro Alves wrote:
>
> > A similar problem could occur in case of an exit event, too -- for instance,
> > if the remote target does not support the multi-process packet or if the
> > packet is disabled.
> >
> > This can be checked by modifying the test
> >
> > testsuite/gdb.server/connect-without-multi-process.exp
> >
> > to continue the inferior to termination.
>
> Oh, excellent, this is the same issue that Philippe discovered with
> Valgrind, for which I sent a patch earlier:
>
> https://sourceware.org/ml/gdb-patches/2019-10/msg00512.html
>
> I did not realize you had already analyzed it. I was having second
> thoughts on whether we can use general_thread, and for that I like
> your patch better than mine. In haste, had completely forgot that
> patch #14 added first_remote_resumed_thread. Also, I wrote a new test
> from scratch, not realizing we already had connect-without-multi-process.exp.
> Sigh. Well, I actually originally wrote that one too, so I just
> completely forgot it. :-D Thanks for finding it.
>
> > I'm attaching these as a patch in case one wants to test. It should be applied
> > after the main multi-target patch.
>
> I tweaked it a bit further, reused the commit log from my version of
> the patch, and rebased it on top of patch #14. Here it is:
Thank you, this looks good to me.
Regards,
-Baris
>
> From 25f279aa79a9860c64d228a9eb7fc974c2367c64 Mon Sep 17 00:00:00 2001
> From: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> Date: Thu, 17 Oct 2019 01:34:19 +0100
> Subject: [PATCH] Avoid another inferior_ptid reference in gdb/remote.c
>
> The multi-target patch makes inferior_ptid point to null_ptid before
> calling into target_wait, which catches bad uses of inferior_ptid,
> since the current selected thread in gdb shouldn't have much relation
> to the thread that reports an event.
>
> One such bad use is found in remote_target::remote_parse_stop_reply,
> where we handle the 'W' or 'X' packets (process exit), and the remote
> target does not support the multi-process extensions, i.e., it does
> not report the PID of the process that exited.
>
> With the multi-target patch, that would result in a failed assertion,
> trying to find the inferior for process pid 0.
>
> gdb/ChangeLog:
> yyyy-mm-dd Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> Pedro Alves <palves@redhat.com>
>
> * remote.c (remote_target::remote_parse_stop_reply) <W/X packets>:
> If no process is specified, return null_ptid instead of
> inferior_ptid.
> (remote_target::wait_as): Handle TARGET_WAITKIND_EXITED /
> TARGET_WAITKIND_SIGNALLED with no pid.
>
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> Pedro Alves <palves@redhat.com>
>
> * gdb.server/connect-without-multi-process.exp: Also test
> continuing to end.
> ---
> gdb/remote.c | 18 +++++++++++++-----
> .../gdb.server/connect-without-multi-process.exp | 7 ++++++-
> 2 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 8f3fdf098d..ad8a14d80b 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -7446,7 +7446,6 @@ Packet: '%s'\n"),
> case 'W': /* Target exited. */
> case 'X':
> {
> - int pid;
> ULONGEST value;
>
> /* GDB used to accept only 2 hex chars here. Stubs should
> @@ -7470,8 +7469,9 @@ Packet: '%s'\n"),
> event->ws.value.sig = GDB_SIGNAL_UNKNOWN;
> }
>
> - /* If no process is specified, assume inferior_ptid. */
> - pid = inferior_ptid.pid ();
> + /* If no process is specified, return null_ptid, and let the
> + caller figure out the right process to use. */
> + int pid = 0;
> if (*p == '\0')
> ;
> else if (*p == ';')
> @@ -7847,8 +7847,16 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status, int options)
> event_ptid = first_remote_resumed_thread ();
> }
> else
> - /* A process exit. Invalidate our notion of current thread. */
> - record_currthread (rs, minus_one_ptid);
> + {
> + /* A process exit. Invalidate our notion of current thread. */
> + record_currthread (rs, minus_one_ptid);
> + /* It's possible that the packet did not include a pid. */
> + if (event_ptid == null_ptid)
> + event_ptid = first_remote_resumed_thread ();
> + /* EVENT_PTID could still be NULL_PTID. Double-check. */
> + if (event_ptid == null_ptid)
> + event_ptid = magic_null_ptid;
> + }
>
> return event_ptid;
> }
> diff --git a/gdb/testsuite/gdb.server/connect-without-multi-process.exp b/gdb/testsuite/gdb.server/connect-without-
> multi-process.exp
> index fba20a6a0b..6ba35bf508 100644
> --- a/gdb/testsuite/gdb.server/connect-without-multi-process.exp
> +++ b/gdb/testsuite/gdb.server/connect-without-multi-process.exp
> @@ -14,7 +14,7 @@
> # along with this program. If not, see <http://www.gnu.org/licenses/>. */
>
> # Check that we can connect to GDBserver with the multiprocess
> -# extensions disabled, and run to main.
> +# extensions disabled, run to main, and finish the process.
>
> load_lib gdbserver-support.exp
>
> @@ -52,6 +52,11 @@ proc do_test {multiprocess} {
> "target $gdbserver_protocol"
>
> gdb_test "continue" "main .*" "continue to main"
> +
> + # The W/X packets do not include the PID of the exiting process
> + # without the multi-process extensions. Check that we handle
> + # process exit correctly in that case.
> + gdb_continue_to_end
> }
>
> foreach multiprocess { "off" "auto" } {
> --
> 2.14.5
>
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928