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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
