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