[Windows] A basic implementation of memory allocations in a debuggee process

[aleksandr.urakov]

Summary

This patch adds a basic implementation of DoAllocateMemory and DoDeallocateMemory for Windows processes. For now it considers only the executable permission (and always allows reads and writes).

To run tests on x86 it requires https://reviews.llvm.org/D52613

[aleksandr.urakov]

Clang-format patch.

[clayborg]

We really should be making a lldb-server that works on windows instead of making a native windows process plug-in that only works on windows. That will allow remote debugging to windows machines instead of requiring a local connection. It will also allows debug sessions to be logged using the GDB remote packets and avoids all of the debugging that goes on with a plug-in (ProcessWindows) that does event notifications and everything differently than other targets.

[aleksandr.urakov]

I didn't know about such a priority, thanks... But in this case lldb-server also must be able to allocate, read and write in a debuggee. Doesn't it use the process plugin for that? Or somehow duplicates this functionality?

I have made some work on expressions evaluation on Windows, and it requires a possibility to allocate memory in a debuggee. Now I understand, that the priority is lldb-server, but is it a bad idea to apply now the changes I've made to proceed with expressions? I can look tomorrow what else was made with the process plugin, but it seems that there are not so many changes.

Don't you know about the status of lldb-server on Windows? How much work requires it?

[zturner]

It requires a lot of work (nobody has started porting it). lldb-server
exists on other platforms but it basically needs a full port to Windows. It
doesn’t use the same process plugin, but it would instead use a different
plugin that would copy much of the low level code. Once it works well
enough the existing process plugin would then be deleted. It would be great
if someone had time to work on it, but I understand if for practical
reasons other work has higher priority

[aleksandr.urakov]

Thanks for explanations!

Unfortunately I can't promise that I'll begin porting lldb-server on Windows in the nearest future. I've looked at my working copy, and there are only two small changes related to ProcessWindows plugin (but they are not related to the current review), may be we'll continue with the plugin for now? To proceed with other things that need some implementation of a such functionality (whether in ProcessWindows or lldb-server). How about this?

[zturner]

I think it’s fine. Eventually when you are ready to support remote
debugging hopefully we can convert it over to using lldb-server

[jingham]

I agree with Zachary, converting to NativeProcess is enough of a project that we should not block useful fixes to the current ProcessWindows plugin on it. OTOH, it is a good project - somebody should add it to the Projects page.

[labath]

lit/Expr/TestIRMemoryMapWindows.test

1–12 ↗

(On Diff #167338)

The only difference in this test is the command line used to compile the inferior right? That sounds like something that we will run into for a lot of lit test, so I think it's important to work something our right away. Making a windows-flavoured copy of each test is not tractable.

Is there a reason you have to use clang-cl here? I was under the impression that clang.exe worked fine on windows too (and used a gcc-compatible command line)...

[zturner]

One idea would be to define some lit substitutions like %debuginfo. It’s
true you can produce a gcc style command line that will be equivalent to a
clang-cl invocation but it won’t be easy. eg you’ll needing to pass
-fms-compatibility as well as various -I for includes.

It may be easier to have substitutions instead

[stella.stamenova]

In D52618#1250909, @zturner wrote:

One idea would be to define some lit substitutions like %debuginfo. It’s
true you can produce a gcc style command line that will be equivalent to a
clang-cl invocation but it won’t be easy. eg you’ll needing to pass
-fms-compatibility as well as various -I for includes.

It may be easier to have substitutions instead

Another option would be to define a way in lit to specify a command to run based on requirements - similar how we can use "windows" or "linux" in the "requires" command.

[zturner]

In D52618#1251076, @stella.stamenova wrote:

In D52618#1250909, @zturner wrote:

One idea would be to define some lit substitutions like %debuginfo. It’s
true you can produce a gcc style command line that will be equivalent to a
clang-cl invocation but it won’t be easy. eg you’ll needing to pass
-fms-compatibility as well as various -I for includes.

It may be easier to have substitutions instead

Another option would be to define a way in lit to specify a command to run based on requirements - similar how we can use "windows" or "linux" in the "requires" command.

Yea that would work too. REQUIRES isn't quite the right thing because that just makes the infrastructure decide whether to run or skip your test. It would need to be something different, like COMPILATION_SETTINGS: debug, opt, noexcept. But I think that would be quite a bit of work and probably not fit nicely with the existing ShTest. You might need a subclass of ShTest like LLDBShTest that can extend its functionality a bit.

[labath]

In D52618#1250909, @zturner wrote:

One idea would be to define some lit substitutions like %debuginfo. It’s
true you can produce a gcc style command line that will be equivalent to a
clang-cl invocation but it won’t be easy. eg you’ll needing to pass
-fms-compatibility as well as various -I for includes.

It may be easier to have substitutions instead

Using substitutions SGTM.

I am not sure if this is a good idea, but it had occured to me that we could put -fms-compatibility and friends into a substitution of it's own, which would be computed by lit (through some equivalent of clang++ -### ?). That way, the tests could still use g++ syntax, only the command lines would contain an extra %cflags argument. This has the advantage of extra flexibility over a predefined set of compile commands (%compile_with_debug, %compile_without_debug, ...), and it might be sufficient to make cross-compiling work, if we ever need it.

[zturner]

In D52618#1252372, @labath wrote:

In D52618#1250909, @zturner wrote:

One idea would be to define some lit substitutions like %debuginfo. It’s
true you can produce a gcc style command line that will be equivalent to a
clang-cl invocation but it won’t be easy. eg you’ll needing to pass
-fms-compatibility as well as various -I for includes.

It may be easier to have substitutions instead

Using substitutions SGTM.

I am not sure if this is a good idea, but it had occured to me that we could put -fms-compatibility and friends into a substitution of it's own, which would be computed by lit (through some equivalent of clang++ -### ?). That way, the tests could still use g++ syntax, only the command lines would contain an extra %cflags argument. This has the advantage of extra flexibility over a predefined set of compile commands (%compile_with_debug, %compile_without_debug, ...), and it might be sufficient to make cross-compiling work, if we ever need it.

Another idea I just thought of, which would basically be the heaviest hammer possible and give the most flexibility is to modify the lit infrastructure (just for lldb, not all of llvm) to look for a compiler_config.py file. If it finds it, it can run the file. This file could define whatever substitutions it wanted. In the top-level LLDB lit configuration, we could provide some basic common infrastructure to easily support common use cases. I don't have specifics in mind for how the implementation would look like, but from a user point of view (i.e. what the compiler_config.py would look like), I'm imagining you could write something like this:

# compiler_config.py
global compiler_config
compiler_config.create_configuration(
    "fooconfig",   # user can reference this config as 'fooconfig' from a test file.
    driver=best,    # use clang-cl on Windows, clang++ otherwise
    debug=True,   # pass /Z7 on Windows, -g otherwise
    optlevel=1,     # pass /O2 on Windows, -O2 otherwise
    output_type=sharedlib,   # pass -fPIC on Linux and /D_DLL on Windows
    exceptions=False,     # pass -fno-exceptions on Linux, /EHs-c- on Windows
    rtti=False,   # pass -fno-rtti on Linux, /GR- on Windows
    mode=compile-only)   # Don't run the linker

compiler_config.create_configuration(
    "barconfig", # user can reference this config as 'barconfig' from a test file.
    driver=g++, # this config always invokes clang++, never anything else.
    debug=False, optlevel=3, output_type=exe)  # etc

Then, in your test file, you could have:

; RUN: %fooconfig %p/foo.cpp
; RUN: %barconfig %p/bar.cpp

This is a very rough outline of the idea, but I think this could actually be really cool if done properly.

[Hui]

source/Plugins/Process/Windows/Common/ProcessWindows.cpp

712 ↗

(On Diff #167338)

Looks to me they are handlers to the GDB remote "m" and "M" packets. If the remote is on Windows host, the debug inferior shall be responsible to handle memory allocation/deallocation for JITed codes. (There is a process utility for InferiorCall). Otherwise, the requests will be answered by remote gdb server with memory region returned.

[zturner]

Is this still blocked on anything?

[aleksandr.urakov]

In D52618#1278442, @zturner wrote:

Is this still blocked on anything?

It seems that it's not blocked on anything. The test, as you have mentioned in the earlier message, is already duplicating only RUN: lldb-test lines, there are no check files used here.

source/Plugins/Process/Windows/Common/ProcessWindows.cpp

712 ↗

(On Diff #167338)

No, they are not, it's not an lldb-server plugin, it's a native Windows process plugin.

[zturner]

I think we should try as hard as possible to have just one way of writing these tests. So if we know there are going to be two different use cases, one where we have mutually exclusive variants (e.g. run a process on OSX, Linux, Windows), and platform agnostic variants (compile for multiple platform / debug info variants, run tests in every possible configuration), I think we should try to unify all of these under a single syntax.

One possible syntax that works today is to duplicate all the compile and test lines but delegate to an external check file.

; file 1
; REQUIRES: windows
; RUN: clang-cl ...
; RUN: lld-link ...
; RUN: lldb-test | FileCheck --check-file=%p/Inputs/some-test.check

; file 2
; UNSUPPORTED: windows
; RUN: clang++ ...
; RUN: lldb-test | FileCheck --check-file=%p/Inputs/some-test.check

This unblocks the patch, doesn't require any fancy logic that would be unfamiliar with people coming to LLDB, and is pretty simple.

I think we can do better than the above by removing the need to have multiple files, but the substitutions only get us halfway there because they don't handle the case where we actually want to repeat the same test multiple times with different command lines. So since we're going to have to fall back to the above approach for that, let's just do it everywhere until we can come up with a solution that unifies everything under one syntax.

FWIW, I've been brainstorming some modifications to the core lit infrastructure that would support this kind of thing. Here's some strawman syntax:

VARIANTS: v1, v2

REQUIRES-v1: windows
UNSUPPORTED-v2: windows

RUN-v1: %cxx %p/Inputs/call-function.cpp -g -o %t
RUN-v2: clang-cl /Zi %p/Inputs/call-function.cpp -o %t

RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-basic
RUN: lldb-test ir-memory-map -host-only %t %S/Inputs/ir-memory-map-basic

RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-overlap1
RUN: lldb-test ir-memory-map -host-only %t %S/Inputs/ir-memory-map-overlap1

RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-mix-malloc-free
RUN: lldb-test ir-memory-map -host-only %t %S/Inputs/ir-memory-map-mix-malloc-free

Now in this case the two variants are mutually exclusive, but they need not be. So, for example, in the case of my target variables test for builtin types, which I wrote as this:

// Test that we can display tag types.
// RUN: clang-cl /Z7 /GS- /GR- /c -Xclang -fkeep-static-consts /Fo%t.obj -- %s
// RUN: lld-link /DEBUG /nodefaultlib /entry:main /OUT:%t.exe /PDB:%t.pdb -- %t.obj
// RUN: env LLDB_USE_NATIVE_PDB_READER=1 lldb -f %t.exe -s \
// RUN:     %p/Inputs/globals-fundamental.lldbinit | FileCheck %s

// bunch of source code

We could re-write it as:

// VARIANTS: v1, v2

// RUN-v1: clang-cl /Z7 /GS- /GR- /c -Xclang -fkeep-static-consts /Fo%t.obj -- %s
// RUN-v1: lld-link /DEBUG /nodefaultlib /entry:main /OUT:%t.exe /PDB:%t.pdb -- %t.obj
// RUN-v1: env LLDB_USE_NATIVE_PDB_READER=1 lldb -f %t.exe -s \
// RUN-v1:     %p/Inputs/globals-fundamental.lldbinit | FileCheck %s
// RUN-v2: clang++ -g -m64 -Xclang -fkeep-static-consts --target=x86_x64-linux-gnu -o %t -- %s
// RUN-v2: lldb -f %t -s %p/Inputs/globals-fundamental.lldbinit | FileCheck %s

// bunch of source code

This will run variant 1, then run variant 2.

I think this fits nicely with the lit "philosophy" and handles all the use cases that we need. But there's a lot of detail in implementing it correctly (and right now it's just in my head, I'm not actually working on it). So I think we should go with the simple thing for now of just having 2 files and one shared check file so that we can unblock the patch and make forward progress

[labath]

Ok, if that's how you guys feel, then I won't stand in your way. I am fine with this patch then.

lldb/trunk/lit/Expr/TestIRMemoryMap.test

-# XFAIL: windows
+# UNSUPPORTED: windows
 
 # RUN: %cxx %p/Inputs/call-function.cpp -g -o %t
 
 # RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-basic
 # RUN: lldb-test ir-memory-map -host-only %t %S/Inputs/ir-memory-map-basic
 
 # RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-overlap1
 # RUN: lldb-test ir-memory-map -host-only %t %S/Inputs/ir-memory-map-overlap1
 
 # RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-mix-malloc-free
 # RUN: lldb-test ir-memory-map -host-only %t %S/Inputs/ir-memory-map-mix-malloc-free

lldb/trunk/lit/Expr/TestIRMemoryMapWindows.test

+# REQUIRES: windows
+
+# RUN: clang-cl /Zi %p/Inputs/call-function.cpp -o %t
+
+# RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-basic
+# RUN: lldb-test ir-memory-map -host-only %t %S/Inputs/ir-memory-map-basic
+
+# RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-overlap1
+# RUN: lldb-test ir-memory-map -host-only %t %S/Inputs/ir-memory-map-overlap1
+
+# RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-mix-malloc-free
+# RUN: lldb-test ir-memory-map -host-only %t %S/Inputs/ir-memory-map-mix-malloc-free

lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.h

public:
   bool UpdateThreadList(ThreadList &old_thread_list,
                         ThreadList &new_thread_list) override;
   bool IsAlive() override;
 
   size_t DoReadMemory(lldb::addr_t vm_addr, void *buf, size_t size,
                       Status &error) override;
   size_t DoWriteMemory(lldb::addr_t vm_addr, const void *buf, size_t size,
                        Status &error) override;
+  lldb::addr_t DoAllocateMemory(size_t size, uint32_t permissions,
+                                Status &error) override;
+  Status DoDeallocateMemory(lldb::addr_t ptr) override;
   Status GetMemoryRegionInfo(lldb::addr_t vm_addr,
                              MemoryRegionInfo &info) override;
 
   lldb::addr_t GetImageInfoAddress() override;
 
   // IDebugDelegate overrides.
   void OnExitProcess(uint32_t exit_code) override;
   void OnDebuggerConnected(lldb::addr_t image_base) override;

lldb/trunk/source/Plugins/Process/Windows/Common/ProcessWindows.cpp

HANDLE process_handle =
       ::OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, FALSE, pid);
   if (process_handle != NULL) {
     file_name = GetProcessExecutableName(process_handle);
     ::CloseHandle(process_handle);
   }
   return file_name;
 }
 
+DWORD ConvertLldbToWinApiProtect(uint32_t protect) {
+  // We also can process a read / write permissions here, but if the debugger
+  // will make later a write into the allocated memory, it will fail. To get
+  // around it is possible inside DoWriteMemory to remember memory permissions,
+  // allow write, write and restore permissions, but for now we process only
+  // the executable permission.
+  //
+  // TODO: Process permissions other than executable
+  if (protect & ePermissionsExecutable)
+    return PAGE_EXECUTE_READWRITE;
+
+  return PAGE_READWRITE;
+}
+
 } // anonymous namespace
 
 namespace lldb_private {
 
 // We store a pointer to this class in the ProcessWindows, so that we don't
 // expose Windows-specific types and implementation details from a public
 // header file.
 class ProcessWindowsData {
if (WriteProcessMemory(handle, addr, buf, size, &bytes_written))
     FlushInstructionCache(handle, addr, bytes_written);
   else {
     error.SetError(GetLastError(), eErrorTypeWin32);
     LLDB_LOG(log, "writing failed with error: {0}", error);
   }
   return bytes_written;
 }
 
+lldb::addr_t ProcessWindows::DoAllocateMemory(size_t size, uint32_t permissions,
+                                              Status &error) {
+  Log *log = ProcessWindowsLog::GetLogIfAny(WINDOWS_LOG_MEMORY);
+  llvm::sys::ScopedLock lock(m_mutex);
+  LLDB_LOG(log, "attempting to allocate {0} bytes with permissions {1}", size,
+           permissions);
+
+  if (!m_session_data) {
+    LLDB_LOG(log, "cannot allocate, there is no active debugger connection.");
+    error.SetErrorString(
+        "cannot allocate, there is no active debugger connection");
+    return 0;
+  }
+
+  HostProcess process = m_session_data->m_debugger->GetProcess();
+  lldb::process_t handle = process.GetNativeProcess().GetSystemHandle();
+  auto protect = ConvertLldbToWinApiProtect(permissions);
+  auto result = VirtualAllocEx(handle, nullptr, size, MEM_COMMIT, protect);
+  if (!result) {
+    error.SetError(GetLastError(), eErrorTypeWin32);
+    LLDB_LOG(log, "allocating failed with error: {0}", error);
+    return 0;
+  }
+
+  return reinterpret_cast<addr_t>(result);
+}
+
+Status ProcessWindows::DoDeallocateMemory(lldb::addr_t ptr) {
+  Status result;
+
+  Log *log = ProcessWindowsLog::GetLogIfAny(WINDOWS_LOG_MEMORY);
+  llvm::sys::ScopedLock lock(m_mutex);
+  LLDB_LOG(log, "attempting to deallocate bytes at address {0}", ptr);
+
+  if (!m_session_data) {
+    LLDB_LOG(log, "cannot deallocate, there is no active debugger connection.");
+    result.SetErrorString(
+        "cannot deallocate, there is no active debugger connection");
+    return result;
+  }
+
+  HostProcess process = m_session_data->m_debugger->GetProcess();
+  lldb::process_t handle = process.GetNativeProcess().GetSystemHandle();
+  if (!VirtualFreeEx(handle, reinterpret_cast<LPVOID>(ptr), 0, MEM_RELEASE)) {
+    result.SetError(GetLastError(), eErrorTypeWin32);
+    LLDB_LOG(log, "deallocating failed with error: {0}", result);
+    return result;
+  }
+
+  return result;
+}
+
 Status ProcessWindows::GetMemoryRegionInfo(lldb::addr_t vm_addr,
                                            MemoryRegionInfo &info) {
   Log *log = ProcessWindowsLog::GetLogIfAny(WINDOWS_LOG_MEMORY);
   Status error;
   llvm::sys::ScopedLock lock(m_mutex);
   info.Clear();
 
   if (!m_session_data) {