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

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: amccarth.
labath added a comment.

I take it that D63166 <https://reviews.llvm.org/D63166> is a prerequisite for this patch. Is that all, or is there something else that we ought to look at first?

Overall, this patch is slightly larger that would be ideal for a proper review, though I appreciate the difficulty in bootstrapping something like this. However, given that the register context gunk is actually a significant portion of this patch (though that's not your fault), maybe you could just restrict this patch to a single architecture, and do others as follow-ups? This would let the overall structure of the code stand out more prominently.

In D63165#1539118 <https://reviews.llvm.org/D63165#1539118>, @amccarth wrote:

> Sorry for the stupid question, but ...
>
> What exactly is meant here by "Native"?  How is a NativeProcessWindows different from ProcessWindows?


The Native*** classes are meant to be used from lldb-server. They look somewhat similar to their non-native counterpart because they still do debugging, but they're a lot dumber, because they only deal with basic process control, and none of the fancy symbolic stuff that you'd need debug info for.



================
Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp:352
+
+  // The very first one shall always be the matin thread.
+  assert(m_threads.empty());
----------------
s/matin/main


================
Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp:529-530
+  ProcessAttachInfo attach_info;
+  attach_info.SetProcessID(pid);
+  attach_info.SetArchitecture(process_info.GetArchitecture());
+
----------------
Fetching the architecture this way looks like it could be racy if the pid is recycled before you get a chance to attach to the process (can that happen?). Is there a way to fetch the architecture *after* you perform the attach operation.


================
Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp:536
+
+  error = process_up->DoAttachToProcessWithID(pid, attach_info);
+  if (error.Fail())
----------------
passing attach_info seems pretty redundant, as at this point the process should already be know the pid and the architecture (they were passed in the constructor). In fact, I think you should just delete `DoAttachToProcessWithID`, do that work in the constructor, and use the `ErrorAsOutParameter` pattern to return the error from there. (The Factory::Attach pattern was trying to avoid/discourage the use fallible constructors, but that's hard to avoid when you need to instantiate another object and set up callbacks. In that case, a fallible constructor is better over a separate function, as this way at least the initialization happens in a single step.)


================
Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.h:16
+#include "IDebugDelegate.h"
+#include "ProcessDebugger.h"
+
----------------
amccarth wrote:
> Where is ProcessDebugger.h?  Should that be part of this patch?
I guess that's introduced in D63166


================
Comment at: source/Plugins/Process/Windows/Common/NativeRegisterContextWindows.h:31
+protected:
+  Status ReadAllRegisterValues(lldb::DataBufferSP &data_sp,
+                               const size_t data_size);
----------------
Is this overriding something? Can you please use `override` to indicate that (throughout this patch)?


================
Comment at: source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_x86_64.cpp:13
+#include "NativeRegisterContextWindows_wow64.h"
+
+#include "NativeThreadWindows.h"
----------------
The empty lines here look completely random. I'd just delete all of the empty lines and let clang-format sort/group the includes for you.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D63165