search

Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

LLDB debugserver - GDB remote protocol 'A' packet format is not spec-compliant #41441

Open Quuxplusone opened 5 years ago

Quuxplusone commented 5 years ago
Bugzilla Link PR42471
Status NEW
Importance P normal
Reported by Spencer Michaels (spmichaels.work@gmail.com)
Reported on 2019-07-01 12:54:45 -0700
Last modified on 2021-11-16 03:30:44 -0800
Version 8.0
Hardware All All
CC jdevlieghere@apple.com, llvm-bugs@lists.llvm.org, llvm-bugzilla-mail@molenda.com, ramana.venkat83@gmail.com, spmichaels.work@gmail.com
Fixed by commit(s)
Attachments patch.diff (919 bytes, text/plain)
Blocks
Blocked by
See also

LLDB's GDB remote protocol implementation defines a parser for the 'A' packet at tools/debugger/source/RNBRemote.cpp:1538 (i.e. RNBRemote::HandlePacket_A()). This packet is used to pass an argv[] array to the program.

GDB's remote protocol spec defines the A packet as follows (see (https://sourceware.org/gdb/onlinedocs/gdb/Packets.html):

‘A arglen,argnum,arg,…’

Initialized argv[] array passed into program. arglen specifies the number of bytes in the hex encoded byte stream arg. See gdbserver for more details.

Note that gdbserver does not actually implement the A packet (see https://github.com/bminor/binutils-gdb/blob/master/gdb/gdbserver/server.c), so the note to "See gdbserver for more details" is moot.

LLDB's implementation assumes that 'arglen' and 'argnum' are base-10 unsigned integers. However, the GDB remote protocol overview specifies that (see https://sourceware.org/gdb/onlinedocs/gdb/Overview.html#Overview):

Except where otherwise noted all numbers are represented in HEX with leading zeros suppressed.

Since the 'A' packet definition does not explicitly specify a base for arglen and argnum, thesei should actually be base-16, not base-10 as they are now.

This would require changes to the two strtoul() calls on lines 1562 and 1574 of RNBRemote.cpp.

Quuxplusone commented 5 years ago

Attached patch.diff (919 bytes, text/plain): Proposed patch, v1

Quuxplusone commented 5 years ago

Hi Michael, good job spotting this.

Unfortunately we need to interoperate with existing implementations that expect base 10 numbers for at least a few years. We'll need to add a new packet to specify which base debugserver should expect during a transition period, and at some point we can switch debugserver. Before that, we'll need to keep lldb/debugserver/lldb-server using

lldb sends this packet in GDBRemoteCommunicationClient::SendArgumentsPacket and we have ANOTHER remote serial protocol stub called lldb-server (used on linux) that has the same behavior in GDBRemoteCommunicationServerCommon::Handle_A both of those in sources/Plugins/Process/gdb-remote/.

I found a similar problem with the vFile:open: packet last winter, there are constant values passed to indicate the read/write flags passed to open() but the values documented at https://sourceware.org/gdb/onlinedocs/gdb/Open-Flags.html#Open-Flags do not match the values lldb uses in enum OpenOptions. I'm going to add a QUseGDBFlagsInvFileOpen packet to debugserver/lldb-server. When the remote stub recognizes this QUseGDBFlagsInvFileOpen packet (sends back OK), then lldb will use the correct (gdb) constant values. And after a few years, I'll change lldb to send the gdb constant values all the time.

Quuxplusone commented 5 years ago

Er sorry, thanks Spencer! :)

Quuxplusone commented 5 years ago 
Hm, another possibilty is to call get the version of the remote serial protocol
stub (qGDBServerVersion packet) which will come back with a reply like
"name:debugserver;version:1100;".  It doesn't look like lldb-server implements
this packet yet, but it would be easy to add.  This would tell you
unambiguously if you are communicating with a debugserver/lldb-server.  Then
you can add a packet like QUseGDBBaseInAPacket to tell the remote stub that
we're using the base16 numbers, and if that comes back as recognized, then use
base 16.

If we get back a name:debugserver and it DOESN'T understand
QUseGDBBaseInAPacket, then we have to send base10.

If qGDBServerVersion is unsupported or comes back with a name !=
debugserver/lldb-server, we can use the gdb base16 numbers.
Quuxplusone commented 5 years ago 
That makes sense. Perhaps it would be better to have a generic packet that
works a bit like "qSupported" to allow the client and server to notify each
other that they implement fixes for LLDB's divergences from the GDB spec, e.g.

----------

Request: "qProtocolFixes:fix;…" where each 'fix' is one of the following
strings:
  - "GDBFlagsInvFileOpen"
  - "GDBBaseInAPacket"
  - ...
Unknown strings are acceptable, but ignored. If a fix  string is not present,
it is assumed that that fix is not present.

Response: "fix;…", same definition as 'fix' above.

----------

This would keep the number of new packets to a minimum and allow for, I think,
seamless future expansion if additional divergences from the GDB spec are
discovered.
Quuxplusone commented 5 years ago

Nice suggestion. I'm wondering about calling it qPacketVersions instead of qProtocolFixes -- so when we want to change the behavior of a packet, we can call the existing version "1" and the next version "2". We add additional information from packets all the time and rarely need to change their behavior in an incompatible way, but it's something that could be useful in the future. Might be worth raising the idea on lldb-dev mailing list, not sure if everyone watches the bugzilla traffic.