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