Re: [PATCH 14/23] Tweak handling of remote errors in response to resumption packet

Aktemur, Tankut Baris
2019-10-09T13:35:01Z

[-- Attachment #1: Type: text/plain, Size: 5748 bytes --]

* On September 7, 2019 1:28 AM, Pedro Alves wrote:
>
> With current master, on a Fedora 27 machine with a kernel with buggy
> watchpoint support, I see:
>
> (gdb) PASS: gdb.threads/watchpoint-fork.exp: parent: singlethreaded: hardware breakpoints work
> continue
> Continuing.
> warning: Remote failure reply: E01
> Remote communication error. Target disconnected.: Connection reset by peer.
> (gdb) FAIL: gdb.threads/watchpoint-fork.exp: parent: singlethreaded: watchpoints work
> continue
> The program is not being run.
> (gdb) FAIL: gdb.threads/watchpoint-fork.exp: parent: singlethreaded: breakpoint after the first fork (the program
> is no longer running)
>
> The FAILs themselves aren't what's interesting here. What is
> interesting is that with the main multi-target patch applied, I was getting this:
>
> (gdb) PASS: gdb.threads/watchpoint-fork.exp: parent: singlethreaded: hardware breakpoints work
> continue
> Continuing.
> warning: Remote failure reply: E01
> /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/inferior.c:285: internal-error: inferior*
> find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) FAIL: gdb.threads/watchpoint-fork.exp: parent: singlethreaded: watchpoints
> work (GDB internal error)
>
> The problem is that in remote_target::wait_as, we're hitting this:
>
> switch (buf[0])
> {
> case 'E': /* Error of some sort. */
> /* We're out of sync with the target now. Did it continue or
> not? Not is more likely, so report a stop. */
> rs->waiting_for_stop_reply = 0;
>
> warning (_("Remote failure reply: %s"), buf);
> status->kind = TARGET_WAITKIND_STOPPED;
> status->value.sig = GDB_SIGNAL_0;
> break;
>
> which leaves event_ptid as null_ptid. At the end of the function, we then reach:
>
> else if (status->kind != TARGET_WAITKIND_EXITED
> && status->kind != TARGET_WAITKIND_SIGNALLED)
> {
> if (event_ptid != null_ptid)
> record_currthread (rs, event_ptid);
> else
> event_ptid = inferior_ptid; <<<<< here
> }
>
> and the trouble is that with the multi-target patch, we'll get here
> with inferior_ptid as null_ptid too. That is done exactly to find
> these implicit assumptions that inferior_ptid is a good choice for
> default thread, which isn't generaly true.
>
> I first thought of fixing this in the "case 'E'" path, but, given that
> this "event_ptid = inferior_ptid" path is also taken when the remote
> target does not support threads at all, no thread-related packets or
> extensions, it's better to fix it in latter path, to handle all
> scenarios that miss reporting a thread.
>
> That's what this patch does.
>

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.

To also catch the process exit case, the coverage of the patch can be
expanded. Instead of

else
/* A process exit. Invalidate our notion of current thread. */
record_currthread (rs, minus_one_ptid);

the following can be done:

else
{
/* 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 (this);
/* EVENT_PTID could still be NULL_PTID. Double-check. */
if (event_ptid == null_ptid)
event_ptid = magic_null_ptid;
}

I'm attaching these as a patch in case one wants to test. It should be applied
after the main multi-target patch.

Regards,
-Baris

> gdb/ChangeLog:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * remote.c (first_remote_resumed_thread): New.
> (remote_target::wait_as): Use it as default event_ptid instead of
> inferior_ptid.
> ---
> gdb/remote.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index bed861c65d..eacaf11976 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -7703,6 +7703,17 @@ remote_target::wait_ns (ptid_t ptid, struct target_waitstatus *status, int optio
> }
> }
>
> +/* Return the first resumed thread. */
> +
> +static ptid_t
> +first_remote_resumed_thread ()
> +{
> + for (thread_info *tp : all_non_exited_threads (minus_one_ptid))
> + if (tp->resumed)
> + return tp->ptid;
> + return null_ptid;
> +}
> +
> /* Wait until the remote machine stops, then return, storing status in
> STATUS just as `wait' would. */
>
> @@ -7839,7 +7850,7 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status, int options)
> if (event_ptid != null_ptid)
> record_currthread (rs, event_ptid);
> else
> - event_ptid = inferior_ptid;
> + event_ptid = first_remote_resumed_thread ();
> }
> else
> /* A process exit. Invalidate our notion of current thread. */
> --
> 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

[-- Attachment #2: remote_event_ptid.patch --]
[-- Type: application/octet-stream, Size: 1763 bytes --]

diff --git a/gdb/remote.c b/gdb/remote.c
index ff75204ee8..d036dcf9cf 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7919,8 +7919,16 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status, int options)
event_ptid = first_remote_resumed_thread (this);
}
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 (this);
+ /* 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..36b42207e2 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,9 @@ proc do_test {multiprocess} {
"target $gdbserver_protocol"

gdb_test "continue" "main .*" "continue to main"
+
+ # Also test the termination because the 'W' packet differs
+ gdb_test "continue" ".* exited normally.*" "continue to termination"
}

foreach multiprocess { "off" "auto" } {