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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It looks like the windows lldb-server implementation does not support all 
permission combinations (`ConvertLldbToWinApiProtect` in 
`ProcessDebugger.cpp`). I've xfailed the test for now -- I'll try to add the 
missing bits tomorrow.

I've also messed up the decorators on one of the tests.

a1ab2b77  
should get that green again, I hope.


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

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

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova added a comment.

It looks like this changed caused a failure on the windows bot and your follow 
up change to fix the test left it still broken. Can you have a look?


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

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

[Pavel Labath via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2c4226f8ac2c: [lldb-server][linux] Add ability to allocate 
memory (authored by labath).

Changed prior to commit:
  https://reviews.llvm.org/D89124?vs=297876=298134#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89124

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
  lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.h
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
  lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
  lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h
  lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp
  lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/test/API/lang/c/stepping/TestStepAndBreakpoints.py
  lldb/test/API/tools/lldb-server/memory-allocation/Makefile
  
lldb/test/API/tools/lldb-server/memory-allocation/TestGdbRemoteMemoryAllocation.py
  lldb/test/API/tools/lldb-server/memory-allocation/main.c
  lldb/test/Shell/Expr/nodefaultlib.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -41,9 +41,6 @@
   MOCK_METHOD0(Detach, Status());
   MOCK_METHOD1(Signal, Status(int Signo));
   MOCK_METHOD0(Kill, Status());
-  MOCK_METHOD3(AllocateMemory,
-   Status(size_t Size, uint32_t Permissions, addr_t ));
-  MOCK_METHOD1(DeallocateMemory, Status(addr_t Addr));
   MOCK_METHOD0(GetSharedLibraryInfoAddress, addr_t());
   MOCK_METHOD0(UpdateThreads, size_t());
   MOCK_CONST_METHOD0(GetAuxvData,
@@ -147,4 +144,4 @@
 };
 } // namespace lldb_private
 
-#endif
\ No newline at end of file
+#endif
Index: lldb/test/Shell/Expr/nodefaultlib.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/nodefaultlib.cpp
@@ -0,0 +1,16 @@
+// Test that we're able to evaluate expressions in inferiors without the
+// standard library (and mmap-like functions in particular).
+
+// REQUIRES: native
+// XFAIL: system-linux && !(target-x86 || target-x86_64)
+// XFAIL: system-netbsd || system-freebsd
+
+// RUN: %build %s --nodefaultlib -o %t
+// RUN: %lldb %t -o "b main" -o run -o "p call_me(5, 6)" -o exit \
+// RUN:   | FileCheck %s
+
+// CHECK: (int) $0 = 30
+
+int call_me(int x, long y) { return x * y; }
+
+int main() { return call_me(4, 5); }
Index: lldb/test/API/tools/lldb-server/memory-allocation/main.c
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/memory-allocation/main.c
@@ -0,0 +1 @@
+int main() { return 0; }
Index: lldb/test/API/tools/lldb-server/memory-allocation/TestGdbRemoteMemoryAllocation.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/memory-allocation/TestGdbRemoteMemoryAllocation.py
@@ -0,0 +1,101 @@
+
+import gdbremote_testcase
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+supported_linux_archs = ["x86_64", "i386"]
+supported_oses = ["linux"]
+
+class TestGdbRemoteMemoryAllocation(gdbremote_testcase.GdbRemoteTestCaseBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def allocate(self, size, permissions):
+self.test_sequence.add_log_lines(["read packet: $_M{:x},{}#00".format(size, permissions),
+  {"direction": "send",
+   "regex":
+   r"^\$([0-9a-f]+)#[0-9a-fA-F]{2}$",
+   "capture": {
+   1: "addr"}},
+  ],
+ True)
+context = self.expect_gdbremote_sequence()
+self.assertIsNotNone(context)
+
+addr = int(context.get("addr"), 16)
+self.test_sequence.add_log_lines(["read packet: $qMemoryRegionInfo:{:x}#00".format(addr),
+  {"direction": "send",
+

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

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 297876.
labath added a comment.

- GetCurrentThread
- const


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89124

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
  lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.h
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
  lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
  lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/test/API/lang/c/stepping/TestStepAndBreakpoints.py
  lldb/test/API/tools/lldb-server/memory-allocation/Makefile
  
lldb/test/API/tools/lldb-server/memory-allocation/TestGdbRemoteMemoryAllocation.py
  lldb/test/API/tools/lldb-server/memory-allocation/main.c
  lldb/test/Shell/Expr/nodefaultlib.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -41,9 +41,6 @@
   MOCK_METHOD0(Detach, Status());
   MOCK_METHOD1(Signal, Status(int Signo));
   MOCK_METHOD0(Kill, Status());
-  MOCK_METHOD3(AllocateMemory,
-   Status(size_t Size, uint32_t Permissions, addr_t ));
-  MOCK_METHOD1(DeallocateMemory, Status(addr_t Addr));
   MOCK_METHOD0(GetSharedLibraryInfoAddress, addr_t());
   MOCK_METHOD0(UpdateThreads, size_t());
   MOCK_CONST_METHOD0(GetAuxvData,
@@ -147,4 +144,4 @@
 };
 } // namespace lldb_private
 
-#endif
\ No newline at end of file
+#endif
Index: lldb/test/Shell/Expr/nodefaultlib.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/nodefaultlib.cpp
@@ -0,0 +1,16 @@
+// Test that we're able to evaluate expressions in inferiors without the
+// standard library (and mmap-like functions in particular).
+
+// REQUIRES: native
+// XFAIL: system-linux && !(target-x86 || target-x86_64)
+// XFAIL: system-netbsd || system-freebsd
+
+// RUN: %build %s --nodefaultlib -o %t
+// RUN: %lldb %t -o "b main" -o run -o "p call_me(5, 6)" -o exit \
+// RUN:   | FileCheck %s
+
+// CHECK: (int) $0 = 30
+
+int call_me(int x, long y) { return x * y; }
+
+int main() { return call_me(4, 5); }
Index: lldb/test/API/tools/lldb-server/memory-allocation/main.c
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/memory-allocation/main.c
@@ -0,0 +1 @@
+int main() { return 0; }
Index: lldb/test/API/tools/lldb-server/memory-allocation/TestGdbRemoteMemoryAllocation.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/memory-allocation/TestGdbRemoteMemoryAllocation.py
@@ -0,0 +1,101 @@
+
+import gdbremote_testcase
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+supported_linux_archs = ["x86_64", "i386"]
+supported_oses = ["linux"]
+
+class TestGdbRemoteMemoryAllocation(gdbremote_testcase.GdbRemoteTestCaseBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def allocate(self, size, permissions):
+self.test_sequence.add_log_lines(["read packet: $_M{:x},{}#00".format(size, permissions),
+  {"direction": "send",
+   "regex":
+   r"^\$([0-9a-f]+)#[0-9a-fA-F]{2}$",
+   "capture": {
+   1: "addr"}},
+  ],
+ True)
+context = self.expect_gdbremote_sequence()
+self.assertIsNotNone(context)
+
+addr = int(context.get("addr"), 16)
+self.test_sequence.add_log_lines(["read packet: $qMemoryRegionInfo:{:x}#00".format(addr),
+  {"direction": "send",
+   "regex":
+   r"^\$start:([0-9a-fA-F]+);size:([0-9a-fA-F]+);permissions:([rwx]*);.*#[0-9a-fA-F]{2}$",
+   "capture": {
+   1: "addr",
+   2: "size",
+   

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

[Pavel Labath via Phabricator via lldb-commits]

labath marked 12 inline comments as done.
labath added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1362
+
+  NativeThreadLinux  = *GetThreadByID(GetID());
+  assert(thread.GetState() == eStateStopped);

jankratochvil wrote:
> There does not need to exist a thread with TID equal to the process PID. That 
> TID could already exit while other TIDs of that PID may be still running.
> https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=432b4d03ad0f23970315e9f9dec080ab4a9ab94b
> simplified and deGPLed as: https://people.redhat.com/jkratoch/leader-exit2.C
> Just currently LLDB server cannot attach to such TID as:
>   lldb-server g --attach >TID< :1234
>   ptrace(PTRACE_ATTACH, >PID<) = -1 EPERM (Operation not permitted)
>   write(2, "error: failed to attach to pid >TID<: Operation not permitted\n", 
> 64) = 64
> But if this bug was fixed then I think this statement could SEGV (unless LLDB 
> will have some internal zombie TID==PID).
> 
Thanks for catching that. I had a vague feeling that it might be possible for 
the process to outlive its main thread, but I didn't know how. I've changed 
this to `GetCurrentThread` which should be always present.


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

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

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil accepted this revision.
jankratochvil added inline comments.
This revision is now accepted and ready to land.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1362
+
+  NativeThreadLinux  = *GetThreadByID(GetID());
+  assert(thread.GetState() == eStateStopped);

There does not need to exist a thread with TID equal to the process PID. That 
TID could already exit while other TIDs of that PID may be still running.
https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=432b4d03ad0f23970315e9f9dec080ab4a9ab94b
simplified and deGPLed as: https://people.redhat.com/jkratoch/leader-exit2.C
Just currently LLDB server cannot attach to such TID as:
  lldb-server g --attach >TID< :1234
  ptrace(PTRACE_ATTACH, >PID<) = -1 EPERM (Operation not permitted)
  write(2, "error: failed to attach to pid >TID<: Operation not permitted\n", 
64) = 64
But if this bug was fixed then I think this statement could SEGV (unless LLDB 
will have some internal zombie TID==PID).




Comment at: 
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:1237
+  lldb_rdx_x86_64, lldb_r10_x86_64, lldb_r8_x86_64,
+  lldb_r9_x86_64};
+return SyscallData{Syscall, Args, lldb_rax_x86_64};

static const ... (4x)


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

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D89124#2325008 , @jankratochvil 
wrote:

> In file included from 
> /home/jkratoch/redhat/llvm-monorepo/lldb/source/Host/common/NativeProcessProtocol.cpp:9:
> /home/jkratoch/redhat/llvm-monorepo/lldb/include/lldb/Host/common/NativeProcessProtocol.h:20:10:
>  fatal error: 'lldb/Utility/UnimplementedError.h' file not found
> #include "lldb/Utility/UnimplementedError.h"

That's because you need the dependant revision D89121 
  (which is now merged).


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

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

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

In file included from 
/home/jkratoch/redhat/llvm-monorepo/lldb/source/Host/common/NativeProcessProtocol.cpp:9:
/home/jkratoch/redhat/llvm-monorepo/lldb/include/lldb/Host/common/NativeProcessProtocol.h:20:10:
 fatal error: 'lldb/Utility/UnimplementedError.h' file not found
#include "lldb/Utility/UnimplementedError.h"


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

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

[Pavel Labath via Phabricator via lldb-commits]

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 ) {
+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 

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

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 297523.
labath added a comment.

address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89124

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
  lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.h
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
  lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
  lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/test/API/lang/c/stepping/TestStepAndBreakpoints.py
  lldb/test/API/tools/lldb-server/memory-allocation/Makefile
  
lldb/test/API/tools/lldb-server/memory-allocation/TestGdbRemoteMemoryAllocation.py
  lldb/test/API/tools/lldb-server/memory-allocation/main.c
  lldb/test/Shell/Expr/nodefaultlib.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -41,9 +41,6 @@
   MOCK_METHOD0(Detach, Status());
   MOCK_METHOD1(Signal, Status(int Signo));
   MOCK_METHOD0(Kill, Status());
-  MOCK_METHOD3(AllocateMemory,
-   Status(size_t Size, uint32_t Permissions, addr_t ));
-  MOCK_METHOD1(DeallocateMemory, Status(addr_t Addr));
   MOCK_METHOD0(GetSharedLibraryInfoAddress, addr_t());
   MOCK_METHOD0(UpdateThreads, size_t());
   MOCK_CONST_METHOD0(GetAuxvData,
@@ -147,4 +144,4 @@
 };
 } // namespace lldb_private
 
-#endif
\ No newline at end of file
+#endif
Index: lldb/test/Shell/Expr/nodefaultlib.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/nodefaultlib.cpp
@@ -0,0 +1,16 @@
+// Test that we're able to evaluate expressions in inferiors without the
+// standard library (and mmap-like functions in particular).
+
+// REQUIRES: native
+// XFAIL: system-linux && !(target-x86 || target-x86_64)
+// XFAIL: system-netbsd || system-freebsd
+
+// RUN: %build %s --nodefaultlib -o %t
+// RUN: %lldb %t -o "b main" -o run -o "p call_me(5, 6)" -o exit \
+// RUN:   | FileCheck %s
+
+// CHECK: (int) $0 = 30
+
+int call_me(int x, long y) { return x * y; }
+
+int main() { return call_me(4, 5); }
Index: lldb/test/API/tools/lldb-server/memory-allocation/main.c
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/memory-allocation/main.c
@@ -0,0 +1 @@
+int main() { return 0; }
Index: lldb/test/API/tools/lldb-server/memory-allocation/TestGdbRemoteMemoryAllocation.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/memory-allocation/TestGdbRemoteMemoryAllocation.py
@@ -0,0 +1,101 @@
+
+import gdbremote_testcase
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+supported_linux_archs = ["x86_64", "i386"]
+supported_oses = ["linux"]
+
+class TestGdbRemoteMemoryAllocation(gdbremote_testcase.GdbRemoteTestCaseBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def allocate(self, size, permissions):
+self.test_sequence.add_log_lines(["read packet: $_M{:x},{}#00".format(size, permissions),
+  {"direction": "send",
+   "regex":
+   r"^\$([0-9a-f]+)#[0-9a-fA-F]{2}$",
+   "capture": {
+   1: "addr"}},
+  ],
+ True)
+context = self.expect_gdbremote_sequence()
+self.assertIsNotNone(context)
+
+addr = int(context.get("addr"), 16)
+self.test_sequence.add_log_lines(["read packet: $qMemoryRegionInfo:{:x}#00".format(addr),
+  {"direction": "send",
+   "regex":
+   r"^\$start:([0-9a-fA-F]+);size:([0-9a-fA-F]+);permissions:([rwx]*);.*#[0-9a-fA-F]{2}$",
+   "capture": {
+   1: "addr",
+   2: "size",
+  

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

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil 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 ) {
+return pair.first.GetExecutable() == MemoryRegionInfo::eYes;
+  });

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.




Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1444
+prot |= PROT_EXEC;
+
+  llvm::Expected Result =

Some sanity check 'permissions' does not have set a bit which 
NativeProcessLinux::AllocateMemory does not understand?


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

[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/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:
> 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?


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

[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

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1350
 
-Status NativeProcessLinux::AllocateMemory(size_t size, uint32_t permissions,
-  lldb::addr_t ) {
-// FIXME implementing this requires the equivalent of
-// InferiorCallPOSIX::InferiorCallMmap, which depends on functional ThreadPlans
-// working with Native*Protocol.
-#if 1
-  return Status("not implemented yet");
-#else
-  addr = LLDB_INVALID_ADDRESS;
-
-  unsigned prot = 0;
-  if (permissions & lldb::ePermissionsReadable)
-prot |= eMmapProtRead;
-  if (permissions & lldb::ePermissionsWritable)
-prot |= eMmapProtWrite;
-  if (permissions & lldb::ePermissionsExecutable)
-prot |= eMmapProtExec;
-
-  // TODO implement this directly in NativeProcessLinux
-  // (and lift to NativeProcessPOSIX if/when that class is refactored out).
-  if (InferiorCallMmap(this, addr, 0, size, prot,
-   eMmapFlagsAnon | eMmapFlagsPrivate, -1, 0)) {
-m_addr_to_mmap_size[addr] = size;
-return Status();
-  } else {
-addr = LLDB_INVALID_ADDRESS;
-return Status("unable to allocate %" PRIu64
-  " bytes of memory with permissions %s",
-  size, GetPermissionsAsCString(permissions));
+llvm::Expected
+NativeProcessLinux::Syscall(llvm::ArrayRef args) {

How much of this would be useful for BSDs? Should some of this be pushed to the 
base classes?


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

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

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: clayborg, jankratochvil, mgorny.
Herald added subscribers: pengfei, emaste.
Herald added a reviewer: JDevlieghere.
Herald added a project: LLDB.
labath requested review of this revision.

This patch adds support for the _M and _m gdb-remote packets, which
(de)allocate memory in the inferior. This works by "injecting" a
m(un)map syscall into the inferior. This consists of:

- finding an executable page of memory
- writing the syscall opcode to it
- setting up registers according to the os syscall convention
- single stepping over the syscall

The advantage of this approach over calling the mmap function is that
this works even in case the mmap function is buggy or unavailable. The
disadvantage is it is more platform-dependent, which is why this patch
only works on X86 (_32 and _64) right now. Adding support for other
linux architectures should be easy and consist of defining the
appropriate syscall constants. Adding support for other OSes depends on
the its ability to do a similar trick.

Depends on D89121 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89124

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
  lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.h
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
  lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
  lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp
  lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/test/API/lang/c/stepping/TestStepAndBreakpoints.py
  lldb/test/API/tools/lldb-server/memory-allocation/Makefile
  
lldb/test/API/tools/lldb-server/memory-allocation/TestGdbRemoteMemoryAllocation.py
  lldb/test/API/tools/lldb-server/memory-allocation/main.c
  lldb/test/Shell/Expr/nodefaultlib.cpp
  lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h

Index: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
===
--- lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
+++ lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h
@@ -41,9 +41,6 @@
   MOCK_METHOD0(Detach, Status());
   MOCK_METHOD1(Signal, Status(int Signo));
   MOCK_METHOD0(Kill, Status());
-  MOCK_METHOD3(AllocateMemory,
-   Status(size_t Size, uint32_t Permissions, addr_t ));
-  MOCK_METHOD1(DeallocateMemory, Status(addr_t Addr));
   MOCK_METHOD0(GetSharedLibraryInfoAddress, addr_t());
   MOCK_METHOD0(UpdateThreads, size_t());
   MOCK_CONST_METHOD0(GetAuxvData,
@@ -147,4 +144,4 @@
 };
 } // namespace lldb_private
 
-#endif
\ No newline at end of file
+#endif
Index: lldb/test/Shell/Expr/nodefaultlib.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/nodefaultlib.cpp
@@ -0,0 +1,12 @@
+// Test that we're able to evaluate expressions in inferiors without the
+// standard library (and mmap-like functions in particular).
+
+// RUN: %build %s --nodefaultlib -o %t
+// RUN: %lldb %t -o "b main" -o run -o "p call_me(5, 6)" -o exit \
+// RUN:   | FileCheck %s
+
+// CHECK: (int) $0 = 30
+
+int call_me(int x, long y) { return x * y; }
+
+int main() { return call_me(4, 5); }
Index: lldb/test/API/tools/lldb-server/memory-allocation/main.c
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/memory-allocation/main.c
@@ -0,0 +1 @@
+int main() { return 0; }
Index: lldb/test/API/tools/lldb-server/memory-allocation/TestGdbRemoteMemoryAllocation.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-server/memory-allocation/TestGdbRemoteMemoryAllocation.py
@@ -0,0 +1,101 @@
+
+import gdbremote_testcase
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+supported_linux_archs = ["x86_64", "i386"]
+supported_oses = ["linux"]
+
+class TestGdbRemoteMemoryAllocation(gdbremote_testcase.GdbRemoteTestCaseBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def allocate(self, size, permissions):
+self.test_sequence.add_log_lines(["read packet: $_M{:x},{}#00".format(size, permissions),
+  {"direction": "send",
+   "regex":
+   r"^\$([0-9a-f]+)#[0-9a-fA-F]{2}$",
+   "capture": {
+