[Lldb-commits] [lldb] r190820 - Improve stability of Linux ProcessMonitor by not using fds for synchronization:

Daniel Malea daniel.malea at intel.com
Mon Sep 16 16:12:18 PDT 2013


Author: dmalea
Date: Mon Sep 16 18:12:18 2013
New Revision: 190820

URL: http://llvm.org/viewvc/llvm-project?rev=190820&view=rev
Log:
Improve stability of Linux ProcessMonitor by not using fds for synchronization:
- ProcessMonitor::[Do|Serve]Operation no longer depend on file descriptors!
- removed unused member functions CloseFD and EnableIPC
- add semaphores to signal when an Operation is ready to be processed/complete.

This commit fixes a bug that was identified under stress-testing (i.e. build
LLVM while running tests) that led to LLDB becoming unresponsive because the
read/write operations on file descriptors in ProcessMonitor were not checked.

Other test runner improvement/convenience:
- pickup environment variables LLDB_LINUX_LOG and LLDB_LINUX_LOG_OPTIONS to
  enable (Linux) logging when running the test suite. Example usage:

        $ LLDB_LINUX_LOG="mylog.txt" LLDB_LINUX_LOG_OPTIONS="process thread" python dotest.py



Modified:
    lldb/trunk/source/Plugins/Process/Linux/ProcessMonitor.cpp
    lldb/trunk/source/Plugins/Process/Linux/ProcessMonitor.h
    lldb/trunk/test/dotest.py

Modified: lldb/trunk/source/Plugins/Process/Linux/ProcessMonitor.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/Linux/ProcessMonitor.cpp?rev=190820&r1=190819&r2=190820&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/Linux/ProcessMonitor.cpp (original)
+++ lldb/trunk/source/Plugins/Process/Linux/ProcessMonitor.cpp Mon Sep 16 18:12:18 2013
@@ -37,7 +37,6 @@
 #include "ProcessPOSIXLog.h"
 #include "ProcessMonitor.h"
 
-
 #define DEBUG_PTRACE_MAXBYTES 20
 
 // Support ptrace extensions even when compiled without required kernel support
@@ -937,20 +936,14 @@ ProcessMonitor::ProcessMonitor(ProcessPO
       m_monitor_thread(LLDB_INVALID_HOST_THREAD),
       m_pid(LLDB_INVALID_PROCESS_ID),
       m_terminal_fd(-1),
-      m_client_fd(-1),
-      m_server_fd(-1)
+      m_operation(0)
 {
-    std::unique_ptr<LaunchArgs> args;
-
-    args.reset(new LaunchArgs(this, module, argv, envp,
-                              stdin_path, stdout_path, stderr_path, working_dir));
+    std::unique_ptr<LaunchArgs> args(new LaunchArgs(this, module, argv, envp,
+                                     stdin_path, stdout_path, stderr_path,
+                                     working_dir));
 
-    // Server/client descriptors.
-    if (!EnableIPC())
-    {
-        error.SetErrorToGenericError();
-        error.SetErrorString("Monitor failed to initialize.");
-    }
+    sem_init(&m_operation_pending, 0, 0);
+    sem_init(&m_operation_done, 0, 0);
 
     StartLaunchOpThread(args.get(), error);
     if (!error.Success())
@@ -996,20 +989,12 @@ ProcessMonitor::ProcessMonitor(ProcessPO
       m_monitor_thread(LLDB_INVALID_HOST_THREAD),
       m_pid(LLDB_INVALID_PROCESS_ID),
       m_terminal_fd(-1),
-
-      m_client_fd(-1),
-      m_server_fd(-1)
+      m_operation(0)
 {
-    std::unique_ptr<AttachArgs> args;
+    sem_init(&m_operation_pending, 0, 0);
+    sem_init(&m_operation_done, 0, 0);
 
-    args.reset(new AttachArgs(this, pid));
-
-    // Server/client descriptors.
-    if (!EnableIPC())
-    {
-        error.SetErrorToGenericError();
-        error.SetErrorString("Monitor failed to initialize.");
-    }
+    std::unique_ptr<AttachArgs> args(new AttachArgs(this, pid));
 
     StartAttachOpThread(args.get(), error);
     if (!error.Success())
@@ -1246,19 +1231,6 @@ FINISH:
     return args->m_error.Success();
 }
 
-bool
-ProcessMonitor::EnableIPC()
-{
-    int fd[2];
-
-    if (socketpair(AF_UNIX, SOCK_STREAM, 0, fd))
-        return false;
-
-    m_client_fd = fd[0];
-    m_server_fd = fd[1];
-    return true;
-}
-
 void
 ProcessMonitor::StartAttachOpThread(AttachArgs *args, lldb_private::Error &error)
 {
@@ -1926,78 +1898,36 @@ void
 ProcessMonitor::ServeOperation(OperationArgs *args)
 {
     int status;
-    pollfd fdset;
 
     ProcessMonitor *monitor = args->m_monitor;
 
-    fdset.fd = monitor->m_server_fd;
-    fdset.events = POLLIN | POLLPRI;
-    fdset.revents = 0;
-
     // 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 (;;)
-    {
-        if ((status = poll(&fdset, 1, -1)) < 0)
-        {
-            switch (errno)
-            {
-            default:
-                assert(false && "Unexpected poll() failure!");
-                continue;
-
-            case EINTR: continue; // Just poll again.
-            case EBADF: return;   // Connection terminated.
-            }
-        }
+    for(;;) {
+        // wait for next pending operation
+        sem_wait(&monitor->m_operation_pending);
 
-        assert(status == 1 && "Too many descriptors!");
+        monitor->m_operation->Execute(monitor);
 
-        if (fdset.revents & POLLIN)
-        {
-            Operation *op = NULL;
-
-        READ_AGAIN:
-            if ((status = read(fdset.fd, &op, sizeof(op))) < 0)
-            {
-                // There is only one acceptable failure.
-                assert(errno == EINTR);
-                goto READ_AGAIN;
-            }
-            if (status == 0)
-                continue; // Poll again. The connection probably terminated.
-            assert(status == sizeof(op));
-            op->Execute(monitor);
-            write(fdset.fd, &op, sizeof(op));
-        }
+        // notify calling thread that operation is complete
+        sem_post(&monitor->m_operation_done);
     }
 }
 
 void
 ProcessMonitor::DoOperation(Operation *op)
 {
-    int status;
-    Operation *ack = NULL;
-    Mutex::Locker lock(m_server_mutex);
+    Mutex::Locker lock(m_operation_mutex);
 
-    // FIXME: Do proper error checking here.
-    write(m_client_fd, &op, sizeof(op));
+    m_operation = op;
 
-READ_AGAIN:
-    if ((status = read(m_client_fd, &ack, sizeof(ack))) < 0)
-    {
-        // If interrupted by a signal handler try again.  Otherwise the monitor
-        // thread probably died and we have a stale file descriptor -- abort the
-        // operation.
-        if (errno == EINTR)
-            goto READ_AGAIN;
-        return;
-    }
+    // notify operation thread that an operation is ready to be processed
+    sem_post(&m_operation_pending);
 
-    assert(status == sizeof(ack));
-    assert(ack == op && "Invalid monitor thread response!");
+    // wait for operation to complete
+    sem_wait(&m_operation_done);
 }
 
 size_t
@@ -2187,8 +2117,9 @@ ProcessMonitor::StopMonitor()
 {
     StopMonitoringChildProcess();
     StopOpThread();
-    CloseFD(m_client_fd);
-    CloseFD(m_server_fd);
+    sem_destroy(&m_operation_pending);
+    sem_destroy(&m_operation_done);
+
     // 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
@@ -2207,13 +2138,3 @@ ProcessMonitor::StopOpThread()
     Host::ThreadJoin(m_operation_thread, &result, NULL);
     m_operation_thread = LLDB_INVALID_HOST_THREAD;
 }
-
-void
-ProcessMonitor::CloseFD(int &fd)
-{
-    if (fd != -1)
-    {
-        close(fd);
-        fd = -1;
-    }
-}

Modified: lldb/trunk/source/Plugins/Process/Linux/ProcessMonitor.h
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/Linux/ProcessMonitor.h?rev=190820&r1=190819&r2=190820&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/Process/Linux/ProcessMonitor.h (original)
+++ lldb/trunk/source/Plugins/Process/Linux/ProcessMonitor.h Mon Sep 16 18:12:18 2013
@@ -193,10 +193,15 @@ private:
     lldb::pid_t m_pid;
     int m_terminal_fd;
 
+    // current operation which must be executed on the priviliged thread
+    Operation *m_operation;
+    lldb_private::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;
 
-    lldb_private::Mutex m_server_mutex;
-    int m_client_fd;
-    int m_server_fd;
 
     struct OperationArgs
     {
@@ -244,9 +249,6 @@ private:
     static bool
     Launch(LaunchArgs *args);
 
-    bool
-    EnableIPC();
-
     struct AttachArgs : OperationArgs
     {
         AttachArgs(ProcessMonitor *monitor,
@@ -309,9 +311,6 @@ private:
     /// Stops the operation thread used to attach/launch a process.
     void
     StopOpThread();
-
-    void
-    CloseFD(int &fd);
 };
 
 #endif // #ifndef liblldb_ProcessMonitor_H_

Modified: lldb/trunk/test/dotest.py
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/test/dotest.py?rev=190820&r1=190819&r2=190820&view=diff
==============================================================================
--- lldb/trunk/test/dotest.py (original)
+++ lldb/trunk/test/dotest.py Mon Sep 16 18:12:18 2013
@@ -1094,6 +1094,7 @@ def lldbLoggings():
     ci = lldb.DBG.GetCommandInterpreter()
     res = lldb.SBCommandReturnObject()
     if ("LLDB_LOG" in os.environ):
+        open(os.environ["LLDB_LOG"], 'w').close()
         if ("LLDB_LOG_OPTION" in os.environ):
             lldb_log_option = os.environ["LLDB_LOG_OPTION"]
         else:
@@ -1103,6 +1104,19 @@ def lldbLoggings():
             res)
         if not res.Succeeded():
             raise Exception('log enable failed (check LLDB_LOG env variable.')
+
+    if ("LLDB_LINUX_LOG" in os.environ):
+        open(os.environ["LLDB_LINUX_LOG"], 'w').close()
+        if ("LLDB_LINUX_LOG_OPTION" in os.environ):
+            lldb_log_option = os.environ["LLDB_LINUX_LOG_OPTION"]
+        else:
+            lldb_log_option = "event process expr state api"
+        ci.HandleCommand(
+            "log enable -n -f " + os.environ["LLDB_LINUX_LOG"] + " linux " + lldb_log_option,
+            res)
+        if not res.Succeeded():
+            raise Exception('log enable failed (check LLDB_LINUX_LOG env variable.')
+ 
     # Ditto for gdb-remote logging if ${GDB_REMOTE_LOG} environment variable is defined.
     # Use ${GDB_REMOTE_LOG} to specify the log file.
     if ("GDB_REMOTE_LOG" in os.environ):





More information about the lldb-commits mailing list