[Lldb-commits] [PATCH] D63165: Initial support for native debugging of x86/x64 Windows processes
Pavel Labath via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Wed Jun 12 03:07:19 PDT 2019
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.
Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.cpp:529-530
+ ProcessAttachInfo attach_info;
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
> 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
+ 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
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.
CHANGES SINCE LAST ACTION
More information about the lldb-commits