Patchwork Fix backwards compatibility with old GDBservers (PR remote/22597) (Re: GDB 8.1 release -- 2018-01-08 update)

login
register
mail settings
Submitter Pedro Alves
Date Jan. 11, 2018, 12:36 a.m.
Message ID <07a9d71f-5c58-0094-a39f-b02a349c140a@redhat.com>
Download mbox | patch
Permalink /patch/25336/
State New
Headers show

Comments

Pedro Alves - Jan. 11, 2018, 12:36 a.m.
On 01/10/2018 11:18 AM, Maciej W. Rozycki wrote:
> On Mon, 8 Jan 2018, Pedro Alves wrote:
> 
>> Maciej, does this work for you / MIPS?
> 
>  It does, with the same `gdbserver' binary I reported the failure against.  
> Thank you for your fix and sorry to mislead you several times about the 
> nature of this regression.

No worries, what was happening wasn't obvious from the logs.

Thanks for discovering the issue, and sorry for causing it in
the first place.

Below's what I pushed, to both master and branch.  (Same patch, but
with info aggregated in the commit log).

I was working on a patch for the other regression you reported,
before the holidays.  I'll try to dig it up tomorrow.

From 936a312c04f9f1dd856571bf7573b5a8f51ed895 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 11 Jan 2018 00:24:59 +0000
Subject: [PATCH] Fix backwards compatibility with old GDBservers (PR
 remote/22597)

At <https://sourceware.org/ml/gdb-patches/2017-12/msg00285.html>,
Maciej reported that commit:

  commit 5cd63fda035d4ba949e6478406162c4673b3c9ef
  Date: Wed Oct 4 18:21:10 2017 +0100
  Subject: Fix "Remote 'g' packet reply is too long" problems with multiple inferiors

made GDB stop working with older stubs.  Any attempt to continue
execution after the initial connection fails with:

  [...]
  Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid = 2670
  Listening on port 2346
  target remote [...]:2346
  Remote debugging using [...]:2346
  Reading symbols from .../lib64/ld.so.1...done.
  [Switching to Thread <main>]
  (gdb) continue
  Cannot execute this command without a live selected thread.
  (gdb)

The problem is:

  (gdb) c
  Cannot execute this command without a live selected thread.
  (gdb) info threads
    Id   Target Id         Frame
    1    Thread 14917      0x00007f341cd98ed0 in _start () from /lib64/ld-linux-x86-64.so.2

  The current thread <Thread ID 2> has terminated.  See `help thread'.
		      ^^^^^^^^^^^
  (gdb)

Note, thread _2_.  There's really only one thread in the inferior
(it's still at the entry point), but still GDB added a bogus second
thread.

The reason GDB started adding a second thread after 5cd63fda035d is
this hunk:

+                 if (event->ptid == null_ptid)
+                   {
+                     const char *thr = strstr (p1 + 1, ";thread:");
+                     if (thr != NULL)
+                       event->ptid = read_ptid (thr + strlen (";thread:"),
+                                                NULL);
+                     else
+                       event->ptid = magic_null_ptid;
+                   }

Note the else branch that falls back to magic_null_ptid.  We reach
that when we process the initial stop reply sent back in response to
the the "?" (status) packet early in the connection setup:

 Sending packet: $?#3f...Ack
 Packet received: T0506:0000000000000000;07:40a510f4fd7f0000;10:d0fe1201577f0000;

And note that that response does not include a ";thread:XXX" part.

This stop reply is processed after listing threads with qfThreadInfo /
qsThreadInfo :

 Sending packet: $qfThreadInfo#bb...Ack
 Packet received: m3915
 Sending packet: $qsThreadInfo#c8...Ack
 Packet received: l

meaning, when we process that stop reply, we treat the event as coming
from a thread with ptid == magic_null_ptid, which is not yet in the
thread list, so we add it then:

  (top-gdb) p ptid
  $1 = {m_pid = 42000, m_lwp = -1, m_tid = 1}
  (top-gdb) bt
  #0  0x0000000000840a8c in add_thread_silent(ptid_t) (ptid=...) at src/gdb/thread.c:269
  #1  0x00000000007ad61d in remote_add_thread(ptid_t, int, int) (ptid=..., running=0, executing=0)
      at src/gdb/remote.c:1838
  #2  0x00000000007ad8de in remote_notice_new_inferior(ptid_t, int) (currthread=..., executing=0)
      at src/gdb/remote.c:1921
  #3  0x00000000007b758b in process_stop_reply(stop_reply*, target_waitstatus*) (stop_reply=0x1158860, status=0x7fffffffcc00)
      at src/gdb/remote.c:7217
  #4  0x00000000007b7a38 in remote_wait_as(ptid_t, target_waitstatus*, int) (ptid=..., status=0x7fffffffcc00, options=0)
      at src/gdb/remote.c:7380
  #5  0x00000000007b7cd1 in remote_wait(target_ops*, ptid_t, target_waitstatus*, int) (ops=0x102fac0 <remote_ops>, ptid=..., status=0x7fffffffcc00, options=0) at src/gdb/remote.c:7446
  #6  0x000000000081587b in delegate_wait(target_ops*, ptid_t, target_waitstatus*, int) (self=0x102fac0 <remote_ops>, arg1=..., arg2=0x7fffffffcc00, arg3=0) at src/gdb/target-delegates.c:138
  #7  0x0000000000827d77 in target_wait(ptid_t, target_waitstatus*, int) (ptid=..., status=0x7fffffffcc00, options=0)
      at src/gdb/target.c:2179
  #8  0x0000000000715fda in do_target_wait(ptid_t, target_waitstatus*, int) (ptid=..., status=0x7fffffffcc00, options=0)
      at src/gdb/infrun.c:3589
  #9  0x0000000000716351 in wait_for_inferior() () at src/gdb/infrun.c:3707
  #10 0x0000000000715435 in start_remote(int) (from_tty=1) at src/gdb/infrun.c:3212

things go downhill from this.

We don't see the problem with current master gdbserver, because that
version always sends the ";thread:" part in the initial stop reply:

 Sending packet: $?#3f...Packet received: T0506:0000000000000000;07:a0d4ffffff7f0000;10:d05eddf7ff7f0000;thread:p3cea.3cea;core:3;

Years ago I had added a "--disable-packet=" command line option to
gdbserver which comes in handy for testing this, since the existing
"--disable-packet=Tthread" precisely makes gdbserver not send that
";thread:" part in stop replies.  The testcase added by this commit
emulates old gdbserver making use of that.

I've compared a testrun at 5cd63fda035d^ (before regression) with
'current master+patch', against old gdbserver at f8b73d13b7ca^.  I
hacked out --once, and "monitor exit" to be able to test.  The results
are a bit too unstable to tell accurately, but it looked like there
were no regressions.  Maciej confirmed this worked for him as well.

No regressions on master (against master gdbserver).

gdb/ChangeLog:
2018-01-11  Pedro Alves  <palves@redhat.com>

	PR remote/22597
	* remote.c (remote_parse_stop_reply): Default to the last-set
	general thread instead of to 'magic_null_ptid'.

gdb/testsuite/ChangeLog:
2018-01-11  Pedro Alves  <palves@redhat.com>

	PR remote/22597
	* gdb.server/stop-reply-no-thread.c: New file.
	* gdb.server/stop-reply-no-thread.exp: New file.
---
 gdb/ChangeLog                                     |  6 ++
 gdb/testsuite/ChangeLog                           |  6 ++
 gdb/remote.c                                      |  8 ++-
 gdb/testsuite/gdb.server/stop-reply-no-thread.c   | 22 +++++++
 gdb/testsuite/gdb.server/stop-reply-no-thread.exp | 74 +++++++++++++++++++++++
 5 files changed, 115 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.server/stop-reply-no-thread.c
 create mode 100644 gdb/testsuite/gdb.server/stop-reply-no-thread.exp

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7d56bf2447f..8edb782f087 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@ 
+2018-01-11  Pedro Alves  <palves@redhat.com>
+
+	PR remote/22597
+	* remote.c (remote_parse_stop_reply): Default to the last-set
+	general thread instead of to 'magic_null_ptid'.
+
 2018-01-10  Pedro Alves  <palves@redhat.com>
 
 	* language.h (language_get_symbol_name_matcher): Rename ...
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 8612d1e32ac..7d7c389d98d 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@ 
+2018-01-11  Pedro Alves  <palves@redhat.com>
+
+	PR remote/22597
+	* gdb.server/stop-reply-no-thread.c: New file.
+	* gdb.server/stop-reply-no-thread.exp: New file.
+
 2018-01-10  Pedro Alves  <palves@redhat.com>
 
 	PR gdb/22670
diff --git a/gdb/remote.c b/gdb/remote.c
index 9ff6028b8d8..a4265087233 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6939,7 +6939,13 @@  Packet: '%s'\n"),
 			event->ptid = read_ptid (thr + strlen (";thread:"),
 						 NULL);
 		      else
-			event->ptid = magic_null_ptid;
+			{
+			  /* Either the current thread hasn't changed,
+			     or the inferior is not multi-threaded.
+			     The event must be for the thread we last
+			     set as (or learned as being) current.  */
+			  event->ptid = event->rs->general_thread;
+			}
 		    }
 
 		  if (rsa == NULL)
diff --git a/gdb/testsuite/gdb.server/stop-reply-no-thread.c b/gdb/testsuite/gdb.server/stop-reply-no-thread.c
new file mode 100644
index 00000000000..a9058de0483
--- /dev/null
+++ b/gdb/testsuite/gdb.server/stop-reply-no-thread.c
@@ -0,0 +1,22 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.server/stop-reply-no-thread.exp b/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
new file mode 100644
index 00000000000..2bb8c7ac7c5
--- /dev/null
+++ b/gdb/testsuite/gdb.server/stop-reply-no-thread.exp
@@ -0,0 +1,74 @@ 
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2018 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test backward compatibility with older GDBservers which did not
+# include ";thread:NNN" in T stop replies when debugging
+# single-threaded programs, even though they'd list the main thread in
+# response to qfThreadInfo/qsThreadInfo.  See PR remote/22597.
+
+load_lib gdbserver-support.exp
+
+if { [skip_gdbserver_tests] } {
+    verbose "skipping gdbserver tests"
+    return -1
+}
+
+standard_testfile
+if [prepare_for_testing "failed to prepare" $testfile $srcfile] {
+    return -1
+}
+
+# Make sure we're disconnected, in case we're testing with an
+# extended-remote board, therefore already connected.
+gdb_test "disconnect" ".*"
+
+# Start GDBserver, with ";thread:NNN" in T stop replies disabled,
+# emulating old gdbservers when debugging single-threaded programs.
+set res [gdbserver_start "--disable-packet=Tthread" $binfile]
+set gdbserver_protocol [lindex $res 0]
+set gdbserver_gdbport [lindex $res 1]
+
+# Disable XML-based thread listing, and multi-process extensions.
+gdb_test_no_output "set remote threads-packet off"
+gdb_test_no_output "set remote multiprocess-feature-packet off"
+
+set res [gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport]
+if ![gdb_assert {$res == 0} "connect"] {
+    return
+}
+
+# There should be only one thread listed.
+set test "info threads"
+gdb_test_multiple $test $test {
+    -re "2 Thread.*$gdb_prompt $" {
+	fail $test
+    }
+    -re "has terminated.*$gdb_prompt $" {
+	fail $test
+    }
+    -re "\\\* 1\[\t \]*Thread\[^\r\n\]*\r\n$gdb_prompt $" {
+	pass $test
+    }
+}
+
+gdb_breakpoint "main"
+
+# Bad GDB behaved like this:
+#  (gdb) c
+#  Cannot execute this command without a live selected thread.
+#  (gdb)
+gdb_test "c" "Breakpoint $decimal, main.*" "continue to main"