[Lldb-commits] [PATCH] D63165: Initial support for native debugging of x86/x64 Windows processes

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I am sorry, but the code still seems a lot more verbose to me than it should be needed to implement the given functionality. I'd like to understand why/if it's that necessary..



================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextWindows_WoW64.cpp:72-105
+static const RegisterInfo *GetRegisterInfoPtr(const ArchSpec &target_arch) {
+  switch (target_arch.GetMachine()) {
+  case llvm::Triple::x86:
+    return g_register_infos_WoW64;
+  default:
+    llvm_unreachable("Unhandled target architecture.");
+  }
----------------
This is very verbose. You could just have `GetRegisterInfo` return `g_register_infos_WoW64` directly (and similarly for other functions. Then all you'd be left with is putting an `assert(target_arch.GetMachine() == llvm::Triple::x86)` into the RegisterContextWindows_WoW64 constructor. Same goes for other register context classes.


================
Comment at: lldb/source/Plugins/Process/Utility/RegisterContextWindows_x86_64.cpp:141
+GetRegisterInfo_WoW64(const lldb_private::ArchSpec &arch) {
+  // A WoW64 register info is the same as the i386's.
+  std::vector<lldb_private::RegisterInfo> &g_register_infos =
----------------
Why is all of this complexity necessary? Couldn't you just switch on the target architecture in `CreateRegisterInfoInterface` in NativeRegisterContextWindows_x86_64.cpp and create either `RegisterContextWindows_WoW64` or `RegisterContextWindows_x86_64` ?

In fact, if RegisterContextWindows_WoW64 is identical to RegisterContextWindows_i386, then why do you even need the _WoW64 version of the class in the first place? FWIW, linux also does not have a RegisterContextLinux_32_bit_process_on_64_bit_kernel class...


================
Comment at: lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp:285-286
+NativeProcessWindows::GetAuxvData() const {
+  // Not available on this target.
+  return nullptr;
+}
----------------
I think it would be better to return an error here. `llvm::errc::not_supported` ?


================
Comment at: lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp:311-337
+  Status error;
+  if (!m_loaded_modules.empty())
+    return Status();
+
+  // Retrieve loaded modules by a Target/Module free implemenation.
+  AutoHandle snapshot(CreateToolhelp32Snapshot(TH32CS_SNAPMODULE, GetID()));
+  if (snapshot.IsValid()) {
----------------
This doesn't seem right. You only retrieve the module list the first time this function was called, but the list of loaded modules can change over time. You probably need to invalidate this cache every time the process is resumed...


================
Comment at: lldb/source/Plugins/Process/Windows/Common/NativeProcessWindows.h:45-49
+  NativeProcessWindows(ProcessLaunchInfo &launch_info, NativeDelegate &delegate,
+                       llvm::Error &E);
+
+  NativeProcessWindows(lldb::pid_t pid, int terminal_fd,
+                       NativeDelegate &delegate, llvm::Error &E);
----------------
labath wrote:
> I guess these shouldn't be public as these object should be constructed through the factory, right?
This doesn't seem to be addressed.


================
Comment at: lldb/source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_x86_64.cpp:104-109
+  if (::SuspendThread(thread_handle) == -1) {
+    error.SetError(GetLastError(), eErrorTypeWin32);
+    LLDB_LOG(log, "{0} ResumeThread failed with error {1}", __FUNCTION__,
+             error);
+    return error;
+  }
----------------
Are these suspend/resume calls necessary? You should be able to assume that the process is stopped (due to breakpoint, exception or whatever). Nobody will be calling functions that get/set registers on a running process. (linux does not support that either, but we don't bother explicitly stopping the process before we attempt to do this).


================
Comment at: lldb/source/Plugins/Process/Windows/Common/NativeThreadWindows.cpp:139-199
+Status NativeThreadWindows::SetWatchpoint(lldb::addr_t addr, size_t size,
+                                          uint32_t watch_flags, bool hardware) {
+  if (!hardware)
+    return Status("not implemented");
+
+  if (m_state == eStateLaunching)
+    return Status();
----------------
IIUC, none of the current register contexts implement watchpoint support right now. Which means this code is dead/untested. Let's remove it for the time being...


================
Comment at: lldb/unittests/tools/lldb-server/tests/TestClient.cpp:258-260
+    // No SIGHUP on Windows to notify llgs that a user terminal already exits.
+    // As a result of that, llgs will stay connected and an ErrorReplyTimeout
+    // result will be expected. Here we check the connection on timeout.
----------------
This doesn't sound right. If the inferior exits, lldb-server should exit too. It doesn't matter if that's implemented via SIGHUP, or whatever. If some of the tests don't pass because of this, maybe you can disable the tests for the time being..


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

https://reviews.llvm.org/D63165