[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: source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp:315
+                                         lldb::addr_t &load_addr) {
+  return GetProcessBaseAddress(m_pid, load_addr);
+}
----------------
This doesn't look right. You're completely ignoring the `file_name` argument. The idea of this function is to return the load (base) address of the module specified by that argument. Here I guess you're always returning the load address of the main module.


================
Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp:529-530
+  ProcessAttachInfo attach_info;
+  attach_info.SetProcessID(pid);
+  attach_info.SetArchitecture(process_info.GetArchitecture());
+
----------------
Hui wrote:
> labath wrote:
> > 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.
> This factory attach will return a constructed native process which needs process architecture ahead to construct its native thread with a proper register context. So I think can't do it after the attach operation. At least need to do it before the factory routine returns. Do you mean to put these codes before line 540, i.e. return std::move(process_up)?
I meant doing this *before* the factory returns, but *after* you perform the actual OS attach syscall is completed.

Combining this with the constructor suggestion below, I'd imagine doing something like
```
Error E = Error::success();
auto process_up = std::make_unique<NativeProcessWindows>(pid, -1, native_delegate, E);
if (E)
  return E;
return std::move(process_up);
```

Then in the process constructor, you'd do:
```
ErrorAsOutParameter EOut(E);
auto delegate_sp = std::make_shared<NativeDebugDelegate>(*this);
ProcessAttachInfo attach_info;
attach_info.SetPID(pid); // TODO: This is dumb, we are already passing the pid separately
E = AttachProcess(pid, attach_info, delegate).ToError();
if (E)
  return;
SetID(GetDebuggedProcessId());
ProcessInstanceInfo info;
if (!Host::GetProcessInfo(pid, info)) {
  E = createStringError(inconvertibleErrorCode(), "Cannot get process information")
  return;
}
m_arch = info.GetArchitecture();
```

Among other things, I'm trying to cut down on the number of layers one has to go through in order to launch/attach. This file already has a number of functions called "attach", D63166 has a bunch more, and then there even more in DebuggerThread. By the time one gets to the bottom of the stack and figures out what's actually happening, he's forgotten what was he actually trying to achieve.


================
Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.h:31
+class NativeProcessWindows : public NativeProcessProtocol,
+                             public ProcessDebugger {
+
----------------
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.


================
Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.h:133
+public:
+  NativeDebugDelegate(NativeProcessWindows *process) : m_process(process) {}
+
----------------
It doesn't look like `process` should ever be null. So, I'd suggest replacing the pointer by a reference, and deleting all the null checks.


================
Comment at: source/Plugins/Process/Windows/Common/NativeRegisterContextWindows.h:31
+protected:
+  Status ReadAllRegisterValues(lldb::DataBufferSP &data_sp,
+                               const size_t data_size);
----------------
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?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D63165