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

[Pavel Labath via Phabricator via lldb-commits]

labath added a subscriber: omjavaid.
labath added a comment.

I have a couple of comments inline, but overall I think this looks pretty good now.

It would be interesting to also test reading %ymm registers, as those are stored in a funny way in the cpu context (and so we are most likely to get them wrong), however, I don't think this has to be done now.



================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-server/register-reading/TestGdbRemoteGPacket.py:140-147
+    @llgs_test
+    def test_g_returns_correct_data_with_suffix_llgs(self):
+        self.init_llgs_test()
+        self.build()
+        self.set_inferior_startup_launch()
+        self.g_returns_correct_data(True)
+
----------------
Right now, these tests will only work on x86_64, so you'll need to add something like `@skipIf(archs=no_match(["x86_64"]))`.

 + @omjavaid in case he's interested in contributing an arm version of these.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1936-1937
+    // Write the register data to the buffer.
+    const uint8_t *const data =
+        reinterpret_cast<const uint8_t *>(reg_value.GetBytes());
+
----------------
It doesn't look like this `reinterpret_cast` is needed. You should be able to feed `reg_value.GetBytes()` directly to `memcpy`...


================
Comment at: lldb/source/Utility/StringExtractorGDBRemote.cpp:379-382
   case 'g':
-    if (packet_size == 1)
-      return eServerPacketType_g;
-    break;
----------------
guiandrade wrote:
> 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?
I think this is fine because *we* don't support any other packet starting with `g`. So, we can just classify that packet as `g`, and later fail due to a syntax error. If we end up supporting another packet like that, we can refine the classification logic then.

This is also the same pattern used by other single-letter packets. (eg. below).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62221