| Message ID | a60eeb53-ddc2-fa21-8c73-d4fb266fc357@redhat.com |
|---|---|
| State | New |
| Headers | show |
* 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
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" } {