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

[Guilherme Andrade via Phabricator via lldb-commits]

guiandrade added a comment.

Thank you so much for the feedback!

In D62221#1511035 <https://reviews.llvm.org/D62221#1511035>, @clayborg wrote:

> Using 'g' packets seems fine to me and we might be able to just enable it. We will need to ensure that this doesn't regress stepping times so we will need to get people to try this out on their systems first. Most of our targets expedite enough registers in the stop reply packet where we don't need to read that many registers. Are you using lldb-server as your GDB remote binary or another GDB server?


I am using lldb-server, @clayborg.

In D62221#1511499 <https://reviews.llvm.org/D62221#1511499>, @labath wrote:

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


@labath, I have a debug engine that populates a UI responsible for displaying register contents, hence I end up having to fetch all of them every step if the user decides to leave it on. I understand that using `g` packets isn't the best way to go every time, but I think if we can find a nice way to decide when to use them, it would be a win. So I will separate this patch into two as you suggested to make it easier to discuss each part.



================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestLldbGdbServer.py:585
+
+    def g_returns_correct_data(self):
+        procs = self.prep_debug_monitor_and_inferior()
----------------
@labath, is it something like this you have in mind? If it is, where should we add the file that contains the inferior that sets the registers?


================
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) {
----------------
labath wrote:
> 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.
Apparently it is not:

[[ https://pastebin.com/raw/zFRQguQP | Reading each register individually ]] 
[[ https://pastebin.com/raw/t8qJAJAE | Using reg_ctx.ReadAllRegisterValues(.) ]].

Yeah, it would be convenient to be able to reuse that function, but then wouldn't that involve modifying several ReadAllRegisterValues implementations in order to do so?


================
Comment at: lldb/source/Utility/StringExtractorGDBRemote.cpp:379-382
   case 'g':
-    if (packet_size == 1)
-      return eServerPacketType_g;
-    break;
----------------
clayborg wrote:
> What if another packet starts with 'g'?
I removed the if because I thought the packet could have the thread id appended to it in [[ https://github.com/llvm/llvm-project/blob/1f46d524a1c6ac66bbd803caf5f1206603759a5f/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp#L535   | ldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp#L535 ]]. If that's really the case, should we still try to make it more robust to avoid prefix collisions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62221