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

[Greg Clayton]

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.

Other than that check, it looks good.

On Jan 14, 2011, at 1:12 PM, Stephen Wilson wrote:

> Propagate the environment if one is not provided.  Also, do not allocate the
> monitor threads launch arguments on the stack (let the monitor thread assume
> ownership).
> ---
> source/Plugins/Process/Linux/ProcessMonitor.cpp |   29 +++++++++++++++--------
> 1 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/source/Plugins/Process/Linux/ProcessMonitor.cpp b/source/Plugins/Process/Linux/ProcessMonitor.cpp
> index 1522263..8dd9804 100644
> --- a/source/Plugins/Process/Linux/ProcessMonitor.cpp
> +++ b/source/Plugins/Process/Linux/ProcessMonitor.cpp
> @@ -463,7 +463,7 @@ ProcessMonitor::ProcessMonitor(ProcessLinux *process,
>       m_client_fd(-1),
>       m_server_fd(-1)
> {
> -    LaunchArgs args(this, module, argv, envp,
> +    LaunchArgs *args = new LaunchArgs(this, module, argv, envp,
>                     stdin_path, stdout_path, stderr_path);
> 
>     // Server/client descriptors.
> @@ -473,13 +473,13 @@ ProcessMonitor::ProcessMonitor(ProcessLinux *process,
>         error.SetErrorString("Monitor failed to initialize.");
>     }
> 
> -    StartOperationThread(&args, error);
> +    StartOperationThread(args, 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 +491,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.");
> @@ -524,12 +525,16 @@ void
> ProcessMonitor::StartOperationThread(LaunchArgs *args, Error &error)
> {
>     static const char *g_thread_name = "lldb.process.linux.operation";
> +    std::auto_ptr<LaunchArgs> args_ap(args);
> 
>     if (m_operation_thread != LLDB_INVALID_HOST_THREAD)
>         return;
> 
>     m_operation_thread =
>         Host::ThreadCreate(g_thread_name, OperationThread, args, &error);
> +
> +    if (error.Success())
> +        args_ap.release();
> }
> 
> void
> @@ -547,12 +552,12 @@ ProcessMonitor::StopOperationThread()
> void *
> ProcessMonitor::OperationThread(void *arg)
> {
> -    LaunchArgs *args = static_cast<LaunchArgs*>(arg);
> +    std::auto_ptr<LaunchArgs> args_ap(static_cast<LaunchArgs*>(arg));
> 
> -    if (!Launch(args))
> +    if (!Launch(args_ap.get()))
>         return NULL;
> 
> -    ServeOperation(args->m_monitor);
> +    ServeOperation(args_ap->m_monitor);
>     return NULL;
> }
> 
> @@ -574,6 +579,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))
>     {
> -- 
> 1.7.3.5
> 
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits