Heads up, I'm actually going to put this on hold for the foreseeable
future. It's more difficult than I anticipated, which although that just
makes the code re-organization more desirable in my mind, it also makes the
risk pretty high. I would rather just let the changes I've done bake for a
while, and then maybe come back to this in a few months, hopefully making
smaller changes along the way that make this easier in the future.
On Fri, Sep 26, 2014 at 9:44 AM, Zachary Turner <ztur...@google.com> wrote:
>
>
> On Wed, Sep 24, 2014 at 10:56 AM, Greg Clayton <gclay...@apple.com> wrote:
>
>> Comments:
>>
>> - When running processes one might want to get signals for the process
>> that are delivered while the child process is running. There can be any
>> amount of signals over the child process' lifetime. This must be included
>> in the design. It might not work for windows, but it will need to be worked
>> into the design.
>>
>
> Can you clarify? The goal was to propose this in such a way that it's
> functionally equivalent to the current code. In the existing code, what is
> the means by which this currently happens? There is the ability to send a
> signal *to* a process, and that is covered in this design by providing a
> Signal() method on HostNativeProcessPosix. Anyone calling that method is
> invoking platform specific functionality, so they would have either already
> be inside platform specific code, or write something like
> HostProcess->GetNativeProcess()->Signal() wrapped in an appropriate #ifdef.
>
>
>
>>
>> - You can change the internal API, but you can't change the public API.
>> If you make changes to any of the launch info classes internally, you will
>> need to keep the SBLaunchInfo API the same. I know you like cleaning things
>> up, but we have clients that use this API and we don't want it broken. You
>> will need to carefully check for any other things that get modified in the
>> launch info class and make sure any such cases are fixed.
>>
> This shouldn't involve a change to the public API.
>
>
>>
>> - We should make ProcessLauncher a plug-in and be able to request them by
>> name. Then we can add things to the launch info to be able to specify the
>> process launcher plug-in name ("posix_spawn", "fork", "apple-xpc",
>> "default" which can map to the default launcher for a host, etc).
>>
> Good idea, probably can be done as a last step. All of the actual logic
> would still live in Host, since it's OS-specific, and the plugin code could
> just be a lightweight shim to select the correct one.
>
>
>> >
>> > This approach yields a number of benefits. For starters, it allows us
>> to abstract out the notion of the launching strategy into a reusable piece
>> of code so that platforms which have multiple launching strategies have the
>> ability to make use of specific strategies in other platform specific code,
>> or so that remote process launches can be treated the same as local process
>> launches. As an example, on MacOSX there are three different launching
>> strategies: Launch with XPC services, launch with Applescript, and launch
>> with posix_spawn. Each of these could be independent implementations of
>> |ProcessLauncher|, with the default implementation on MacOSX being a fourth
>> implementation that chooses one of the first 3 implementations to delegate
>> to at runtime. As a result, the 3 implementations can be used
>> independently of each other should the need arise.
>> >
>> > A second benefit is that it opens up the possibility of treating remote
>> process launch transparently with local process launch -- just have a
>> |RemoteProcessLauncher| class.
>>
>> Uh... Then don't call your class HostProcess. I really would rather not
>> have remote launching involved in this at all. Remote launching should
>> result in a lldb::pid_t and it won't use the native process layer for
>> anything, so I would avoid including remote process launching in this as it
>> mucks up what info might need to be in the launch info (remote host/port,
>> password, etc). So no remote process support please.
>>
> Fair enough, I mentioned that just as something that may or may not be
> useful in the future, but the design lends itself nicely to. I don't want
> to choose names based on something that might not happen though, so I think
> it still makes sense to go with HostProcess, since for now that's what it
> actually is, and gives some naming consistency between this and HostThread.
>
>
>>
>> >
>> > There are additional benefits in the form of overall code health,
>> platform separation, and just generally improving the modularity of the
>> codebase, although those are perhaps less tangible.
>> >
>> > Monitoring for process exit
>> > It is important to know when a process exits, for 2 reasons. First, on
>> some platforms the process needs to be reaped. Second, asynchronous exit
>> callbacks need to be invoked. The implementation of the monitoring is
>> platform dependent, and a strategy for each platform can be summarized as
>> follows:
>> >
>> > • Windows
>> > • Create a background thread, use WaitForSingleObject
>> > • [Better, alternative implementation] Create one
>> "static" background thread for all processes, use WaitForMultipleObjects()
>> to monitor many processes at once, like select().
>> > • MacOSX
>> > • Processes spawned with posix_spawn - Use MacOSX
>> "dispatch" library to queue monitoring code on a system thread.
>> > • Processes spawned with XPC - Use MacOSX "dispatch"
>> library to queue monitoring code on a system thread.
>> > • Processes spawned with Applescript - No monitoring
>> necessary
>> > • Non-MacOSX posix systems
>> > • Processes spawned with posix_spawn - Create a
>> background thread, use waitpid in the background thread.
>>
>> We also need to be able to deliver signals during child process execution.
>>
> Mentioned earlier, but this is already covered. HostProcess is a wrapper
> around a HostNativeProcess, and provides a method just like HostThread
> does, which returns the native process. Since signal is not a generic
> operation, it wouldn't live on HostProcess, but on HostNativeProcessPosix.
> Anyone wishing to deliver a signal to a child process would write
> host_process.GetNativeProcess()->Signal(SIGTERM) or whatever. If they
> write this from generic code it would need to be inside of an #ifdef. If
> they write it from platform-specific code it will just work.
>
>
_______________________________________________
lldb-dev mailing list
lldb-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
