search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.97k stars 11.94k forks source link

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

Open llvmbot opened 5 years ago

llvmbot commented 5 years ago
Bugzilla Link 42471
Version 8.0
OS All
Attachments Proposed patch, v1
Reporter LLVM Bugzilla Contributor
CC @JDevlieghere,@jasonmolenda,@RamNalamothu

Extended Description

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.

jasonmolenda 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.

llvmbot 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:

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.

jasonmolenda 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.

jasonmolenda commented 5 years ago

Er sorry, thanks Spencer! :)

jasonmolenda 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.