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