public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* GDB 8.1 release -- 2018-01-08 update
@ 2018-01-08  7:49 Joel Brobecker
  2018-01-08  9:55 ` Maciej W. Rozycki
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Joel Brobecker @ 2018-01-08  7:49 UTC (permalink / raw)
  To: gdb-patches; +Cc: Pedro Alves, macro, tom, simon.marchi

Hello,

The gdb-8.1-branch was created last Friday :).

Here is the list of issues identified as blocking for 8.1 release:

  * [Pedro] gdb/22583
    gdb.base/breakpoint-in-ro-region.exp regressions on software
    single-step tar gets

  * [Maciej] remote/22597
    Empty `qsThreadInfo' reply handling regression causing inability to execute

    I'm trying to understand whether this is specific to mips or more
    general. And whether this only affects GDB when debugging with older
    stubs or whether it affects us more generally.

    Depending on the answer, the issue might not be so severe as
    to hold the release.

    Maciej - can you tell where we are on this issue, and whether
    you think it really is blocking for 8.1?

  * [Pedro/Joel] gdb/22670
    regressions in Ada caused by introduction of wild matching in
    C++ patch series

    Pedro has a fix for all the issues identified so far.
    On my end, I have gone through most of the failures I see
    using the AdaCore testsuite, with 5 left to go. Intuitively,
    I think we're through, but I'll have confirmation by end of
    this week.

  * [TomT] no PR yet
    Regression on 32-bit: gdb.guile/scm-ports.exp [Re: [RFA 1/2]
    Fix two regress ions in scalar printing
    https://sourceware.org/ml/gdb-patches/2017-12/msg00215.html

    Tom posted a patch which was recently approved. So it's a matter
    of pushing it, and backporting it.

  * [SimonM] no PR yet
    hurd: Add enough auxv support for AT_ENTRY for PIE binaries

    Patch looks good to Simon. Just needs to be pushed, or else
    backported if not pushed before branching.

    Any progress on this patch?

Looking at the list, once we close gdb/22670, I think we can
make our first pre-release, but please let me know if you believe
some of those issues above are critical enough that a pre-release
would not be useful yet.

Thanks!
-- 
Joel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: GDB 8.1 release -- 2018-01-08 update
  2018-01-08  7:49 GDB 8.1 release -- 2018-01-08 update Joel Brobecker
@ 2018-01-08  9:55 ` Maciej W. Rozycki
  2018-01-08 16:35   ` Pedro Alves
  2018-01-08 15:50 ` GDB 8.1 release -- 2018-01-08 update Simon Marchi
  2018-01-08 15:53 ` Tom Tromey
  2 siblings, 1 reply; 9+ messages in thread
From: Maciej W. Rozycki @ 2018-01-08  9:55 UTC (permalink / raw)
  To: Joel Brobecker
  Cc: gdb-patches, Pedro Alves, Maciej W. Rozycki, tom, simon.marchi

Hi Joel,

>   * [Maciej] remote/22597
>     Empty `qsThreadInfo' reply handling regression causing inability to execute
> 
>     I'm trying to understand whether this is specific to mips or more
>     general. And whether this only affects GDB when debugging with older
>     stubs or whether it affects us more generally.
> 
>     Depending on the answer, the issue might not be so severe as
>     to hold the release.
> 
>     Maciej - can you tell where we are on this issue, and whether
>     you think it really is blocking for 8.1?

 GDB uses the special thread ID 0, standing for `any', which older 
`gdbserver' versions do not recognise.  It does not verify beforehand 
whether `gdbserver' supports this request and does not handle an error 
reply gracefully.  Consequently an error reply to a `Hg0' packet issued 
causes GDB to lose track of what is going on, making it impossible to 
continue with the debug session.  This happens with all sessions in the 
initial connection handshake, making the combination of new GDB and old 
`gdbserver' unusable.

 Additionally a `Hg' packet with a legitimate thread ID (the only one at 
this point) is issued, which is handled correctly and returns a successful 
reply, immediately before the offending `Hg0' packet with no actions in 
between.  Which means the `Hg0' packet is not really needed as it wouldn't 
do anything anyway if `gdbserver' supported it.

 Previously no `Hg' packets were issued at this point in the initial 
connection handshake; GDB was satisfied with the single thread ID returned 
by `ThreadInfo' packet queries initially issued and didn't try at this 
point to switch to the same thread either named explicitly or with the 
special `any' thread ID, and debugging worked just fine.

 I can see in the remote log however that an error reply returned in 
response to `Hg0' issued earlier on is handled gracefully as is for `Hc-1' 
issued elsewhere, so this is specific to the point and sequence shown in 
the bug report, and likely the issuing place that is not prepared.  I can 
share the full good and bad log I have if that might help.

 I have no low-level understanding as to how the offending commit actually 
caused this change in behaviour, so I cannot comment on it.  I think we 
should avoid introducing functional regressions and continue supporting 
older stubs, which people may (have to) use for one reason or another.  
Therefore I think it would be preferable if we fixed the regression before 
8.1.

 Does it answer your question?

  Maciej

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: GDB 8.1 release -- 2018-01-08 update
  2018-01-08  7:49 GDB 8.1 release -- 2018-01-08 update Joel Brobecker
  2018-01-08  9:55 ` Maciej W. Rozycki
@ 2018-01-08 15:50 ` Simon Marchi
  2018-01-08 15:53 ` Tom Tromey
  2 siblings, 0 replies; 9+ messages in thread
From: Simon Marchi @ 2018-01-08 15:50 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches, Pedro Alves, macro, tom, simon.marchi

On 2018-01-08 02:49, Joel Brobecker wrote:
>   * [SimonM] no PR yet
>     hurd: Add enough auxv support for AT_ENTRY for PIE binaries
> 
>     Patch looks good to Simon. Just needs to be pushed, or else
>     backported if not pushed before branching.
> 
>     Any progress on this patch?

This is now pushed (master and branch).  I moved the item on the wiki to 
done.

Simon

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: GDB 8.1 release -- 2018-01-08 update
  2018-01-08  7:49 GDB 8.1 release -- 2018-01-08 update Joel Brobecker
  2018-01-08  9:55 ` Maciej W. Rozycki
  2018-01-08 15:50 ` GDB 8.1 release -- 2018-01-08 update Simon Marchi
@ 2018-01-08 15:53 ` Tom Tromey
  2 siblings, 0 replies; 9+ messages in thread
From: Tom Tromey @ 2018-01-08 15:53 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches, Pedro Alves, macro, tom, simon.marchi

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel>     Tom posted a patch which was recently approved. So it's a matter
Joel>     of pushing it, and backporting it.

I still need to re-research it to answer Pedro's questions.

Tom

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: GDB 8.1 release -- 2018-01-08 update
  2018-01-08  9:55 ` Maciej W. Rozycki
@ 2018-01-08 16:35   ` Pedro Alves
  2018-01-08 17:00     ` Maciej W. Rozycki
  2018-01-09  1:28     ` [PATCH] Fix backwards compatibility with old GDBservers (PR remote/22597) (Re: GDB 8.1 release -- 2018-01-08 update) Pedro Alves
  0 siblings, 2 replies; 9+ messages in thread
From: Pedro Alves @ 2018-01-08 16:35 UTC (permalink / raw)
  To: Maciej W. Rozycki, Joel Brobecker
  Cc: gdb-patches, Maciej W. Rozycki, tom, simon.marchi

On 01/08/2018 09:54 AM, Maciej W. Rozycki wrote:
> Hi Joel,
> 
>>   * [Maciej] remote/22597
>>     Empty `qsThreadInfo' reply handling regression causing inability to execute
>>
>>     I'm trying to understand whether this is specific to mips or more
>>     general. And whether this only affects GDB when debugging with older
>>     stubs or whether it affects us more generally.
>>
>>     Depending on the answer, the issue might not be so severe as
>>     to hold the release.
>>
>>     Maciej - can you tell where we are on this issue, and whether
>>     you think it really is blocking for 8.1?
> 
>  GDB uses the special thread ID 0, standing for `any', which older 
> `gdbserver' versions do not recognise.  It does not verify beforehand 
> whether `gdbserver' supports this request and does not handle an error 
> reply gracefully.  Consequently an error reply to a `Hg0' packet issued 
> causes GDB to lose track of what is going on, making it impossible to 
> continue with the debug session.  This happens with all sessions in the 
> initial connection handshake, making the combination of new GDB and old 
> `gdbserver' unusable.

I'm looking at this.  I can reproduce it on x86-64 using a gdbserver
from 2007 (git hash "f8b73d13b7ca^", the same revision Maciej's
gdbserver is built from).  I confirm that 5cd63fda035d somehow
introduces the regression.  No idea why yet.

So not specific to MIPS.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: GDB 8.1 release -- 2018-01-08 update
  2018-01-08 16:35   ` Pedro Alves
@ 2018-01-08 17:00     ` Maciej W. Rozycki
  2018-01-09  1:28     ` [PATCH] Fix backwards compatibility with old GDBservers (PR remote/22597) (Re: GDB 8.1 release -- 2018-01-08 update) Pedro Alves
  1 sibling, 0 replies; 9+ messages in thread
From: Maciej W. Rozycki @ 2018-01-08 17:00 UTC (permalink / raw)
  To: Pedro Alves
  Cc: Joel Brobecker, gdb-patches, Maciej W. Rozycki, tom, simon.marchi

On Mon, 8 Jan 2018, Pedro Alves wrote:

> >  GDB uses the special thread ID 0, standing for `any', which older 
> > `gdbserver' versions do not recognise.  It does not verify beforehand 
> > whether `gdbserver' supports this request and does not handle an error 
> > reply gracefully.  Consequently an error reply to a `Hg0' packet issued 
> > causes GDB to lose track of what is going on, making it impossible to 
> > continue with the debug session.  This happens with all sessions in the 
> > initial connection handshake, making the combination of new GDB and old 
> > `gdbserver' unusable.
> 
> I'm looking at this.  I can reproduce it on x86-64 using a gdbserver
> from 2007 (git hash "f8b73d13b7ca^", the same revision Maciej's
> gdbserver is built from).  I confirm that 5cd63fda035d somehow
> introduces the regression.  No idea why yet.

 Great, thanks!

  Maciej

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] Fix backwards compatibility with old GDBservers (PR remote/22597) (Re: GDB 8.1 release -- 2018-01-08 update)
  2018-01-08 16:35   ` Pedro Alves
  2018-01-08 17:00     ` Maciej W. Rozycki
@ 2018-01-09  1:28     ` Pedro Alves
  2018-01-10 11:18       ` Maciej W. Rozycki
  1 sibling, 1 reply; 9+ messages in thread
From: Pedro Alves @ 2018-01-09  1:28 UTC (permalink / raw)
  To: Maciej W. Rozycki, Joel Brobecker
  Cc: gdb-patches, Maciej W. Rozycki, tom, simon.marchi

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

On 01/08/2018 04:34 PM, Pedro Alves wrote:
> On 01/08/2018 09:54 AM, Maciej W. Rozycki wrote:
>>
>>  GDB uses the special thread ID 0, standing for `any', which older 
>> `gdbserver' versions do not recognise.  It does not verify beforehand 
>> whether `gdbserver' supports this request and does not handle an error 
>> reply gracefully.  Consequently an error reply to a `Hg0' packet issued 
>> causes GDB to lose track of what is going on, making it impossible to 
>> continue with the debug session.  This happens with all sessions in the 
>> initial connection handshake, making the combination of new GDB and old 
>> `gdbserver' unusable.
> 
> I'm looking at this.  I can reproduce it on x86-64 using a gdbserver
> from 2007 (git hash "f8b73d13b7ca^", the same revision Maciej's
> gdbserver is built from).  I confirm that 5cd63fda035d somehow
> introduces the regression.  No idea why yet.
> 
> So not specific to MIPS.

So the "E01"s in response to Hg0 aren't really what causes things
to get out of sync.  GDB doesn't actually throw an error in response
to those "E01"'s.

The real 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 somehow
GDB added a second thread.  It's because GDB thinks there's
more than one thread that we start seeing the Hg (select
general thread) packets.

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 in the attached
patch 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.

No regressions on master (against master gdbserver).

Maciej, does this work for you / MIPS?

[-- Attachment #2: 0001-Fix-backwards-compatibility-with-old-GDBservers-PR-r.patch --]
[-- Type: text/x-patch, Size: 5388 bytes --]

From 120d9979a082b349590f64faa40dba0c04716646 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 9 Jan 2018 00:37:36 +0000
Subject: [PATCH] Fix backwards compatibility with old GDBservers (PR
 remote/22597)

gdb/testsuite/ChangeLog:
yyyy-mm-dd  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:
yyyy-mm-dd  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/remote.c                                      |  8 ++-
 gdb/testsuite/gdb.server/stop-reply-no-thread.c   | 22 +++++++
 gdb/testsuite/gdb.server/stop-reply-no-thread.exp | 74 +++++++++++++++++++++++
 3 files changed, 103 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

diff --git a/gdb/remote.c b/gdb/remote.c
index 81c772a5451..a1cd9ae1df3 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6940,7 +6940,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"
-- 
2.14.3


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Fix backwards compatibility with old GDBservers (PR remote/22597) (Re: GDB 8.1 release -- 2018-01-08 update)
  2018-01-09  1:28     ` [PATCH] Fix backwards compatibility with old GDBservers (PR remote/22597) (Re: GDB 8.1 release -- 2018-01-08 update) Pedro Alves
@ 2018-01-10 11:18       ` Maciej W. Rozycki
  2018-01-11  0:36         ` Pedro Alves
  0 siblings, 1 reply; 9+ messages in thread
From: Maciej W. Rozycki @ 2018-01-10 11:18 UTC (permalink / raw)
  To: Pedro Alves
  Cc: Joel Brobecker, gdb-patches, Maciej W. Rozycki, tom, simon.marchi

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.

  Maciej

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Fix backwards compatibility with old GDBservers (PR remote/22597) (Re: GDB 8.1 release -- 2018-01-08 update)
  2018-01-10 11:18       ` Maciej W. Rozycki
@ 2018-01-11  0:36         ` Pedro Alves
  0 siblings, 0 replies; 9+ messages in thread
From: Pedro Alves @ 2018-01-11  0:36 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: Joel Brobecker, gdb-patches, Maciej W. Rozycki, tom, simon.marchi

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

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"
-- 
2.14.3

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-01-11  0:36 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-08  7:49 GDB 8.1 release -- 2018-01-08 update Joel Brobecker
2018-01-08  9:55 ` Maciej W. Rozycki
2018-01-08 16:35   ` Pedro Alves
2018-01-08 17:00     ` Maciej W. Rozycki
2018-01-09  1:28     ` [PATCH] Fix backwards compatibility with old GDBservers (PR remote/22597) (Re: GDB 8.1 release -- 2018-01-08 update) Pedro Alves
2018-01-10 11:18       ` Maciej W. Rozycki
2018-01-11  0:36         ` Pedro Alves
2018-01-08 15:50 ` GDB 8.1 release -- 2018-01-08 update Simon Marchi
2018-01-08 15:53 ` Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).