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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
