[Lldb-commits] [PATCH] D74398: [lldb-server] jThreadsInfo returns stack memory

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks. Could you also add the other kind of test (the one inline asm) I mentioned. In an ideal world we'd have a test case for every boundary condition, but we're pretty far from that right now. Even so, one test case like that would be nice.

I am imagining the inferior doing something like:

  movabsq $0xdead, %rax
  pushq %rax ; fake pc
  leaq 0x1000(%rsp), %rbp ; larger than kMaxFrameSize
  pushq %rbp
  movq %rsp, %rbp
  pushq $1 ; fake frame contents
  pushq $2
  pushq $3
  
  incq %rax
  push %rax; second fake pc
  pushq %rbp
  movq %rsp, %rbp
  pushq $4 ; fake frame contents
  pushq $5
  pushq $6
  int3

and then the test would assert that the result contains the entirety of the first fake frame, the bp+pc of the second fake frame, and then would stop due to hitting the kMaxFrameSize boundary.



================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteThreadsInStopReply.py:373
+        # Read memory chunks from jThreadsInfo.
+        memory_chunks = self.gather_threads_info_memory()
+        # Check the chunks are correct.
----------------
Also assert that you have at least one chunk here?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:501-504
+static void AddMemoryChunk(json::Array &stack_memory_chunks, addr_t address,
+                           std::vector<int8_t> &bytes) {
+  if (bytes.empty())
+    return;
----------------
I think it would be cleaner if this returned the json::Object as a return value (much like `GetStackMemoryAsJSON` returns a json::Array).


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:534
+  } else {
+    return stack_memory_chunks;
+  }
----------------
You will need to "handle" the error inside the Expected object here. The object asserts (at runtime) that you do that at the right time. (http://llvm.org/docs/ProgrammersManual.html#recoverable-errors).

That said, if you're not going to do anything with the error, then maybe the GetRegisterValue doesn't really need to return an Expected (maybe Optional<RegisterValue>)


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:548
+                               static_cast<size_t>(fp_value - sp_value));
+  std::vector<int8_t> buf(byte_count, 0);
+
----------------
we usually use arrays of *u*int8_t to represent uninterpreted data.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:578
+    lldb_private::DataExtractor extractor(fp_ra_buf.data(), fp_and_ra_size,
+                                          endian::InlHostByteOrder(),
+                                          address_size);
----------------
It is probably going to be the same, but you might as well use `process.GetArchitecture().GetByteOrder()`, since you have it handy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74398