NativeProcessLinux: Merge operation and monitor threads

[labath]

Summary

This commit moves the functionality of the operation thread into the new monitor thread. This is
required to avoid a kernel race between the two threads and I believe it actually makes the code
cleaner.

[tberghammer]

I think it made the code much cleaner

source/Plugins/Process/Linux/NativeProcessLinux.cpp

1215 ↗

(On Diff #23935)

Please move it to a static const char to the top of the file with a name indicating its meaning and use the same variable in the switch

1423 ↗

(On Diff #23935)

Can we get rid of the goto?

[labath]

responding to review comments

[labath]

Thanks. Please take another look.

source/Plugins/Process/Linux/NativeProcessLinux.cpp

1215 ↗

(On Diff #23935)

I've made it a private member of the Monitor class.

1423 ↗

(On Diff #23935)

Yep.

[ovyalov]

Please see my comments.

Overall looks great!

source/Plugins/Process/Linux/NativeProcessLinux.cpp

1077 ↗

(On Diff #23936)

Could you return Error instead of bool?

1080 ↗

(On Diff #23936)

Please combine definition of status variable with its initialization

1122 ↗

(On Diff #23936)

Nit: please move up m_operation_mutex so mutex is initialized before the data it protects.

1199 ↗

(On Diff #23936)

is it waiting for initial command to complete? If so - please add the comment.

1217 ↗

(On Diff #23936)

s/1/sizeof(operation_command)

[labath]

Respond to review comments.

source/Plugins/Process/Linux/NativeProcessLinux.cpp

1077 ↗

(On Diff #23936)

done.

1080 ↗

(On Diff #23936)

done.

1122 ↗

(On Diff #23936)

done.

1199 ↗

(On Diff #23936)

Affirmative. Added comment.

1217 ↗

(On Diff #23936)

done.

[tberghammer]

Looks good

lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.h

namespace process_linux {
 
     private:
 
         class Monitor;
 
         ArchSpec m_arch;
 
         std::unique_ptr<Monitor> m_monitor_up;
-        HostThread m_operation_thread;
-
-        // current operation which must be executed on the priviliged thread
-        void *m_operation;
-        Mutex m_operation_mutex;
-
-        // semaphores notified when Operation is ready to be processed and when
-        // the operation is complete.
-        sem_t m_operation_pending;
-        sem_t m_operation_done;
 
         LazyBool m_supports_mem_region;
         std::vector<MemoryRegionInfo> m_mem_region_cache;
         Mutex m_mem_region_cache_mutex;
 
         std::unique_ptr<ThreadStateCoordinator> m_coordinator_up;
         HostThread m_coordinator_thread;
 
         // List of thread ids stepping with a breakpoint with the address of
         // the relevan breakpoint
         std::map<lldb::tid_t, lldb::addr_t> m_threads_stepping_with_breakpoint;
 
-        struct OperationArgs
-        {
-            OperationArgs(NativeProcessLinux *monitor);
-
-            ~OperationArgs();
-
-            NativeProcessLinux *m_monitor;      // The monitor performing the attach.
-            sem_t m_semaphore;              // Posted to once operation complete.
-            Error m_error;    // Set if process operation failed.
-        };
-
         /// @class LauchArgs
         ///
         /// @brief Simple structure to pass data to the thread responsible for
         /// launching a child process.
-        struct LaunchArgs : OperationArgs
+        struct LaunchArgs
         {
-            LaunchArgs(NativeProcessLinux *monitor,
-                    Module *module,
+            LaunchArgs(Module *module,
                     char const **argv,
                     char const **envp,
                     const std::string &stdin_path,
                     const std::string &stdout_path,
                     const std::string &stderr_path,
                     const char *working_dir,
                     const ProcessLaunchInfo &launch_info);
 
             ~LaunchArgs();
 
             Module *m_module;                 // The executable image to launch.
             char const **m_argv;              // Process arguments.
             char const **m_envp;              // Process environment.
             const std::string &m_stdin_path;  // Redirect stdin if not empty.
             const std::string &m_stdout_path; // Redirect stdout if not empty.
             const std::string &m_stderr_path; // Redirect stderr if not empty.
             const char *m_working_dir;        // Working directory or NULL.
             const ProcessLaunchInfo &m_launch_info;
         };
 
-        struct AttachArgs : OperationArgs
-        {
-            AttachArgs(NativeProcessLinux *monitor,
-                       lldb::pid_t pid);
-
-            ~AttachArgs();
-
-            lldb::pid_t m_pid;              // pid of the process to be attached.
-        };
+        typedef std::function<::pid_t(Error &)> InitialOperation;
 
         // ---------------------------------------------------------------------
         // Private Instance Methods
         // ---------------------------------------------------------------------
         NativeProcessLinux ();
 
         /// Launches an inferior process ready for debugging.  Forms the
         /// implementation of Process::DoLaunch.
private:
             Error &error);
 
         /// Attaches to an existing process.  Forms the
         /// implementation of Process::DoAttach
         void
         AttachToInferior (lldb::pid_t pid, Error &error);
 
         void
-        StartMonitorThread(Error &error);
-
-        void
-        StartLaunchOpThread(LaunchArgs *args, Error &error);
+        StartMonitorThread(const InitialOperation &operation, Error &error);
 
-        static void *
-        LaunchOpThread(void *arg);
-
-        static bool
-        Launch(LaunchArgs *args);
+        ::pid_t
+        Launch(LaunchArgs *args, Error &error);
 
-        void
-        StartAttachOpThread(AttachArgs *args, Error &error);
-
-        static void *
-        AttachOpThread(void *args);
-
-        static bool
-        Attach(AttachArgs *args);
+        ::pid_t
+        Attach(lldb::pid_t pid, Error &error);
 
         static Error
         SetDefaultPtraceOpts(const lldb::pid_t);
 
-        static void
-        ServeOperation(OperationArgs *args);
-
         static bool
         DupDescriptor(const char *path, int fd, int flags);
 
         static void *
         MonitorThread(void *baton);
 
         void
         MonitorCallback(lldb::pid_t pid, bool exited, int signal, int status);
#if 0
 
         static ::ProcessMessage::CrashReason
         GetCrashReasonForSIGFPE(const siginfo_t *info);
 
         static ::ProcessMessage::CrashReason
         GetCrashReasonForSIGBUS(const siginfo_t *info);
 #endif
 
-        void
-        DoOperation(void *op);
-
-        /// Stops the operation thread used to attach/launch a process.
-        void
-        StopOpThread();
-
         Error
         StartCoordinatorThread ();
 
         static void*
         CoordinatorThread (void *arg);
 
         void
         StopCoordinatorThread ();

lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp

 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::process_linux;
 using namespace llvm;
 
 // Private bits we only need internally.
 namespace
 {
-    static void * const EXIT_OPERATION = nullptr;
-
     const UnixSignals&
     GetUnixSignals ()
     {
         static process_linux::LinuxSignals signals;
         return signals;
     }
 
     ThreadStateCoordinator::LogFunction
#endif
     void
     DetachOperation::Execute(NativeProcessLinux *monitor)
     {
         PTRACE(PTRACE_DETACH, m_tid, nullptr, 0, 0, m_error);
     }
 
 } // end of anonymous namespace
 
+// Simple helper function to ensure flags are enabled on the given file
+// descriptor.
+static Error
+EnsureFDFlags(int fd, int flags)
+{
+    Error error;
+
+    int status = fcntl(fd, F_GETFL);
+    if (status == -1)
+    {
+        error.SetErrorToErrno();
+        return error;
+    }
+
+    if (fcntl(fd, F_SETFL, status | flags) == -1)
+    {
+        error.SetErrorToErrno();
+        return error;
+    }
+
+    return error;
+}
+
+// This class encapsulates the privileged thread which performs all ptrace and wait operations on
+// the inferior. The thread consists of a main loop which waits for events and processes them
+//   - SIGCHLD (delivered over a signalfd file descriptor): These signals notify us of events in
+//     the inferior process. Upon receiving this signal we do a waitpid to get more information
+//     and dispatch to NativeProcessLinux::MonitorCallback.
+//   - requests for ptrace operations: These initiated via the DoOperation method, which funnels
+//     them to the Monitor thread via m_operation member. The Monitor thread is signaled over a
+//     pipe, and the completion of the operation is signalled over the semaphore.
+//   - thread exit event: this is signaled from the Monitor destructor by closing the write end
+//     of the command pipe.
 class NativeProcessLinux::Monitor {
 private:
-    ::pid_t             m_child_pid;
-    NativeProcessLinux *m_native_process;
+    // The initial monitor operation (launch or attach). It returns a inferior process id.
+    std::unique_ptr<InitialOperation> m_initial_operation_up;
+
+    ::pid_t                           m_child_pid = -1;
+    NativeProcessLinux              * m_native_process;
 
     enum { READ, WRITE };
     int        m_pipefd[2] = {-1, -1};
     int        m_signal_fd = -1;
     HostThread m_thread;
 
+    // current operation which must be executed on the priviliged thread
+    Mutex      m_operation_mutex;
+    Operation *m_operation = nullptr;
+    sem_t      m_operation_sem;
+    Error      m_operation_error;
+
+    static constexpr char operation_command = 'o';
+
     void
     HandleSignals();
 
     void
     HandleWait();
 
     // Returns true if the thread should exit.
     bool
     HandleCommands();
 
     void
     MainLoop();
 
     static void *
     RunMonitor(void *arg);
 
+    Error
+    WaitForOperation();
 public:
-    // Takes ownership of command_fd.
-    Monitor(::pid_t child_pid, NativeProcessLinux *native_process)
-        : m_child_pid(-getpgid(child_pid)), m_native_process(native_process)
-    {}
+    Monitor(const InitialOperation &initial_operation,
+            NativeProcessLinux *native_process)
+        : m_initial_operation_up(new InitialOperation(initial_operation)),
+          m_native_process(native_process)
+    {
+        sem_init(&m_operation_sem, 0, 0);
+    }
 
     ~Monitor();
 
     Error
     Initialize();
+
+    void
+    DoOperation(Operation *op);
 };
+constexpr char NativeProcessLinux::Monitor::operation_command;
 
 Error
 NativeProcessLinux::Monitor::Initialize()
 {
     Error error;
 
     // We get a SIGCHLD every time something interesting happens with the inferior. We shall be
     // listening for these signals over a signalfd file descriptors. This allows us to wait for
NativeProcessLinux::Monitor::Initialize()
     }
 
     if (pipe2(m_pipefd, O_CLOEXEC) == -1)
     {
         error.SetErrorToErrno();
         return error;
     }
 
-    m_thread = ThreadLauncher::LaunchThread("monitor", Monitor::RunMonitor, this, nullptr);
+    if ((error = EnsureFDFlags(m_pipefd[READ], O_NONBLOCK)).Fail()) {
+        return error;
+    }
+
+    static const char g_thread_name[] = "lldb.process.nativelinux.monitor";
+    m_thread = ThreadLauncher::LaunchThread(g_thread_name, Monitor::RunMonitor, this, nullptr);
     if (!m_thread.IsJoinable())
         return Error("Failed to create monitor thread for NativeProcessLinux.");
 
-    return error;
+    // Wait for initial operation to complete.
+    return WaitForOperation();
+}
+
+void
+NativeProcessLinux::Monitor::DoOperation(Operation *op)
+{
+    if (m_thread.EqualsThread(pthread_self())) {
+        // If we're on the Monitor thread, we can simply execute the operation.
+        op->Execute(m_native_process);
+        return;
+    }
+
+    // Otherwise we need to pass the operation to the Monitor thread so it can handle it.
+    Mutex::Locker lock(m_operation_mutex);
+
+    m_operation = op;
+
+    // notify the thread that an operation is ready to be processed
+    write(m_pipefd[WRITE], &operation_command, sizeof operation_command);
+
+    WaitForOperation();
 }
 
 NativeProcessLinux::Monitor::~Monitor()
 {
     if (m_pipefd[WRITE] >= 0)
         close(m_pipefd[WRITE]);
     if (m_thread.IsJoinable())
         m_thread.Join(nullptr);
     if (m_pipefd[READ] >= 0)
         close(m_pipefd[READ]);
     if (m_signal_fd >= 0)
         close(m_signal_fd);
+    sem_destroy(&m_operation_sem);
 }
 
 void
 NativeProcessLinux::Monitor::HandleSignals()
 {
     Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
 
     // We don't really care about the content of the SIGCHLD siginfo structure, as we will get
while (true)
             return true;
         }
         if (size == 0) // end of file - write end closed
         {
             if (log)
                 log->Printf("NativeProcessLinux::Monitor::%s exit command received, exiting...", __FUNCTION__);
             return true; // We are done.
         }
+
+        switch (command)
+        {
+        case operation_command:
+            m_operation->Execute(m_native_process);
+
+            // notify calling thread that operation is complete
+            sem_post(&m_operation_sem);
+            break;
+        default:
             if (log)
                 log->Printf("NativeProcessLinux::Monitor::%s received unknown command '%c'",
                         __FUNCTION__, command);
         }
     }
+}
 
 void
 NativeProcessLinux::Monitor::MainLoop()
 {
+    ::pid_t child_pid = (*m_initial_operation_up)(m_operation_error);
+    m_initial_operation_up.reset();
+    m_child_pid = -getpgid(child_pid),
+    sem_post(&m_operation_sem);
+
     while (true)
     {
         fd_set fds;
         FD_ZERO(&fds);
         FD_SET(m_signal_fd, &fds);
         FD_SET(m_pipefd[READ], &fds);
 
         int max_fd = std::max(m_signal_fd, m_pipefd[READ]) + 1;
while (true)
         if (FD_ISSET(m_signal_fd, &fds))
         {
             HandleSignals();
             HandleWait();
         }
     }
 }
 
-void *
-NativeProcessLinux::Monitor::RunMonitor(void *arg)
-{
-    static_cast<Monitor *>(arg)->MainLoop();
-    return nullptr;
-}
-
-
-// Simple helper function to ensure flags are enabled on the given file
-// descriptor.
-static bool
-EnsureFDFlags(int fd, int flags, Error &error)
+Error
+NativeProcessLinux::Monitor::WaitForOperation()
 {
-    int status;
-
-    if ((status = fcntl(fd, F_GETFL)) == -1)
+    Error error;
+    while (sem_wait(&m_operation_sem) != 0)
     {
-        error.SetErrorToErrno();
-        return false;
-    }
+        if (errno == EINTR)
+            continue;
 
-    if (fcntl(fd, F_SETFL, status | flags) == -1)
-    {
         error.SetErrorToErrno();
-        return false;
+        return error;
     }
 
-    return true;
+    return m_operation_error;
 }
 
-NativeProcessLinux::OperationArgs::OperationArgs(NativeProcessLinux *monitor)
-    : m_monitor(monitor)
+void *
+NativeProcessLinux::Monitor::RunMonitor(void *arg)
 {
-    sem_init(&m_semaphore, 0, 0);
+    static_cast<Monitor *>(arg)->MainLoop();
+    return nullptr;
 }
 
-NativeProcessLinux::OperationArgs::~OperationArgs()
-{
-    sem_destroy(&m_semaphore);
-}
 
-NativeProcessLinux::LaunchArgs::LaunchArgs(NativeProcessLinux *monitor,
-                                       Module *module,
+NativeProcessLinux::LaunchArgs::LaunchArgs(Module *module,
                                        char const **argv,
                                        char const **envp,
                                        const std::string &stdin_path,
                                        const std::string &stdout_path,
                                        const std::string &stderr_path,
                                        const char *working_dir,
                                        const ProcessLaunchInfo &launch_info)
-    : OperationArgs(monitor),
-      m_module(module),
+    : m_module(module),
       m_argv(argv),
       m_envp(envp),
       m_stdin_path(stdin_path),
       m_stdout_path(stdout_path),
       m_stderr_path(stderr_path),
       m_working_dir(working_dir),
       m_launch_info(launch_info)
 {
 }
 
 NativeProcessLinux::LaunchArgs::~LaunchArgs()
 { }
 
-NativeProcessLinux::AttachArgs::AttachArgs(NativeProcessLinux *monitor,
-                                       lldb::pid_t pid)
-    : OperationArgs(monitor), m_pid(pid) { }
-
-NativeProcessLinux::AttachArgs::~AttachArgs()
-{ }
-
 // -----------------------------------------------------------------------------
 // Public Static Methods
 // -----------------------------------------------------------------------------
 
 Error
 NativeProcessLinux::LaunchProcess (
     Module *exe_module,
     ProcessLaunchInfo &launch_info,

 
 // -----------------------------------------------------------------------------
 // Public Instance Methods
 // -----------------------------------------------------------------------------
 
 NativeProcessLinux::NativeProcessLinux () :
     NativeProcessProtocol (LLDB_INVALID_PROCESS_ID),
     m_arch (),
-    m_operation_thread (),
-    m_operation (nullptr),
-    m_operation_mutex (),
-    m_operation_pending (),
-    m_operation_done (),
     m_supports_mem_region (eLazyBoolCalculate),
     m_mem_region_cache (),
     m_mem_region_cache_mutex (),
     m_coordinator_up (new ThreadStateCoordinator (GetThreadLoggerFunction ())),
     m_coordinator_thread ()
 {
 }
 
 //------------------------------------------------------------------------------
-/// The basic design of the NativeProcessLinux is built around two threads.
-///
-/// One thread (@see SignalThread) simply blocks on a call to waitpid() looking
-/// for changes in the debugee state.  When a change is detected a
-/// ProcessMessage is sent to the associated ProcessLinux instance.  This thread
-/// "drives" state changes in the debugger.
-///
-/// The second thread (@see OperationThread) is responsible for two things 1)
-/// launching or attaching to the inferior process, and then 2) servicing
-/// operations such as register reads/writes, stepping, etc.  See the comments
-/// on the Operation class for more info as to why this is needed.
+// NativeProcessLinux spawns a new thread which performs all operations on the inferior process.
+// Refer to Monitor and Operation classes to see why this is necessary.
+//------------------------------------------------------------------------------
 void
 NativeProcessLinux::LaunchInferior (
     Module *module,
     const char *argv[],
     const char *envp[],
     const std::string &stdin_path,
     const std::string &stdout_path,
     const std::string &stderr_path,
     const char *working_dir,
     const ProcessLaunchInfo &launch_info,
     Error &error)
 {
     if (module)
         m_arch = module->GetArchitecture ();
 
     SetState (eStateLaunching);
 
     std::unique_ptr<LaunchArgs> args(
         new LaunchArgs(
-            this, module, argv, envp,
+            module, argv, envp,
             stdin_path, stdout_path, stderr_path,
             working_dir, launch_info));
 
-    sem_init (&m_operation_pending, 0, 0);
-    sem_init (&m_operation_done, 0, 0);
-
-    StartLaunchOpThread (args.get(), error);
+    StartMonitorThread ([&] (Error &e) { return Launch(args.get(), e); }, error);
     if (!error.Success ())
         return;
 
     error = StartCoordinatorThread ();
     if (!error.Success ())
         return;
-
-WAIT_AGAIN:
-    // Wait for the operation thread to initialize.
-    if (sem_wait(&args->m_semaphore))
-    {
-        if (errno == EINTR)
-            goto WAIT_AGAIN;
-        else
-        {
-            error.SetErrorToErrno();
-            return;
-        }
-    }
-
-    // Check that the launch was a success.
-    if (!args->m_error.Success())
-    {
-        StopOpThread();
-        StopCoordinatorThread ();
-        error = args->m_error;
-        return;
-    }
-
-    // Finally, start monitoring the child process for change in state.
-    StartMonitorThread(error);
 }
 
 void
 NativeProcessLinux::AttachToInferior (lldb::pid_t pid, Error &error)
 {
     Log *log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS));
     if (log)
         log->Printf ("NativeProcessLinux::%s (pid = %" PRIi64 ")", __FUNCTION__, pid);
NativeProcessLinux::AttachToInferior (lldb::pid_t pid, Error &error)
     // Set the architecture to the exe architecture.
     m_arch = exe_module_sp->GetArchitecture();
     if (log)
         log->Printf ("NativeProcessLinux::%s (pid = %" PRIi64 ") detected architecture %s", __FUNCTION__, pid, m_arch.GetArchitectureName ());
 
     m_pid = pid;
     SetState(eStateAttaching);
 
-    sem_init (&m_operation_pending, 0, 0);
-    sem_init (&m_operation_done, 0, 0);
-
-    std::unique_ptr<AttachArgs> args (new AttachArgs (this, pid));
-
-    StartAttachOpThread(args.get (), error);
+    StartMonitorThread ([=] (Error &e) { return Attach(pid, e); }, error);
     if (!error.Success ())
         return;
 
     error = StartCoordinatorThread ();
     if (!error.Success ())
         return;
-
-WAIT_AGAIN:
-    // Wait for the operation thread to initialize.
-    if (sem_wait (&args->m_semaphore))
-    {
-        if (errno == EINTR)
-            goto WAIT_AGAIN;
-        else
-        {
-            error.SetErrorToErrno ();
-            return;
-        }
-    }
-
-    // Check that the attach was a success.
-    if (!args->m_error.Success ())
-    {
-        StopOpThread ();
-        StopCoordinatorThread ();
-        error = args->m_error;
-        return;
-    }
-
-    // Finally, start monitoring the child process for change in state.
-    StartMonitorThread(error);
 }
 
 void
 NativeProcessLinux::Terminate ()
 {
     StopMonitor();
 }
 
-//------------------------------------------------------------------------------
-// Thread setup and tear down.
-
-void
-NativeProcessLinux::StartLaunchOpThread(LaunchArgs *args, Error &error)
-{
-    static const char *g_thread_name = "lldb.process.nativelinux.operation";
-
-    if (m_operation_thread.IsJoinable())
-        return;
-
-    m_operation_thread = ThreadLauncher::LaunchThread(g_thread_name, LaunchOpThread, args, &error);
-}
-
-void *
-NativeProcessLinux::LaunchOpThread(void *arg)
-{
-    LaunchArgs *args = static_cast<LaunchArgs*>(arg);
-
-    if (!Launch(args)) {
-        sem_post(&args->m_semaphore);
-        return NULL;
-    }
-
-    ServeOperation(args);
-    return NULL;
-}
-
-bool
-NativeProcessLinux::Launch(LaunchArgs *args)
+::pid_t
+NativeProcessLinux::Launch(LaunchArgs *args, Error &error)
 {
     assert (args && "null args");
-    if (!args)
-        return false;
-
-    NativeProcessLinux *monitor = args->m_monitor;
-    assert (monitor && "monitor is NULL");
 
     const char **argv = args->m_argv;
     const char **envp = args->m_envp;
     const char *working_dir = args->m_working_dir;
 
     lldb_utility::PseudoTerminal terminal;
     const size_t err_len = 1024;
     char err_str[err_len];
     lldb::pid_t pid;
     NativeThreadProtocolSP thread_sp;
 
     lldb::ThreadSP inferior;
 
     // Propagate the environment if one is not supplied.
     if (envp == NULL || envp[0] == NULL)
         envp = const_cast<const char **>(environ);
 
     if ((pid = terminal.Fork(err_str, err_len)) == static_cast<lldb::pid_t> (-1))
     {
-        args->m_error.SetErrorToGenericError();
-        args->m_error.SetErrorString("Process fork failed.");
-        return false;
+        error.SetErrorToGenericError();
+        error.SetErrorStringWithFormat("Process fork failed: %s", err_str);
+        return -1;
     }
 
     // Recognized child exit status codes.
     enum {
         ePtraceFailed = 1,
         eDupStdinFailed,
         eDupStdoutFailed,
         eDupStderrFailed,
         eChdirFailed,
         eExecFailed,
         eSetGidFailed
     };
 
     // Child process.
     if (pid == 0)
     {
         // FIXME consider opening a pipe between parent/child and have this forked child
         // send log info to parent re: launch status, in place of the log lines removed here.
 
         // Start tracing this child that is about to exec.
-        PTRACE(PTRACE_TRACEME, 0, nullptr, nullptr, 0, args->m_error);
-        if (args->m_error.Fail())
+        PTRACE(PTRACE_TRACEME, 0, nullptr, nullptr, 0, error);
+        if (error.Fail())
             exit(ePtraceFailed);
 
         // terminal has already dupped the tty descriptors to stdin/out/err.
         // This closes original fd from which they were copied (and avoids
         // leaking descriptors to the debugged process.
         terminal.CloseSlaveFileDescriptor();
 
         // Do not inherit setgid powers.
NativeProcessLinux::Launch(LaunchArgs *args, Error &error)
     //
     Log *log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS));
 
     // Wait for the child process to trap on its call to execve.
     ::pid_t wpid;
     int status;
     if ((wpid = waitpid(pid, &status, 0)) < 0)
     {
-        args->m_error.SetErrorToErrno();
-
+        error.SetErrorToErrno();
         if (log)
-            log->Printf ("NativeProcessLinux::%s waitpid for inferior failed with %s", __FUNCTION__, args->m_error.AsCString ());
+            log->Printf ("NativeProcessLinux::%s waitpid for inferior failed with %s",
+                    __FUNCTION__, error.AsCString ());
 
         // Mark the inferior as invalid.
         // FIXME this could really use a new state - eStateLaunchFailure.  For now, using eStateInvalid.
-        monitor->SetState (StateType::eStateInvalid);
+        SetState (StateType::eStateInvalid);
 
-        return false;
+        return -1;
     }
     else if (WIFEXITED(status))
     {
         // open, dup or execve likely failed for some reason.
-        args->m_error.SetErrorToGenericError();
+        error.SetErrorToGenericError();
         switch (WEXITSTATUS(status))
         {
             case ePtraceFailed:
-                args->m_error.SetErrorString("Child ptrace failed.");
+                error.SetErrorString("Child ptrace failed.");
                 break;
             case eDupStdinFailed:
-                args->m_error.SetErrorString("Child open stdin failed.");
+                error.SetErrorString("Child open stdin failed.");
                 break;
             case eDupStdoutFailed:
-                args->m_error.SetErrorString("Child open stdout failed.");
+                error.SetErrorString("Child open stdout failed.");
                 break;
             case eDupStderrFailed:
-                args->m_error.SetErrorString("Child open stderr failed.");
+                error.SetErrorString("Child open stderr failed.");
                 break;
             case eChdirFailed:
-                args->m_error.SetErrorString("Child failed to set working directory.");
+                error.SetErrorString("Child failed to set working directory.");
                 break;
             case eExecFailed:
-                args->m_error.SetErrorString("Child exec failed.");
+                error.SetErrorString("Child exec failed.");
                 break;
             case eSetGidFailed:
-                args->m_error.SetErrorString("Child setgid failed.");
+                error.SetErrorString("Child setgid failed.");
                 break;
             default:
-                args->m_error.SetErrorString("Child returned unknown exit status.");
+                error.SetErrorString("Child returned unknown exit status.");
                 break;
         }
 
         if (log)
         {
             log->Printf ("NativeProcessLinux::%s inferior exited with status %d before issuing a STOP",
                     __FUNCTION__,
                     WEXITSTATUS(status));
         }
 
         // Mark the inferior as invalid.
         // FIXME this could really use a new state - eStateLaunchFailure.  For now, using eStateInvalid.
-        monitor->SetState (StateType::eStateInvalid);
+        SetState (StateType::eStateInvalid);
 
-        return false;
+        return -1;
     }
     assert(WIFSTOPPED(status) && (wpid == static_cast< ::pid_t> (pid)) &&
            "Could not sync with inferior process.");
 
     if (log)
         log->Printf ("NativeProcessLinux::%s inferior started, now in stopped state", __FUNCTION__);
 
-    args->m_error = SetDefaultPtraceOpts(pid);
-    if (args->m_error.Fail())
+    error = SetDefaultPtraceOpts(pid);
+    if (error.Fail())
     {
         if (log)
             log->Printf ("NativeProcessLinux::%s inferior failed to set default ptrace options: %s",
-                    __FUNCTION__,
-                    args->m_error.AsCString ());
+                    __FUNCTION__, error.AsCString ());
 
         // Mark the inferior as invalid.
         // FIXME this could really use a new state - eStateLaunchFailure.  For now, using eStateInvalid.
-        monitor->SetState (StateType::eStateInvalid);
+        SetState (StateType::eStateInvalid);
 
-        return false;
+        return -1;
     }
 
     // Release the master terminal descriptor and pass it off to the
     // NativeProcessLinux instance.  Similarly stash the inferior pid.
-    monitor->m_terminal_fd = terminal.ReleaseMasterFileDescriptor();
-    monitor->m_pid = pid;
+    m_terminal_fd = terminal.ReleaseMasterFileDescriptor();
+    m_pid = pid;
 
     // Set the terminal fd to be in non blocking mode (it simplifies the
     // implementation of ProcessLinux::GetSTDOUT to have a non-blocking
     // descriptor to read from).
-    if (!EnsureFDFlags(monitor->m_terminal_fd, O_NONBLOCK, args->m_error))
+    error = EnsureFDFlags(m_terminal_fd, O_NONBLOCK);
+    if (error.Fail())
     {
         if (log)
             log->Printf ("NativeProcessLinux::%s inferior EnsureFDFlags failed for ensuring terminal O_NONBLOCK setting: %s",
-                    __FUNCTION__,
-                    args->m_error.AsCString ());
+                    __FUNCTION__, error.AsCString ());
 
         // Mark the inferior as invalid.
         // FIXME this could really use a new state - eStateLaunchFailure.  For now, using eStateInvalid.
-        monitor->SetState (StateType::eStateInvalid);
+        SetState (StateType::eStateInvalid);
 
-        return false;
+        return -1;
     }
 
     if (log)
         log->Printf ("NativeProcessLinux::%s() adding pid = %" PRIu64, __FUNCTION__, pid);
 
-    thread_sp = monitor->AddThread (pid);
+    thread_sp = AddThread (pid);
     assert (thread_sp && "AddThread() returned a nullptr thread");
-    monitor->NotifyThreadCreateStopped (pid);
+    NotifyThreadCreateStopped (pid);
     std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetStoppedBySignal (SIGSTOP);
 
     // Let our process instance know the thread has stopped.
-    monitor->SetCurrentThreadID (thread_sp->GetID ());
-    monitor->SetState (StateType::eStateStopped);
+    SetCurrentThreadID (thread_sp->GetID ());
+    SetState (StateType::eStateStopped);
 
     if (log)
     {
-        if (args->m_error.Success ())
+        if (error.Success ())
         {
             log->Printf ("NativeProcessLinux::%s inferior launching succeeded", __FUNCTION__);
         }
         else
         {
             log->Printf ("NativeProcessLinux::%s inferior launching failed: %s",
-                __FUNCTION__,
-                args->m_error.AsCString ());
+                __FUNCTION__, error.AsCString ());
+            return -1;
         }
     }
-    return args->m_error.Success();
-}
-
-void
-NativeProcessLinux::StartAttachOpThread(AttachArgs *args, Error &error)
-{
-    static const char *g_thread_name = "lldb.process.linux.operation";
-
-    if (m_operation_thread.IsJoinable())
-        return;
-
-    m_operation_thread = ThreadLauncher::LaunchThread(g_thread_name, AttachOpThread, args, &error);
+    return pid;
 }
 
-void *
-NativeProcessLinux::AttachOpThread(void *arg)
+::pid_t
+NativeProcessLinux::Attach(lldb::pid_t pid, Error &error)
 {
-    AttachArgs *args = static_cast<AttachArgs*>(arg);
-
-    if (!Attach(args)) {
-        sem_post(&args->m_semaphore);
-        return nullptr;
-    }
-
-    ServeOperation(args);
-    return nullptr;
-}
-
-bool
-NativeProcessLinux::Attach(AttachArgs *args)
-{
-    lldb::pid_t pid = args->m_pid;
-
-    NativeProcessLinux *monitor = args->m_monitor;
     lldb::ThreadSP inferior;
     Log *log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS));
 
     // Use a map to keep track of the threads which we have attached/need to attach.
     Host::TidMap tids_to_attach;
     if (pid <= 1)
     {
-        args->m_error.SetErrorToGenericError();
-        args->m_error.SetErrorString("Attaching to process 1 is not allowed.");
-        goto FINISH;
+        error.SetErrorToGenericError();
+        error.SetErrorString("Attaching to process 1 is not allowed.");
+        return -1;
     }
 
     while (Host::FindProcessThreads(pid, tids_to_attach))
     {
         for (Host::TidMap::iterator it = tids_to_attach.begin();
              it != tids_to_attach.end();)
         {
             if (it->second == false)
             {
                 lldb::tid_t tid = it->first;
 
                 // Attach to the requested process.
                 // An attach will cause the thread to stop with a SIGSTOP.
-                PTRACE(PTRACE_ATTACH, tid, nullptr, nullptr, 0, args->m_error);
-                if (args->m_error.Fail())
+                PTRACE(PTRACE_ATTACH, tid, nullptr, nullptr, 0, error);
+                if (error.Fail())
                 {
                     // No such thread. The thread may have exited.
                     // More error handling may be needed.
-                    if (args->m_error.GetError() == ESRCH)
+                    if (error.GetError() == ESRCH)
                     {
                         it = tids_to_attach.erase(it);
                         continue;
                     }
                     else
-                        goto FINISH;
+                        return -1;
                 }
 
                 int status;
                 // Need to use __WALL otherwise we receive an error with errno=ECHLD
                 // At this point we should have a thread stopped if waitpid succeeds.
                 if ((status = waitpid(tid, NULL, __WALL)) < 0)
                 {
                     // No such thread. The thread may have exited.
                     // More error handling may be needed.
                     if (errno == ESRCH)
                     {
                         it = tids_to_attach.erase(it);
                         continue;
                     }
                     else
                     {
-                        args->m_error.SetErrorToErrno();
-                        goto FINISH;
+                        error.SetErrorToErrno();
+                        return -1;
                     }
                 }
 
-                args->m_error = SetDefaultPtraceOpts(tid);
-                if (args->m_error.Fail())
-                    goto FINISH;
-
+                error = SetDefaultPtraceOpts(tid);
+                if (error.Fail())
+                    return -1;
 
                 if (log)
                     log->Printf ("NativeProcessLinux::%s() adding tid = %" PRIu64, __FUNCTION__, tid);
 
                 it->second = true;
 
                 // Create the thread, mark it as stopped.
-                NativeThreadProtocolSP thread_sp (monitor->AddThread (static_cast<lldb::tid_t> (tid)));
+                NativeThreadProtocolSP thread_sp (AddThread (static_cast<lldb::tid_t> (tid)));
                 assert (thread_sp && "AddThread() returned a nullptr");
 
                 // This will notify this is a new thread and tell the system it is stopped.
-                monitor->NotifyThreadCreateStopped (tid);
+                NotifyThreadCreateStopped (tid);
                 std::static_pointer_cast<NativeThreadLinux> (thread_sp)->SetStoppedBySignal (SIGSTOP);
-                monitor->SetCurrentThreadID (thread_sp->GetID ());
+                SetCurrentThreadID (thread_sp->GetID ());
             }
 
             // move the loop forward
             ++it;
         }
     }
 
     if (tids_to_attach.size() > 0)
     {
-        monitor->m_pid = pid;
+        m_pid = pid;
         // Let our process instance know the thread has stopped.
-        monitor->SetState (StateType::eStateStopped);
+        SetState (StateType::eStateStopped);
     }
     else
     {
-        args->m_error.SetErrorToGenericError();
-        args->m_error.SetErrorString("No such process.");
+        error.SetErrorToGenericError();
+        error.SetErrorString("No such process.");
+        return -1;
     }
 
- FINISH:
-    return args->m_error.Success();
+    return pid;
 }
 
 Error
 NativeProcessLinux::SetDefaultPtraceOpts(lldb::pid_t pid)
 {
     long ptrace_opts = 0;
 
     // Have the child raise an event on exit.  This is used to keep the child in
case BUS_OBJERR:
         reason = ProcessMessage::eHardwareError;
         break;
     }
 
     return reason;
 }
 #endif
 
-void
-NativeProcessLinux::ServeOperation(OperationArgs *args)
-{
-    NativeProcessLinux *monitor = args->m_monitor;
-
-    // We are finised with the arguments and are ready to go.  Sync with the
-    // parent thread and start serving operations on the inferior.
-    sem_post(&args->m_semaphore);
-
-    for(;;)
-    {
-        // wait for next pending operation
-        if (sem_wait(&monitor->m_operation_pending))
-        {
-            if (errno == EINTR)
-                continue;
-            assert(false && "Unexpected errno from sem_wait");
-        }
-
-        // EXIT_OPERATION used to stop the operation thread because Cancel() isn't supported on
-        // android. We don't have to send a post to the m_operation_done semaphore because in this
-        // case the synchronization is achieved by a Join() call
-        if (monitor->m_operation == EXIT_OPERATION)
-            break;
-
-        static_cast<Operation*>(monitor->m_operation)->Execute(monitor);
-
-        // notify calling thread that operation is complete
-        sem_post(&monitor->m_operation_done);
-    }
-}
-
-void
-NativeProcessLinux::DoOperation(void *op)
-{
-    Mutex::Locker lock(m_operation_mutex);
-
-    m_operation = op;
-
-    // notify operation thread that an operation is ready to be processed
-    sem_post(&m_operation_pending);
-
-    // Don't wait for the operation to complete in case of an exit operation. The operation thread
-    // will exit without posting to the semaphore
-    if (m_operation == EXIT_OPERATION)
-        return;
-
-    // wait for operation to complete
-    while (sem_wait(&m_operation_done))
-    {
-        if (errno == EINTR)
-            continue;
-        assert(false && "Unexpected errno from sem_wait");
-    }
-}
-
 Error
 NativeProcessLinux::ReadMemory (lldb::addr_t addr, void *buf, lldb::addr_t size, lldb::addr_t &bytes_read)
 {
     ReadOperation op(addr, buf, size, bytes_read);
-    DoOperation(&op);
+    m_monitor_up->DoOperation(&op);
     return op.GetError ();
 }
 
 Error
 NativeProcessLinux::WriteMemory (lldb::addr_t addr, const void *buf, lldb::addr_t size, lldb::addr_t &bytes_written)
 {
     WriteOperation op(addr, buf, size, bytes_written);
-    DoOperation(&op);
+    m_monitor_up->DoOperation(&op);
     return op.GetError ();
 }
 
 Error
 NativeProcessLinux::ReadRegisterValue(lldb::tid_t tid, uint32_t offset, const char* reg_name,
                                       uint32_t size, RegisterValue &value)
 {
     ReadRegOperation op(tid, offset, reg_name, value);
-    DoOperation(&op);
+    m_monitor_up->DoOperation(&op);
     return op.GetError();
 }
 
 Error
 NativeProcessLinux::WriteRegisterValue(lldb::tid_t tid, unsigned offset,
                                    const char* reg_name, const RegisterValue &value)
 {
     WriteRegOperation op(tid, offset, reg_name, value);
-    DoOperation(&op);
+    m_monitor_up->DoOperation(&op);
     return op.GetError();
 }
 
 Error
 NativeProcessLinux::ReadGPR(lldb::tid_t tid, void *buf, size_t buf_size)
 {
     ReadGPROperation op(tid, buf, buf_size);
-    DoOperation(&op);
+    m_monitor_up->DoOperation(&op);
     return op.GetError();
 }
 
 Error
 NativeProcessLinux::ReadFPR(lldb::tid_t tid, void *buf, size_t buf_size)
 {
     ReadFPROperation op(tid, buf, buf_size);
-    DoOperation(&op);
+    m_monitor_up->DoOperation(&op);
     return op.GetError();
 }
 
 Error
 NativeProcessLinux::ReadRegisterSet(lldb::tid_t tid, void *buf, size_t buf_size, unsigned int regset)
 {
     ReadRegisterSetOperation op(tid, buf, buf_size, regset);
-    DoOperation(&op);
+    m_monitor_up->DoOperation(&op);
     return op.GetError();
 }
 
 Error
 NativeProcessLinux::WriteGPR(lldb::tid_t tid, void *buf, size_t buf_size)
 {
     WriteGPROperation op(tid, buf, buf_size);
-    DoOperation(&op);
+    m_monitor_up->DoOperation(&op);
     return op.GetError();
 }
 
 Error
 NativeProcessLinux::WriteFPR(lldb::tid_t tid, void *buf, size_t buf_size)
 {
     WriteFPROperation op(tid, buf, buf_size);
-    DoOperation(&op);
+    m_monitor_up->DoOperation(&op);
     return op.GetError();
 }
 
 Error
 NativeProcessLinux::WriteRegisterSet(lldb::tid_t tid, void *buf, size_t buf_size, unsigned int regset)
 {
     WriteRegisterSetOperation op(tid, buf, buf_size, regset);
-    DoOperation(&op);
+    m_monitor_up->DoOperation(&op);
     return op.GetError();
 }
 
 Error
 NativeProcessLinux::Resume (lldb::tid_t tid, uint32_t signo)
 {
     Log *log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS));
 
     if (log)
         log->Printf ("NativeProcessLinux::%s() resuming thread = %"  PRIu64 " with signal %s", __FUNCTION__, tid,
                                  GetUnixSignals().GetSignalAsCString (signo));
     ResumeOperation op (tid, signo);
-    DoOperation (&op);
+    m_monitor_up->DoOperation (&op);
     if (log)
         log->Printf ("NativeProcessLinux::%s() resuming thread = %"  PRIu64 " result = %s", __FUNCTION__, tid, op.GetError().Success() ? "true" : "false");
     return op.GetError();
 }
 
 #if defined(__arm__)
 
 namespace {

 }
 
 #else // defined(__arm__)
 
 Error
 NativeProcessLinux::SingleStep(lldb::tid_t tid, uint32_t signo)
 {
     SingleStepOperation op(tid, signo);
-    DoOperation(&op);
+    m_monitor_up->DoOperation(&op);
     return op.GetError();
 }
 
 #endif // defined(__arm__)
 
 Error
 NativeProcessLinux::GetSignalInfo(lldb::tid_t tid, void *siginfo)
 {
     SiginfoOperation op(tid, siginfo);
-    DoOperation(&op);
+    m_monitor_up->DoOperation(&op);
     return op.GetError();
 }
 
 Error
 NativeProcessLinux::GetEventMessage(lldb::tid_t tid, unsigned long *message)
 {
     EventMessageOperation op(tid, message);
-    DoOperation(&op);
+    m_monitor_up->DoOperation(&op);
     return op.GetError();
 }
 
 Error
 NativeProcessLinux::Detach(lldb::tid_t tid)
 {
     if (tid == LLDB_INVALID_THREAD_ID)
         return Error();
 
     DetachOperation op(tid);
-    DoOperation(&op);
+    m_monitor_up->DoOperation(&op);
     return op.GetError();
 }
 
 bool
 NativeProcessLinux::DupDescriptor(const char *path, int fd, int flags)
 {
     int target_fd = open(path, flags, 0666);
 
     if (target_fd == -1)
         return false;
 
     if (dup2(target_fd, fd) == -1)
         return false;
 
     return (close(target_fd) == -1) ? false : true;
 }
 
 void
-NativeProcessLinux::StartMonitorThread(Error &error)
+NativeProcessLinux::StartMonitorThread(const InitialOperation &initial_operation, Error &error)
 {
-    m_monitor_up.reset(new Monitor(GetID(), this));
+    m_monitor_up.reset(new Monitor(initial_operation, this));
     error = m_monitor_up->Initialize();
     if (error.Fail()) {
         m_monitor_up.reset();
     }
 }
 
 void
 NativeProcessLinux::StopMonitor()
 {
-    m_monitor_up.reset();
     StopCoordinatorThread ();
-    StopOpThread();
-    sem_destroy(&m_operation_pending);
-    sem_destroy(&m_operation_done);
-
-    // TODO: validate whether this still holds, fix up comment.
-    // Note: ProcessPOSIX passes the m_terminal_fd file descriptor to
-    // Process::SetSTDIOFileDescriptor, which in turn transfers ownership of
-    // the descriptor to a ConnectionFileDescriptor object.  Consequently
-    // even though still has the file descriptor, we shouldn't close it here.
-}
-
-void
-NativeProcessLinux::StopOpThread()
-{
-    if (!m_operation_thread.IsJoinable())
-        return;
-
-    DoOperation(EXIT_OPERATION);
-    m_operation_thread.Join(nullptr);
+    m_monitor_up.reset();
 }
 
 Error
 NativeProcessLinux::StartCoordinatorThread ()
 {
     Error error;
     static const char *g_thread_name = "lldb.process.linux.ts_coordinator";
     Log *const log (GetLogIfAllCategoriesSet (LIBLLDB_LOG_THREAD));

lldb/trunk/test/functionalities/thread/concurrent_events/TestConcurrentEvents.py

class ConcurrentEventsTestCase(TestBase):
     #
     @dwarf_test
     def test_signal_break_dwarf(self):
         """Test signal and a breakpoint in multiple threads."""
         self.buildDwarf(dictionary=self.getBuildFlags())
         self.do_thread_actions(num_breakpoint_threads=1, num_signal_threads=1)
 
     @dwarf_test
-    @expectedFailureLinux # this test fails 2/100 dosep runs
     def test_delay_signal_break_dwarf(self):
         """Test (1-second delay) signal and a breakpoint in multiple threads."""
         self.buildDwarf(dictionary=self.getBuildFlags())
         self.do_thread_actions(num_breakpoint_threads=1, num_delay_signal_threads=1)
 
     @dwarf_test
     def test_signal_delay_break_dwarf(self):
         """Test signal and a (1 second delay) breakpoint in multiple threads."""