[Lldb-commits] [PATCH] D63166: Move common functionality from processwindows into processdebugger

Hui Huang via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed Jul 3 18:39:31 PDT 2019

Hui 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) {
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. 



More information about the lldb-commits mailing list