[Lldb-commits] [PATCH] D89124: [lldb-server][linux] Add ability to allocate memory

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1400
+                            .ToError())
+    return std::move(Err);
+
----------------
Is it possible that the page we find is executable but not writeable and is this part intended to handle that?
(maybe this is a silly question, the code had to be written into that page in the first place I guess)


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1404
+
+  int req = SupportHardwareSingleStepping() ? PTRACE_SINGLESTEP : PTRACE_CONT;
+  if (llvm::Error Err =
----------------
Could you add a comment here explaining this? Like "either we single step just syscall, or continue then hit a trap instruction after the syscall"
(if I have that right)

The description for PTRACE_SINGLESTEP confused me a bit with "next entry to":
> Restart the stopped tracee as for PTRACE_CONT, but arrange for
> the tracee to be stopped at the next entry to or exit from a
> system call, or after execution of a single instruction,
> respectively.

But I assume what happens here is you step "syscall" and then the whole mmap call happens, then you come back. Instead of getting kept before the mmap actually happens.




================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1419
+  uint64_t result = reg_ctx.ReadRegisterAsUnsigned(syscall_data.Result, -ESRCH);
+  uint64_t errno_threshold =
+      (uint64_t(-1) >> (64 - 8 * m_arch.GetAddressByteSize())) - 0x1000;
----------------
What does this calculation do/mean? (0x1000 is 4k which is a page size maybe?)


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1421
+      (uint64_t(-1) >> (64 - 8 * m_arch.GetAddressByteSize())) - 0x1000;
+  if (result > errno_threshold) {
+    return llvm::errorCodeToError(
----------------
For these ifs, are you putting {} because the return is split over two lines? I think it would compile without but is this one of the exceptions to the coding standard? (if clang-format isn't doing this for you that is)


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:1249
+  case llvm::Triple::x86:
+    return MmapData{192, 91};
+  case llvm::Triple::x86_64:
----------------
What's the source for these numbers?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2346
+
+  const lldb::addr_t size = packet.GetHexMaxU64(false, LLDB_INVALID_ADDRESS);
+  if (size == LLDB_INVALID_ADDRESS)
----------------
(assuming this packet type is supported by GDB already) Does it also use hex for the size field? I ask because I came across some file loading packets that were hex for lldb, int for gdb.

Obviously we don't have a hard requirement to match but if we're adding something new might as well.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2366
+    }
+  }
+
----------------
Is no permissions here a valid packet? (as long as mmap doesn't mind I guess it is)


================
Comment at: lldb/test/API/lang/c/stepping/TestStepAndBreakpoints.py:23
     @expectedFailureAll(oslist=['freebsd'], bugnumber='llvm.org/pr17932')
-    @expectedFailureAll(oslist=["linux"], bugnumber="llvm.org/pr14437")
     @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24777")
----------------
This test now passes *because* we have the ability to allocate memory, correct? (I thought it was a stray change at first)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89124