[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