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