[Bug remote/22597] New: Empty `qsThreadInfo' reply handling regression causing inability to execute

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug remote/22597] New: Empty `qsThreadInfo' reply handling regression causing inability to execute

macro@linux-mips.org
https://sourceware.org/bugzilla/show_bug.cgi?id=22597

            Bug ID: 22597
           Summary: Empty `qsThreadInfo' reply handling regression causing
                    inability to execute
           Product: gdb
           Version: HEAD
            Status: NEW
          Severity: normal
          Priority: P2
         Component: remote
          Assignee: unassigned at sourceware dot org
          Reporter: [hidden email]
                CC: palves at redhat dot com
  Target Milestone: 8.1

At <https://sourceware.org/ml/gdb-patches/2017-12/msg00285.html>, I wrote:

On Tue, 12 Dec 2017, Maciej W. Rozycki wrote:

>  Another regression.  This makes `mips-mti-linux-gnu' GDB stop working
> with older stubs.  I have discovered it in an attempt to regression-test
> said GDB with a remote n64 target and `x86_64-pc-linux-gnu' host, and an
> outstanding change which affects non-XML 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)
>
> This is as from commit 5cd63fda035d.  Up to 5cd63fda035d^ instead I get:
>
> [...]
> Process .../gdb/testsuite/outputs/gdb.base/advance/advance created; pid = 30044
> Listening on port 2346
> target remote [...]:2346
> Remote debugging using [...]:2346
> Reading symbols from .../lib64/ld.so.1...done.
> 0x000000fff79534f0 in __start () from .../lib64/ld.so.1
> (gdb) continue
> Continuing.
> warning: Could not load shared library symbols for linux-vdso.so.1.
> Do you need "set solib-search-path" or "set sysroot"?
>
> Breakpoint 1, main () at .../gdb/testsuite/gdb.base/advance.c:41
> 41  c = 5;
> (gdb)
>
> At the protocol level the difference starts here (bad):
>
> Sending packet: $qfThreadInfo#bb...Ack
> Packet received: m1814
> Sending packet: $qsThreadInfo#c8...Ack
> Packet received: l
> [Switching to Thread <main>]
> Sending packet: $qSymbol::#5b...Ack
> Packet received: qSymbol:6e70746c5f76657273696f6e
> Sending packet: $qSymbol::6e70746c5f76657273696f6e#4d...Ack
> Packet received: OK
> Sending packet: $Hg1814#7d...Ack
> Packet received: OK
> Sending packet: $Hg0#df...Ack
> Packet received: E01
> (gdb) continue
> Cannot execute this command without a live selected thread.
> (gdb)
>
> vs (good):
>
> Sending packet: $qfThreadInfo#bb...Ack
> Packet received: m154d
> Sending packet: $qsThreadInfo#c8...Ack
> Packet received: l
> Sending packet: $mfff726c4f0,4#cb...Ack
> Packet received: 03e0c825
> Sending packet: $mfff726c4ec,4#fd...Ack
> Packet received: 00000000
> Sending packet: $mfff726c4f0,4#cb...Ack
> Packet received: 03e0c825
> 0x000000fff726c4f0 in __start () from .../lib64/ld.so.1
> Sending packet: $qSymbol::#5b...Ack
> Packet received: qSymbol:6e70746c5f76657273696f6e
> Sending packet: $qSymbol::6e70746c5f76657273696f6e#4d...Ack
> Packet received: OK
> (gdb) continue
> Continuing.
> Sending packet: $mfff726c4f0,4#cb...Ack
> Packet received: 03e0c825
> Sending packet: $Z0,120000d6c,4#36...Ack
> Packet received:
> Packet Z0 (software-breakpoint) is NOT supported
> [...]
>
> The version of `gdbserver' causing this regression is as at commit
> f8b73d13b7ca^, the last MIPS backend version with no XML support.  It's
> likely that later versions hit this regression too, as the mode of failure
> is clearly not XML-related.

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

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

macro@linux-mips.org
https://sourceware.org/bugzilla/show_bug.cgi?id=22597

--- Comment #1 from Maciej W. Rozycki <[hidden email]> ---
As investigated here:

<https://sourceware.org/ml/gdb-patches/2017-12/msg00299.html>

the problem comes from the use of a `Hg0' packet which isn't supported
by older stubs that support the general `Hg' form.  Given that `Hg1814'
has been previously issued there's no point in following up with `Hg0'.

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug remote/22597] Regression: backwards compatibility with old GDBservers broken

macro@linux-mips.org
In reply to this post by macro@linux-mips.org
https://sourceware.org/bugzilla/show_bug.cgi?id=22597

Pedro Alves <palves at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Empty `qsThreadInfo' reply  |Regression: backwards
                   |handling regression causing |compatibility with old
                   |inability to execute        |GDBservers broken

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug remote/22597] Regression: backwards compatibility with old GDBservers broken

macro@linux-mips.org
In reply to this post by macro@linux-mips.org
https://sourceware.org/bugzilla/show_bug.cgi?id=22597

--- Comment #2 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Pedro Alves <[hidden email]>:

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

commit 3cada74087687907311b52781354ff551e10a0ed
Author: Pedro Alves <[hidden email]>
Date:   Thu Jan 11 00:23:04 2018 +0000

    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  <[hidden email]>

        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  <[hidden email]>

        PR remote/22597
        * gdb.server/stop-reply-no-thread.c: New file.
        * gdb.server/stop-reply-no-thread.exp: New file.

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug remote/22597] Regression: backwards compatibility with old GDBservers broken

macro@linux-mips.org
In reply to this post by macro@linux-mips.org
https://sourceware.org/bugzilla/show_bug.cgi?id=22597

--- Comment #3 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The gdb-8.1-branch branch has been updated by Pedro Alves
<[hidden email]>:

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

commit 936a312c04f9f1dd856571bf7573b5a8f51ed895
Author: Pedro Alves <[hidden email]>
Date:   Thu Jan 11 00:24:59 2018 +0000

    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  <[hidden email]>

        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  <[hidden email]>

        PR remote/22597
        * gdb.server/stop-reply-no-thread.c: New file.
        * gdb.server/stop-reply-no-thread.exp: New file.

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug remote/22597] Regression: backwards compatibility with old GDBservers broken

macro@linux-mips.org
In reply to this post by macro@linux-mips.org
https://sourceware.org/bugzilla/show_bug.cgi?id=22597

Pedro Alves <palves at redhat dot com> changed:

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

--- Comment #4 from Pedro Alves <palves at redhat dot com> ---
Fix merged.

--
You are receiving this mail because:
You are on the CC list for the bug.
Reply | Threaded
Open this post in threaded view
|

[Bug remote/22597] Regression: backwards compatibility with old GDBservers broken

macro@linux-mips.org
In reply to this post by macro@linux-mips.org
https://sourceware.org/bugzilla/show_bug.cgi?id=22597

Jeremi <jeremip11 at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jeremip11 at gmail dot com

--
You are receiving this mail because:
You are on the CC list for the bug.