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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
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 =
----------------
Hui wrote:
> labath wrote:
> > 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...
> I think WoW64 is i686 that shall deserve a separate arch specific implementation?
I am sorry, but I don't follow. Can you repeat the question?

(FWIW, I don't believe that the fact that two things are different from a certain point of view justifies copy-pasting (at least) 150 LOC. If you think it's confusing to use something called "i386" in things dealing with WoW64, you can always typedef the WOW64 context to it, or rename the i386 context to something more generic.)


================
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);
----------------
Hui wrote:
> labath wrote:
> > 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.
> These constructors contain a call to function template make_unique that is not allowed to access private members of NativeProcessWindows.
> Simply do the following could address this concern.
> 
> ```
> 
> -  auto process_up =
> -      std::make_unique<NativeProcessWindows>(launch_info, native_delegate, E);
> 
> +  auto process_up = std::unique_ptr<NativeProcessWindows>(
> +      new NativeProcessWindows(launch_info, native_delegate, E));
> ```
> 
The make_unique thing is unfortunate, but that is the nature of c++. Since you're concerned about consistency, I'll mention that other classes (e.g. NativeProcessLinux) also prioritize clarifying what is the public interface of a class over the niceness of being able to call make_unique.


================
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;
+  }
----------------
Hui wrote:
> labath wrote:
> > 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).
> Yes, agreed. But such codes are added to be consistent with what is now in upstream from https://reviews.llvm.org/rL364216. (CacheAllRegisterValues).
> 
Interesting. I don't think it's worth being consistent with that, as that code is hopefully going to go away soon, and lldb-server can offer much stronger guarantees about when these things can be called (due to everything being funneled through the gdb-remote protocol, which doesn't even support sending any packets while the process is running).

However, I am not familiar with windows APIs, so I'll leave this decision up to you...


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

https://reviews.llvm.org/D63165