[Lldb-commits] [PATCH] D89227: [lldb] Note difference in vFile:pread/pwrite format for lldb

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

In D89227#2327443 <https://reviews.llvm.org/D89227#2327443>, @labath wrote:

> In D89227#2327068 <https://reviews.llvm.org/D89227#2327068>, @DavidSpickett wrote:
>
>> And I'm saying lldb-server and not that and debug-server because if I'm honest, I don't really know what the latter is. I assume this applies to both though.
>
> debugserver is an apple-only implementation of the remote stub. However, I think that may be a red herring, because in lldb land the vFile packets are handled by lldb-server platform, which is used even on apple platforms (IIUC).

On the Darwin systems we're using a standalone "lldb-platform" I wrote a coupe of years ago, I had a bit of trouble getting lldb-server to work with the way we communicate between iOS devices and macs and was annoyed enough that a simple agent was causing me so much headache, so I wrote a freestanding implementation of the platform packets.  It doesn't use any llvm/lldb so it's never been clear if it made sense to upstream.  I wrote it with the primary design goal being simplicity and no thought given to performance, so I'm afraid upstreaming it would result in people improving the performance and reducing the simplicity, or integrating llvm/lldb classes, and that's not really what I wanted here.

(I think there's a real value from having a separate implementation of the protocol packets as well, that's why I took the time to write the platform protocol doc after writing the server).

In this case, my implementation probably does the wrong thing --

  int file_fd;
  if (ascii_to_i (file_fd_string, 10, file_fd) == false)
      return send_packet (m_fd, "E01");
  
  int offset;
  if (ascii_to_i (offset_string, 10, offset) == false)
      return send_packet (m_fd, "E01");
  
  std::string bytes = binary_unescape_data (encoded_contents);

(which calls strtoul with the base parameter - I don't know what stroul would do with "0x100" and base 10).

FWIW this is our 3rd protocol bug that I know of.

The A packet https://bugs.llvm.org/show_bug.cgi?id=42471  (another base10 vrs base16 mixup)
The vFile:pread / vFile:pwrite packet https://bugs.llvm.org/show_bug.cgi?id=47820
The vFile:open flags where lldb uses enum OpenOptions  and gdb uses https://sourceware.org/gdb/onlinedocs/gdb/Open-Flags.html#Open-Flags

We've discussed how to transition away from the current buggy behavior with these before, I think Pavel and I have been converging on using the qSupported packet at the start of the connection.  e.g. at the start of a session, lldb sends:

qSupported:xmlRegisters=i386,arm,mips,arc

and the remote stub replies:

qXfer:features:read+;PacketSize=20000;qEcho+

( https://www.sourceware.org/gdb/onlinedocs/gdb/General-Query-Packets.html#qSupported )

lldb can add stubfeatures on to the qSupported packet like

qSupported: xmlRegisters=i386,arm,mips,arc;a-packet-base16;vfilep-base16;vfileopen-flags-gdb-compat;

and the response can include a-packet-base16+; vfilep-base16+; vfileopen-flags-gdb-compat+;

After some undefined transition period :) we change lldb to assume the correct gdb-compatible behavior.

The biggest hurdle for changing the default are iOS/watchOS devices out in the field - we cannot update the old debugservers that run on them, so we often need lldb to communicate with debugservers that are 4-5 years old (I forget the exact OS matrix for what Xcode supports for debugging, but it goes back a ways).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89227/new/

https://reviews.llvm.org/D89227