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

Mon Oct 12 02:45:00 PDT 2020

labath added inline comments.

================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1354
+  auto region_it = llvm::find_if(m_mem_region_cache, [](const auto &pair) {
+    return pair.first.GetExecutable() == MemoryRegionInfo::eYes;
+  });
----------------
jankratochvil wrote:
> Finding arbitrary address would not be safe if LLDB supports [[ https://lldb.llvm.org/status/projects.html#non-stop-debugging | non-stop mode if implemented ]]. It also makes the address a bit non-deterministic. IIRC GDB is using executable's e_entry (="_start" if it exists). But I do not have any strong reason for that.
> 
Lldb also uses the program entry point for the "call mmap(...)" approach of allocating memory. That is a pretty good choice when one needs to find a line of code that will not be executed during normal program operation (not stop or otherwise). However:
- here we don't need that requirement (for all-stop mode, anyway), as we're not calling any function (even mmap can execute a fair amount of code). We're just executing a single instruction.
- technically, there's nothing preventing the application from unmapping the entry point address, or reusing it for something else. If I wanted a foolproof solution, I'd still need to implement a fallback algorithm (as well as the code to search for the entry point, as we right now don't have anything that lldb-server could use).

Given that, it seems better to just go for the more complete solution straight away. It's true that this makes the address harder to predict (I'm trying not to use the word nondeterministic, because that's not really it), but such is life. And coming up with a non-stop-compatible solution for this is so complicated that I'd leave this for some other time (worst case: we disable this feature, or force a temporary stop of all threads).

================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1400
+                            .ToError())
+    return std::move(Err);
+
----------------
DavidSpickett wrote:
> 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)
Ptrace bypasses normal write-protection checks (~all code is not writable these days, and we wouldn't be able to set breakpoints otherwise), so checking for writable addresses would not be useful.

================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1404
+
+  int req = SupportHardwareSingleStepping() ? PTRACE_SINGLESTEP : PTRACE_CONT;
+  if (llvm::Error Err =
----------------
DavidSpickett wrote:
> 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.
> 
> 
Yes, that's exactly what happens. Ptrace can't step "into" the kernel. The reason that description is confusing is because it a description of both PTRACE_SINGLESTEP *and* PTRACE_SYSCALL, with the latter stopping before the syscall.

I've considered using a sequence of two PTRACE_SYSCALLs to avoid the trap instruction requirement, but it wasn't clear to me that this would help in any way (and it would make the code longer).

================
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;
----------------
DavidSpickett wrote:
> What does this calculation do/mean? (0x1000 is 4k which is a page size maybe?)
This is the linux syscall convention for returning errors. The fact that it matches the page size is probably not accidental, though it was not strictly necessary.

I've added a short comment to elaborate.

================
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(
----------------
DavidSpickett wrote:
> 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)
clang-format never adds new tokens -- it just reformats existing ones.

Regarding the coding standard, this is in a fairly grey area. I could invoke the "braces should be used when a single-statement body is complex enough that it becomes difficult to see where the block containing the following statement began" rule and say this statement is "complex enough" (I think it was even more complex when I've first written it). I'm sure some people wouldn't consider it complex, but I think it's better to err on the side of adding more braces (I generally add braces for all multi-line statements).

================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1444
+    prot |= PROT_EXEC;
+
+  llvm::Expected<uint64_t> Result =
----------------
jankratochvil wrote:
> Some sanity check 'permissions' does not have set a bit which NativeProcessLinux::AllocateMemory does not understand?
A new permission flag seems fairly unlikely, but sure, why not..

================
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:
----------------
DavidSpickett wrote:
> What's the source for these numbers?
There are plenty of ways to find this -- internet is full of linux system call tables. I just preprocessed a file referencing SYS_MMAP/MUNMAP constants. :P

I also added a note about the file they can be found in (/usr/include/asm/unistd.h)

================
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)
----------------
DavidSpickett wrote:
> (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.
This isn't adding a completely new packet (you'll notice the patch does not include any clientside changes). This packet is already supported by debugserver, and I believe it is an lldb addition -- I did not come across anything like it in the gdb docs.

However, I would be interested to hear about the protocol mismatches you've found.

================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2366
+    }
+  }
+
----------------
DavidSpickett wrote:
> Is no permissions here a valid packet? (as long as mmap doesn't mind I guess it is)
Yeah, you can do that in theory -- permissions can be later changed with mprotect(2). Lldb does not make any use of that though.

================
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")
----------------
DavidSpickett wrote:
> DavidSpickett wrote:
> > This test now passes *because* we have the ability to allocate memory, correct? (I thought it was a stray change at first)
> Actually shouldn't this be an expected failure on non x86 linux?
This is a funny one. The reason this test fails is because of the stop-id check on line 148 (IIRC). Without this feature lldb will allocate memory by finding and calling the mmap libc function. This seems to mess up the "expression stop id" counter. So it's a real bug, but not one that is likely to cause problems for most people.

However, you're right that this "bug" is not "fixed" only on x86.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89124