Makes sense. I think the handle makes more sense in the ProcessInstanceInfo than the ProcessInfo, since by definition a HANDLE implies that you're referring to an instance of the process that is currently running, which is what ProcessInstanceInfo is for.
Still though, I wonder if it might be better to have LaunchProcess take a ProcessInstanceInfo* argument that could be filled out by the function. I agree that just putting the lldb::host::process_t directly into the ProcessLaunchInfo alongside the pid would be easier, but I don't mind doing the extra work for a more robust solution, if you agree that there's value in doing it this way. On Tue, Aug 12, 2014 at 4:09 PM, Greg Clayton <[email protected]> wrote: > > > On Aug 12, 2014, at 3:58 PM, Zachary Turner <[email protected]> wrote: > > > > Thanks, that makes sense. The problem is that Host::LaunchProcess > doesn't really return anything useful. When I create a process on Windows, > I get a HANDLE back that I can use to later manipulate the process. Note > that this isn't the same as a Pid. I guess Host::LaunchProcess will return > the pid in the launch_info structure, > > Indeed it does. > > > which I could then use to get another handle, but that would involve > closing the first handle before I get the second handle, which I'm a little > wary of, and I would feel better just using the original handle. But > there's no obvious way to pass it back. > > We can have the LaunchInfo contain the new "host" version of the process > handle which would need to be added to the lldb-types.h. We had spoke about > this before about possibly adding a: > > namespace lldb { > namespace host { > typedef pid_t process_t; > } > } > > On windows you could make this a process handle and add > lldb_private::ProcessInfo could have a new ivar added: > > namespace lldb_private > { > class ProcessInfo > { > > FileSpec m_executable; > std::string m_arg0; // argv[0] if supported. If empty, then use > m_executable. > // Not all process plug-ins support specifying an argv[0] > // that differs from the resolved platform executable > // (which is in m_executable) > Args m_arguments; // All program arguments except argv[0] > Args m_environment; > uint32_t m_uid; > uint32_t m_gid; > ArchSpec m_arch; > lldb::pid_t m_pid; > lldb::host::process_t m_process_handle; // <<<<< new ivar > }; > > > > > I'm not sure what the right abstraction is here that would make sense on > all platforms, but I feel like what is needed is some kind of NativeProcess > class, and LaunchProcess's signature could change to this: > > I believe the lldb::host::process_t should be enough. It can be a class if > needed for your system, or it can just be a quick typedef like above. > > > > > static Error > > Host::LaunchProcess (ProcessLaunchInfo &launch_info, NativeProcess > **process); > > > > If the user passes in NULL, then we just clean up the process handle (or > do nothing, depending on platform), and if it's not NULL, then Host > allocates the appropriate type of Platform specific Process object. For > example, it allocates a NativeProcessLinux, or a NativeProcessMacOSX, or a > NativeProcessWindows, etc. > > > > Do you think something like this could work? Or maybe have other ideas? > > I would update the ProcessInfo (ProcessInstanceInfo inherits from it) > since this also ties into the platform functions that find and get info for > processes: > > > virtual uint32_t > Platform::FindProcesses (const ProcessInstanceInfoMatch > &match_info, > ProcessInstanceInfoList &proc_infos); > > virtual bool > Platform::GetProcessInfo (lldb::pid_t pid, ProcessInstanceInfo > &proc_info); > > And it would be nice to get these native process handles there as well. > > Greg >
_______________________________________________ lldb-dev mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
