labath added inline comments.
================
Comment at: source/Plugins/Process/Windows/Common/ProcessDebugger.cpp:169-170
+
+Status ProcessDebugger::AttachProcess(lldb::pid_t pid,
+ const ProcessAttachInfo &attach_info,
+ DebugDelegateSP delegate) {
----------------
Hui wrote:
> labath wrote:
> > Hui wrote:
> > > Hui wrote:
> > > > labath wrote:
> > > > > BTW, looking at the other patch, I just realized that this api is
> > > > > extremely redundant:
> > > > > a) `ProcessAttachInfo` already contains a `pid` field so passing it
> > > > > separately makes no sense
> > > > > b) `DebuggerThread` does not seem to be doing anything with the
> > > > > ProcessAttachInfo struct anyway.
> > > > I once checked the pid shipped in attach_info. It was
> > > > LLDB_INVALID_PROCESS_ID. Maybe it is an issue to fix.
> > > Just double checked. The pid shipped in attach_info is correct. So the
> > > pid field can be removed.
> > > However the Process class does have a virtual method
> > > DoAttachToProcessWithID that requires pid as an input.
> > Hm.., I didn't realize this comes all the way down from the Process. Maybe
> > let's keep this as is for now, since this patch is just about moving code.
> There is a build bot reported about attaching process by id/name error. When
> I looked into that, it turned out that the attach_info did not carry the
> correct process id at all (pid = 0). This resulted some test cases fail and
> timeout.
I am fine with keeping the `pid` field here as this seems to be a larger issue
that ought to be handled separately.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63166/new/
https://reviews.llvm.org/D63166
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
