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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
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()
----------------
guiandrade wrote:
> @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?
Yes, that looks pretty much like what I had in mind. About the inferior, what I think we should do here is move this test into a separate subfolder. Then the test can have it's own Makefile, cpp file, etc..

If you need to share some code between this test and the new one, you can put the common stuff into `lldbgdbserverutils.py` or `gdbremote_testcase.py`. Or, if the only two tests using the common code are this one and the `p` test below, then maybe you can move the `p` test too, so that both register reading tests live together.


================
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) {
----------------
guiandrade wrote:
> 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?
Ok, I think i see what ReadAllRegisterValues is doing. Let's keep the implementation here then


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1916
+  // 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();
----------------
You could just  use a `std::vector<uint8_t>`, and memcpy data of each register into it (resizing as needed). Using a `std::map` for this thing seems pretty wasteful.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1946-1954
+  uint32_t next_pos = 0;
+  for (const auto &it : regs_buffer) {
+    // Populate the gap between the last and the current position
+    // with zeroes.
+    while (next_pos++ < it.first)
+      response.PutHex8(0);
+
----------------
Then this would just be `response.PutBytesAsRawHex(vector.data(), vector.size())`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62221