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