[Lldb-commits] [PATCH 6/8] Miscellaneous cleanups in ProcessMonitor.

Greg Clayton gclayton at apple.com
Fri Jan 14 15:49:08 PST 2011


Yep, just make sure if the parent of the thread goes away, that it is sure to make the thread terminate otherwise the parent will delete the value in the auto_ptr and the still running thread might crash? Most of our code has a parent class object that might spawn a thread, then the parent object will pass itself (the "this" pointer to the thread), and the parent object ensures that the thread is shut down properly when it "void Clear()" is called on the object. The destructor of the parent object then calls Clear() to make sure the thread shuts down.

Then you can call accessors on the parent class object to get the LaunchArgs data. Feel free to take your own approch, but I just wanted to tell you what other code is commonly doing withing LLDB.

Greg Clayton

On Jan 14, 2011, at 2:57 PM, Stephen Wilson wrote:

> Greg Clayton <gclayton at apple.com> writes:
> 
>> The lifetime of args seems possibly racy? What if
>> "StartOperationThread()" spawns a thread which gets an error and exits
>> super quickly, could the thread put "args" into its auto_ptr and
>> delete "args" before the code that launches that thread is done with
>> "args"? I can't see all the code here, so I am just curious.
> 
> I took another look over the code.  You are right, there is a race if
> the monitor thread encounters an error when launching the inferior.
> 
> This corrected patch uses the previous approach of having the parent
> thread maintain ownership.  Holding onto the launch args in an auto_ptr
> should be safe here since we synchronize with the child thread via a
> semaphore, and the child does not reference the launch args beyond that
> point.
> 
> Thanks for the review!
> 
> 
> 
> 
>    Miscellaneous cleanups in ProcessMonitor.
> 
>    Propagate the environment if one is not provided.  Also, do not allocate the
>    monitor threads launch arguments on the stack.
> 
> diff --git a/source/Plugins/Process/Linux/ProcessMonitor.cpp b/source/Plugins/Process/Linux/ProcessMonitor.cpp
> index 1522263..f21cb00 100644
> --- a/source/Plugins/Process/Linux/ProcessMonitor.cpp
> +++ b/source/Plugins/Process/Linux/ProcessMonitor.cpp
> @@ -463,8 +463,10 @@ ProcessMonitor::ProcessMonitor(ProcessLinux *process,
>       m_client_fd(-1),
>       m_server_fd(-1)
> {
> -    LaunchArgs args(this, module, argv, envp,
> -                    stdin_path, stdout_path, stderr_path);
> +    std::auto_ptr<LaunchArgs> args;
> +
> +    args.reset(new LaunchArgs(this, module, argv, envp,
> +                              stdin_path, stdout_path, stderr_path));
> 
>     // Server/client descriptors.
>     if (!EnableIPC())
> @@ -473,13 +475,13 @@ ProcessMonitor::ProcessMonitor(ProcessLinux *process,
>         error.SetErrorString("Monitor failed to initialize.");
>     }
> 
> -    StartOperationThread(&args, error);
> +    StartOperationThread(args.get(), error);
>     if (!error.Success())
>         return;
> 
> WAIT_AGAIN:
>     // Wait for the operation thread to initialize.
> -    if (sem_wait(&args.m_semaphore))
> +    if (sem_wait(&args->m_semaphore))
>     {
>         if (errno == EINTR)
>             goto WAIT_AGAIN;
> @@ -491,16 +493,17 @@ WAIT_AGAIN:
>     }
> 
>     // Check that the launch was a success.
> -    if (!args.m_error.Success())
> +    if (!args->m_error.Success())
>     {
>         StopOperationThread();
> -        error = args.m_error;
> +        error = args->m_error;
>         return;
>     }
> 
>     // Finally, start monitoring the child process for change in state.
> -    if (!(m_monitor_thread = Host::StartMonitoringChildProcess(
> -              ProcessMonitor::MonitorCallback, this, GetPID(), true)))
> +    m_monitor_thread = Host::StartMonitoringChildProcess(
> +        ProcessMonitor::MonitorCallback, this, GetPID(), true);
> +    if (m_monitor_thread == LLDB_INVALID_HOST_THREAD)
>     {
>         error.SetErrorToGenericError();
>         error.SetErrorString("Process launch failed.");
> @@ -574,6 +577,10 @@ ProcessMonitor::Launch(LaunchArgs *args)
> 
>     lldb::ThreadSP inferior;
> 
> +    // Propagate the environment if one is not supplied.
> +    if (envp == NULL || envp[0] == NULL)
> +        envp = const_cast<const char **>(environ);
> +
>     // Pseudo terminal setup.
>     if (!terminal.OpenFirstAvailableMaster(O_RDWR | O_NOCTTY, err_str, err_len))
>     {
> 





More information about the lldb-commits mailing list