[Lldb-commits] [PATCH] D62221: [lldb-server][LLGS] Support 'g' packets

[Pavel Labath via Phabricator via lldb-commits]

labath added a subscriber: mgorny.
labath added a comment.

I think it would be good to split this patch into two:

- implementing `g` packet in lldb-server
- deciding when lldb sends the `g` packet

For the first part, I don't see any reason why lldb-server should *not* support the `g` packet.

The second part, as others have pointed out is a bit more tricky, as the `g` packet may not always be a win. For that, it would be nice to know more about your use case. When you say "all registers are being fetched every step", do you mean that lldb does that on its own, or that you (as in, something external to lldb) for whatever reason want to have all registers read out after each step?

If it's the first thing, then we can probably do something even better, and make sure that lldb-server sends the required registers directly in the stop-reply packet.

If it's the second thing then we can play around with the conditions under which lldb can use the `g` packet. Eg. it definitely sounds like `register read --all` would benefit from this packet, but maybe `register read $specific_register` might not. Or this may not even matter, as for the most common values of `$specific_register`, we should have the data available from the stop-reply packets, and others aren't really used that often...



================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteGPacket.py:14
 
-    def run_test_g_packet(self):
+    def run_test_g_packet(self, read, write):
         self.build()
----------------
This test is pretty weak, as all it does is check that "some" data was received. It would be much better to implement something similar to what @mgorny did in <https://reviews.llvm.org/D61072> and other patches, only at lldb-server level. I.e., have an inferior which sets as many registers as possible to known values, and then have the test assert that. There should already be some code which parses `qRegisterInfo` responses in the test suite somewhere, so all you'd need is to fetch the offsets from there, and check that the values are indeed correct.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1912-1954
+  NativeRegisterContext &reg_ctx = thread->GetRegisterContext();
+
+  // As the register offsets are not necessarily sorted,
+  // use a map to store all registers data.
+  std::map<uint32_t, uint8_t> regs_buffer;
+  for (uint32_t reg_num = 0; reg_num < reg_ctx.GetUserRegisterCount();
+       ++reg_num) {
----------------
There's a `NativeRegisterContext::ReadAllRegisterValues` function. Can you check if that returns the data in the format that you need here?

Even if it doesn't, there's a good chance we could tweak it so that it does, as I believe it's now only used for implementing QSave/RestoreRegisterState, and the format we use for saving that is completely up to us.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62221