[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/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);
----------------
I guess these shouldn't be public as these object should be constructed through the factory, right?


================
Comment at: lldb/source/Plugins/Process/Windows/Common/NativeRegisterContextWindows.cpp:31-36
+  if (!data_sp) {
+    error.SetErrorStringWithFormat(
+        "failed to allocate DataBufferHeap instance of size %" PRIu64,
+        data_size);
+    return error;
+  }
----------------
`new` doesn't fail. This is dead code.


================
Comment at: lldb/source/Plugins/Process/Windows/Common/NativeRegisterContextWindows.cpp:52-57
+  if (dst == nullptr) {
+    error.SetErrorStringWithFormat("DataBufferHeap instance of size %" PRIu64
+                                   " returned a null pointer",
+                                   data_size);
+    return error;
+  }
----------------
This can't ever be true.


================
Comment at: lldb/source/Plugins/Process/Windows/Common/NativeRegisterContextWindows.cpp:68-89
+  if (!data_sp) {
+    error.SetErrorStringWithFormat(
+        "NativeRegisterContextWindows::%s invalid data_sp provided",
+        __FUNCTION__);
+    return error;
+  }
+
----------------
These look like they should be operating invariants (at most guarded by an assertion, but probably even that is not needed). Right now, they're just making it hard to figure out what this function actually does...


================
Comment at: lldb/source/Plugins/Process/Windows/Common/NativeRegisterContextWindows_WoW64.cpp:82
+  ::WOW64_CONTEXT tls_context;
+  NativeThreadWindows *wthread = static_cast<NativeThreadWindows *>(&m_thread);
+
----------------
How about a helper method like `NativeThreadWindows &GetThread()` to hide the static_cast everywhere. You could also change the constructor argument to `NativeThreadWindows&` to make it clear that the register context is only to be constructed with threads of this type.

a `GetThreadHandle` might be nice too since fetching the thread seems to be invariably followed by the `GetHostThread().GetNativeThread().GetSystemHandle()` blurb.


================
Comment at: lldb/source/Plugins/Process/Windows/Common/NativeThreadWindows.cpp:80-84
+Status NativeThreadWindows::DoDestroy() {
+  m_state = eStateExited;
+  m_stop_info.reason = StopReason::eStopReasonThreadExiting;
+  return Status();
+}
----------------
What's the purpose of this function? It seems to be only called immediately before deleting the thread, so nobody gets to read it's stop reason anyway...


================
Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.h:31
+class NativeProcessWindows : public NativeProcessProtocol,
+                             public ProcessDebugger {
+
----------------
Hui wrote:
> labath wrote:
> > Hui wrote:
> > > labath wrote:
> > > > I'm not sure this multiple inheritance is really helping anything here. While looking through the code i kept wondering "why doesn't this function just do the job itself instead of delegating to somebody else" only to have to remind myself that the function it is delegating to is actually a different object, which does not know anything about the bigger picture.
> > > > 
> > > > If using composition here wouldn't introduce additional complexities/code (which it doesn't look like it would), I think it might be better to do that instead.
> > > Do you mean to move what is in ProcessDebugger into nativeprocess instead?  
> > No, just instead of inheriting from a ProcessDebugger, making it a member variable instead. Unless there are reasons which make that hard/impossible...
> Debug event callback in ProcessDebugger need to be overridden by both processwindows and nativeprocess. Not sure if the change is doable. Also same change is required to processwindows. That will make the patch a little bigger.
Ok, let's leave it like that for now. However, I find it very weird that the `ProcessDebugger` class takes a "delegate" object *and* it requires you to inherit from it to override some of it's methods. Seems like one of these extension mechanisms ought to be enough.


================
Comment at: source/Plugins/Process/Windows/Common/NativeRegisterContextWindows.h:31
+protected:
+  Status ReadAllRegisterValues(lldb::DataBufferSP &data_sp,
+                               const size_t data_size);
----------------
labath wrote:
> Hui wrote:
> > labath wrote:
> > > Is this overriding something? Can you please use `override` to indicate that (throughout this patch)?
> > No, it doesn't override anything. It has different signature from  the pure virtual method with the same name.
> > 
> > 
> > ```
> > NativeRegisterContext::virtual Status ReadAllRegisterValues(lldb::DataBufferSP &data_sp) = 0;
> > ```
> > 
> > It would be better to change the name to be ReadAllRegisterValuesWithSize or something else.
> Hm.., interesting. I think I am starting to understand what's going on here. IIUC, the size of the register context is a constant for any given architecture, but it varies between architectures (i.e., NativeRegisterContextWindows_XXX instances). If that's the case, then it sounds like the data_size should be an argument to the NativeRegisterContextWindows constructor, instead it being passed down for every call. Would that work?
I still think the register context size would be better handled as an constructor argument.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D63165