Thanks for the heads up, Zachary!
On Tue, Sep 30, 2014 at 9:58 AM, Zachary Turner <ztur...@google.com> wrote:
> 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
>
>
--
Todd Fiala | Software Engineer | tfi...@google.com | 650-943-3180
_______________________________________________
lldb-dev mailing list
lldb-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
