[Bug varobj/23377] New: [8.1 -> 8.2 regression] gdb.mi/mi-var-cmd.exp, with gdbserver

[palves at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

Bug ID: 23377
Summary: [8.1 -> 8.2 regression] gdb.mi/mi-var-cmd.exp, with
gdbserver
Product: gdb
Version: unknown
Status: NEW
Severity: normal
Priority: P2
Component: varobj
Assignee: unassigned at sourceware dot org
Reporter: palves at redhat dot com
Target Milestone: ---

On Fedora 27, with --target_board=native-gdbserver/native-extended-gdbserver,
comparing 8.1 vs 8.2/master test results, I see this regression:

-PASS: gdb.base/multi-forks.exp: detach 4
-PASS: gdb.base/multi-forks.exp: detach 5
-PASS: gdb.base/multi-forks.exp: kill 6
+FAIL: gdb.base/multi-forks.exp: detach 4
+FAIL: gdb.base/multi-forks.exp: detach 5
+FAIL: gdb.base/multi-forks.exp: kill 6

gdb.log shows:

detach inferior 3
Detaching from program:
build/gdb/testsuite/outputs/gdb.base/multi-forks/multi-forks, process 1474
Remote connection closed
(gdb) PASS: gdb.base/multi-forks.exp: detach 3
detach inferior 4
warning: Inferior ID 4 is not running.
(gdb) FAIL: gdb.base/multi-forks.exp: detach 4
detach inferior 5

Native testing passes cleanly.

[palves at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

Pedro Alves <palves at redhat dot com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Summary|[8.1 -> 8.2 regression] |[8.1 -> 8.2 regression]
|gdb.mi/mi-var-cmd.exp, with |gdb.base/multi-forks.exp,
|gdbserver |with gdbserver

[palves at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

Pedro Alves <palves at redhat dot com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Component|varobj |gdb

[palves at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

Pedro Alves <palves at redhat dot com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Target Milestone|--- |8.2

[palves at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

--- Comment #1 from Pedro Alves <palves at redhat dot com> ---
Running the testcase manually reveals that this is gdbserver failing an
assertion:

$ ./gdbserver :9999 ../testsuite/outputs/gdb.base/multi-forks/multi-forks
Process ../testsuite/outputs/gdb.base/multi-forks/multi-forks created; pid =
24569
Listening on port 9999
Remote debugging from host 127.0.0.1
24569 ready
24569 done
24590 ready
24590 done
24591 ready
24591 done
24594 ready
24594 done
Detaching from process 24590
src/gdb/gdbserver/inferiors.c:212: A problem internal to GDBserver has been
detected.
process_info* current_process(): Assertion `current_thread != NULL' failed.

[palves at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

--- Comment #2 from Pedro Alves <palves at redhat dot com> ---
(gdb) bt
#0 0x000000000040a57e in internal_error(char const*, int, char const*, ...)
(file=0x4a53c0 "src/gdb/gdbserver/inferiors.c", line=212, fmt=0x4a539e "%s:
Assertion `%s' failed.") at src/gdb/gdbserver/../common/errors.c:54
#1 0x0000000000420acf in current_process() () at
src/gdb/gdbserver/inferiors.c:212
#2 0x00000000004226a0 in any_persistent_commands() () at
gdb/gdbserver/mem-break.c:308
#3 0x000000000042cb43 in handle_detach(char*) (own_buf=0x6f0280 "D;62ea") at
src/gdb/gdbserver/server.c:1210
#4 0x0000000000433af3 in process_serial_event() () at
src/gdb/gdbserver/server.c:4055
#5 0x0000000000434878 in handle_serial_event(int, void*) (err=0,
client_data=0x0)

[sergiodj at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

Sergio Durigan Junior <sergiodj at redhat dot com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |sergiodj at redhat dot com

--- Comment #3 from Sergio Durigan Junior <sergiodj at redhat dot com> ---
I ran a bisect here, and found the the failure started happening on:

f2ffa92bbce9dd5fbedc138ac2a3bc8a88327d09 is the first bad commit
commit f2ffa92bbce9dd5fbedc138ac2a3bc8a88327d09
Author: Pedro Alves <***@redhat.com>
Date: Thu Jun 28 20:18:24 2018 +0100

gdb: Eliminate the 'stop_pc' global

I'll see about investigating more later.

[palves at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

--- Comment #4 from Pedro Alves <palves at redhat dot com> ---
Thanks, but please don't bother -- I had git bisected too, and started writing
a patch last Friday. I'll finish it this week.

[sergiodj at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

--- Comment #5 from Sergio Durigan Junior <sergiodj at redhat dot com> ---
(In reply to Pedro Alves from comment #4)

Post by palves at redhat dot com
Thanks, but please don't bother -- I had git bisected too, and started
writing a patch last Friday. I'll finish it this week.

Hm, OK. However, I dug a bit deeper and found that the following patch solves
the regression, and doesn't introduce any more failures. In fact, I had the
impression that the initial patch to remove stop_pc shouldn't have removed this
specific piece of code... Anyway, opinions?

diff --git a/gdb/thread.c b/gdb/thread.c
index 517a80715e..ebd852570c 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1388,6 +1388,13 @@ switch_to_thread (thread_info *thr)
switch_to_thread_no_regs (thr);

reinit_frame_cache ();
+
+ /* We don't check for is_stopped, because we're called at times
+ while in the TARGET_RUNNING state, e.g., while handling an
+ internal event. */
+ if (thr->state != THREAD_EXITED
+ && !thr->executing)
+ thr->suspend.stop_pc = regcache_read_pc (get_thread_regcache (thr));
}

/* See common/common-gdbthread.h. */

[sergiodj at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

--- Comment #6 from Sergio Durigan Junior <sergiodj at redhat dot com> ---
<sergiodj> | palves: hey. I know you're busy with other things, but did you
have a chance to look at
https://sourceware.org/bugzilla/show_bug.cgi?id=23377#c5 and see if it makes
sense?
<palves> | yeah, i'm busy and couldn't reply yet. but, it does not make sense.
<palves> | the gist is simply that the whole point of the original patch was to
remove that bit you'd be adding.

Since Pedro is already working on a patch for the bug, I'll leave this it to
him.

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

--- Comment #7 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Pedro Alves <***@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=31445d1036f7fc41de2724cb016913c9b1461bb1

commit 31445d1036f7fc41de2724cb016913c9b1461bb1
Author: Pedro Alves <***@redhat.com>
Date: Wed Jul 11 23:31:44 2018 +0100

GDBserver: Don't assume a current process in D;PID implementation (PR
gdb/23377)

This fixes a gdb.base/multi-forks.exp regression with GDBserver.

Git commit f2ffa92bbce9 ("gdb: Eliminate the 'stop_pc' global") caused
the regression by exposing a latent bug in gdbserver.

The bug is that GDBserver's implementation of the D;PID packet
incorrectly assumes that the selected thread points to the process
being detached. This happens via the any_persistent_commands call,
which calls current_process:

(gdb) bt
#0 0x000000000040a57e in internal_error(char const*, int, char const*,
...)
(file=0x4a53c0 "src/gdb/gdbserver/inferiors.c", line=212, fmt=0x4a539e
"%s:
Assertion `%s' failed.") at src/gdb/gdbserver/../common/errors.c:54
#1 0x0000000000420acf in current_process() () at
src/gdb/gdbserver/inferiors.c:212
#2 0x00000000004226a0 in any_persistent_commands() () at
gdb/gdbserver/mem-break.c:308
#3 0x000000000042cb43 in handle_detach(char*) (own_buf=0x6f0280
"D;62ea") at
src/gdb/gdbserver/server.c:1210
#4 0x0000000000433af3 in process_serial_event() () at
src/gdb/gdbserver/server.c:4055
#5 0x0000000000434878 in handle_serial_event(int, void*) (err=0,
client_data=0x0)

The "eliminate stop_pc" commit exposes the problem because before that
commit, GDB's switch_to_thread always read the newly-selected thread's
PC, and that would end up forcing GDBserver's selected thread to
change accordingly as side effect. After that commit, GDB no longer
reads the thread's PC, and GDBserver does not switch the thread.

Fix this by removing the assumption from GDBserver.

gdb/gdbserver/ChangeLog:
2018-07-11 Pedro Alves <***@redhat.com>

PR gdb/23377
* mem-break.c (any_persistent_commands): Add process_info
parameter and use it instead of relying on the current process.
Change return type to bool.
* mem-break.h (any_persistent_commands): Add process_info
parameter and change return type to bool.
* server.c (handle_detach): Remove require_running_or_return call.
Look up the process_info for the process we're about to detach.
If not found, return back error to GDB. Adjust
any_persistent_commands call to pass down a process pointer.

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

--- Comment #8 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Pedro Alves <***@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=4c7333b308f5178813745f40e641231efb1cb763

commit 4c7333b308f5178813745f40e641231efb1cb763
Author: Pedro Alves <***@redhat.com>
Date: Wed Jul 11 23:31:44 2018 +0100

GDB: Work around D;PID handling bug in older GDBservers (PR gdb/23377)

This commit adds a GDB workaround for the GDBserver bug exposed by
commit f2ffa92bbce9 ("gdb: Eliminate the 'stop_pc' global"), so that
newer GDBs can continue working with older GDBservers.

gdb/ChangeLog:
2018-07-11 Pedro Alves <***@redhat.com>

PR gdb/23377
* remote.c (remote_target::remote_detach_pid): Call
set_current_process.

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

--- Comment #9 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The gdb-8.12-branch branch has been updated by Pedro Alves
<***@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=1f010560368f7726eba2629a3c26298cab5d87b3

commit 1f010560368f7726eba2629a3c26298cab5d87b3
Author: Pedro Alves <***@redhat.com>
Date: Wed Jul 11 23:40:50 2018 +0100

GDBserver: Don't assume a current process in D;PID implementation (PR
gdb/23377)

This fixes a gdb.base/multi-forks.exp regression with GDBserver.

Git commit f2ffa92bbce9 ("gdb: Eliminate the 'stop_pc' global") caused
the regression by exposing a latent bug in gdbserver.

The bug is that GDBserver's implementation of the D;PID packet
incorrectly assumes that the selected thread points to the process
being detached. This happens via the any_persistent_commands call,
which calls current_process:

(gdb) bt
#0 0x000000000040a57e in internal_error(char const*, int, char const*,
...)
(file=0x4a53c0 "src/gdb/gdbserver/inferiors.c", line=212, fmt=0x4a539e
"%s:
Assertion `%s' failed.") at src/gdb/gdbserver/../common/errors.c:54
#1 0x0000000000420acf in current_process() () at
src/gdb/gdbserver/inferiors.c:212
#2 0x00000000004226a0 in any_persistent_commands() () at
gdb/gdbserver/mem-break.c:308
#3 0x000000000042cb43 in handle_detach(char*) (own_buf=0x6f0280
"D;62ea") at
src/gdb/gdbserver/server.c:1210
#4 0x0000000000433af3 in process_serial_event() () at
src/gdb/gdbserver/server.c:4055
#5 0x0000000000434878 in handle_serial_event(int, void*) (err=0,
client_data=0x0)

The "eliminate stop_pc" commit exposes the problem because before that
commit, GDB's switch_to_thread always read the newly-selected thread's
PC, and that would end up forcing GDBserver's selected thread to
change accordingly as side effect. After that commit, GDB no longer
reads the thread's PC, and GDBserver does not switch the thread.

Fix this by removing the assumption from GDBserver.

gdb/gdbserver/ChangeLog:
2018-07-11 Pedro Alves <***@redhat.com>

PR gdb/23377
* mem-break.c (any_persistent_commands): Add process_info
parameter and use it instead of relying on the current process.
Change return type to bool.
* mem-break.h (any_persistent_commands): Add process_info
parameter and change return type to bool.
* server.c (handle_detach): Remove require_running_or_return call.
Look up the process_info for the process we're about to detach.
If not found, return back error to GDB. Adjust
any_persistent_commands call to pass down a process pointer.

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

--- Comment #10 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The gdb-8.12-branch branch has been updated by Pedro Alves
<***@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=49b000dc5686e016f05b717f18d2c8f865eb3617

commit 49b000dc5686e016f05b717f18d2c8f865eb3617
Author: Pedro Alves <***@redhat.com>
Date: Wed Jul 11 23:40:51 2018 +0100

GDB: Work around D;PID handling bug in older GDBservers (PR gdb/23377)

This commit adds a GDB workaround for the GDBserver bug exposed by
commit f2ffa92bbce9 ("gdb: Eliminate the 'stop_pc' global"), so that
newer GDBs can continue working with older GDBservers.

gdb/ChangeLog:
2018-07-11 Pedro Alves <***@redhat.com>

PR gdb/23377
* remote.c (remote_target::remote_detach_pid): Call
set_current_process.

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

--- Comment #11 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The gdb-8.2-branch branch has been updated by Pedro Alves
<***@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=1f010560368f7726eba2629a3c26298cab5d87b3

commit 1f010560368f7726eba2629a3c26298cab5d87b3
Author: Pedro Alves <***@redhat.com>
Date: Wed Jul 11 23:40:50 2018 +0100

GDBserver: Don't assume a current process in D;PID implementation (PR
gdb/23377)

This fixes a gdb.base/multi-forks.exp regression with GDBserver.

Git commit f2ffa92bbce9 ("gdb: Eliminate the 'stop_pc' global") caused
the regression by exposing a latent bug in gdbserver.

The bug is that GDBserver's implementation of the D;PID packet
incorrectly assumes that the selected thread points to the process
being detached. This happens via the any_persistent_commands call,
which calls current_process:

(gdb) bt
#0 0x000000000040a57e in internal_error(char const*, int, char const*,
...)
(file=0x4a53c0 "src/gdb/gdbserver/inferiors.c", line=212, fmt=0x4a539e
"%s:
Assertion `%s' failed.") at src/gdb/gdbserver/../common/errors.c:54
#1 0x0000000000420acf in current_process() () at
src/gdb/gdbserver/inferiors.c:212
#2 0x00000000004226a0 in any_persistent_commands() () at
gdb/gdbserver/mem-break.c:308
#3 0x000000000042cb43 in handle_detach(char*) (own_buf=0x6f0280
"D;62ea") at
src/gdb/gdbserver/server.c:1210
#4 0x0000000000433af3 in process_serial_event() () at
src/gdb/gdbserver/server.c:4055
#5 0x0000000000434878 in handle_serial_event(int, void*) (err=0,
client_data=0x0)

The "eliminate stop_pc" commit exposes the problem because before that
commit, GDB's switch_to_thread always read the newly-selected thread's
PC, and that would end up forcing GDBserver's selected thread to
change accordingly as side effect. After that commit, GDB no longer
reads the thread's PC, and GDBserver does not switch the thread.

Fix this by removing the assumption from GDBserver.

gdb/gdbserver/ChangeLog:
2018-07-11 Pedro Alves <***@redhat.com>

PR gdb/23377
* mem-break.c (any_persistent_commands): Add process_info
parameter and use it instead of relying on the current process.
Change return type to bool.
* mem-break.h (any_persistent_commands): Add process_info
parameter and change return type to bool.
* server.c (handle_detach): Remove require_running_or_return call.
Look up the process_info for the process we're about to detach.
If not found, return back error to GDB. Adjust
any_persistent_commands call to pass down a process pointer.

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

--- Comment #12 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The gdb-8.2-branch branch has been updated by Pedro Alves
<***@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=49b000dc5686e016f05b717f18d2c8f865eb3617

commit 49b000dc5686e016f05b717f18d2c8f865eb3617
Author: Pedro Alves <***@redhat.com>
Date: Wed Jul 11 23:40:51 2018 +0100

GDB: Work around D;PID handling bug in older GDBservers (PR gdb/23377)

This commit adds a GDB workaround for the GDBserver bug exposed by
commit f2ffa92bbce9 ("gdb: Eliminate the 'stop_pc' global"), so that
newer GDBs can continue working with older GDBservers.

gdb/ChangeLog:
2018-07-11 Pedro Alves <***@redhat.com>

PR gdb/23377
* remote.c (remote_target::remote_detach_pid): Call
set_current_process.

[palves at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23377

Pedro Alves <palves at redhat dot com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED

--- Comment #13 from Pedro Alves <palves at redhat dot com> ---
Fixed.