[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)
Pedro Alves
palves@redhat.com
Thu Oct 17 00:54:00 GMT 2019
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(-)
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
More information about the Gdb-patches
mailing list