[Lldb-commits] [PATCH] D62221: [lldb-server][LLGS] Support 'g' packets
Pavel Labath via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Tue May 28 00:08:00 PDT 2019
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
+ def test_g_returns_correct_data_with_suffix_llgs(self):
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
- if (packet_size == 1)
- return eServerPacketType_g;
> 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).
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the lldb-commits