Patchwork [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)

login
register
mail settings
Submitter Pedro Alves
Date Oct. 17, 2019, 12:54 a.m.
Message ID <a60eeb53-ddc2-fa21-8c73-d4fb266fc357@redhat.com>
Download mbox | patch
Permalink /patch/35079/
State New
Headers show

Comments

Pedro Alves - Oct. 17, 2019, 12:54 a.m.
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:

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(-)
Tankut Baris Aktemur - Oct. 17, 2019, 10:45 a.m.
* 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

Patch

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" } {