Skip to content

Commit 194357c

Browse files
committedMay 12, 2016
Fix a race in ProcessGDBRemote::MonitorDebugServerProcess
Summary: MonitorDebugServerProcess went to a lot of effort to make sure its asynchronous invocation does not cause any mischief, but it was still not race-free. Specifically, in a quick stop-restart sequence (like the one in TestAddressBreakpoints) the copying of the process shared pointer via target_sp->GetProcessSP() was racing with the resetting of the pointer in DeleteCurrentProcess, as they were both accessing the same shared_ptr object. To avoid this, I simply pass in a weak_ptr to the process when the callback is created. Locking this pointer is race-free as they are two separate object even though they point to the same process instance. This also removes the need for the complicated tap-dance around retrieving the process pointer. Reviewers: clayborg Subscribers: tberghammer, lldb-commits Differential Revision: http://reviews.llvm.org/D20107 llvm-svn: 269281
1 parent 55d3833 commit 194357c

File tree

2 files changed

+42
-69
lines changed

2 files changed

+42
-69
lines changed
 

‎lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

+40-68
Original file line numberDiff line numberDiff line change
@@ -3597,7 +3597,8 @@ ProcessGDBRemote::LaunchAndConnectToDebugserver (const ProcessInfo &process_info
35973597
// special terminal key sequences (^C) don't affect debugserver.
35983598
debugserver_launch_info.SetLaunchInSeparateProcessGroup(true);
35993599

3600-
debugserver_launch_info.SetMonitorProcessCallback(std::bind(MonitorDebugserverProcess, this, _1, _2, _3, _4),
3600+
const std::weak_ptr<ProcessGDBRemote> this_wp = std::static_pointer_cast<ProcessGDBRemote>(shared_from_this());
3601+
debugserver_launch_info.SetMonitorProcessCallback(std::bind(MonitorDebugserverProcess, this_wp, _1, _2, _3, _4),
36013602
false);
36023603
debugserver_launch_info.SetUserID(process_info.GetUserID());
36033604

@@ -3660,87 +3661,58 @@ ProcessGDBRemote::LaunchAndConnectToDebugserver (const ProcessInfo &process_info
36603661
}
36613662

36623663
bool
3663-
ProcessGDBRemote::MonitorDebugserverProcess(ProcessGDBRemote *process, lldb::pid_t debugserver_pid,
3664+
ProcessGDBRemote::MonitorDebugserverProcess(std::weak_ptr<ProcessGDBRemote> process_wp, lldb::pid_t debugserver_pid,
36643665
bool exited, // True if the process did exit
36653666
int signo, // Zero for no signal
36663667
int exit_status // Exit value of process if signal is zero
36673668
)
36683669
{
3669-
// The baton is a "ProcessGDBRemote *". Now this class might be gone
3670-
// and might not exist anymore, so we need to carefully try to get the
3671-
// target for this process first since we have a race condition when
3672-
// we are done running between getting the notice that the inferior
3673-
// process has died and the debugserver that was debugging this process.
3674-
// In our test suite, we are also continually running process after
3675-
// process, so we must be very careful to make sure:
3676-
// 1 - process object hasn't been deleted already
3677-
// 2 - that a new process object hasn't been recreated in its place
3678-
36793670
// "debugserver_pid" argument passed in is the process ID for
36803671
// debugserver that we are tracking...
36813672
Log *log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
3673+
const bool handled = true;
36823674

3683-
// Get a shared pointer to the target that has a matching process pointer.
3684-
// This target could be gone, or the target could already have a new process
3685-
// object inside of it
3686-
TargetSP target_sp (Debugger::FindTargetWithProcess(process));
3675+
if (log)
3676+
log->Printf("ProcessGDBRemote::%s(process_wp, pid=%" PRIu64 ", signo=%i (0x%x), exit_status=%i)", __FUNCTION__,
3677+
debugserver_pid, signo, signo, exit_status);
36873678

3679+
std::shared_ptr<ProcessGDBRemote> process_sp = process_wp.lock();
36883680
if (log)
3689-
log->Printf("ProcessGDBRemote::%s(process=%p, pid=%" PRIu64 ", signo=%i (0x%x), exit_status=%i)", __FUNCTION__,
3690-
process, debugserver_pid, signo, signo, exit_status);
3691-
3692-
if (target_sp)
3693-
{
3694-
// We found a process in a target that matches, but another thread
3695-
// might be in the process of launching a new process that will
3696-
// soon replace it, so get a shared pointer to the process so we
3697-
// can keep it alive.
3698-
ProcessSP process_sp (target_sp->GetProcessSP());
3699-
// Now we have a shared pointer to the process that can't go away on us
3700-
// so we now make sure it was the same as the one passed in, and also make
3701-
// sure that our previous "process *" didn't get deleted and have a new
3702-
// "process *" created in its place with the same pointer. To verify this
3703-
// we make sure the process has our debugserver process ID. If we pass all
3704-
// of these tests, then we are sure that this process is the one we were
3705-
// looking for.
3706-
if (process_sp && process == process_sp.get() && process->m_debugserver_pid == debugserver_pid)
3681+
log->Printf("ProcessGDBRemote::%s(process = %p)", __FUNCTION__, process_sp.get());
3682+
if (!process_sp || process_sp->m_debugserver_pid != debugserver_pid)
3683+
return handled;
3684+
3685+
// Sleep for a half a second to make sure our inferior process has
3686+
// time to set its exit status before we set it incorrectly when
3687+
// both the debugserver and the inferior process shut down.
3688+
usleep(500000);
3689+
// If our process hasn't yet exited, debugserver might have died.
3690+
// If the process did exit, then we are reaping it.
3691+
const StateType state = process_sp->GetState();
3692+
3693+
if (state != eStateInvalid && state != eStateUnloaded && state != eStateExited && state != eStateDetached)
3694+
{
3695+
char error_str[1024];
3696+
if (signo)
37073697
{
3708-
// Sleep for a half a second to make sure our inferior process has
3709-
// time to set its exit status before we set it incorrectly when
3710-
// both the debugserver and the inferior process shut down.
3711-
usleep (500000);
3712-
// If our process hasn't yet exited, debugserver might have died.
3713-
// If the process did exit, the we are reaping it.
3714-
const StateType state = process->GetState();
3715-
3716-
if (process->m_debugserver_pid != LLDB_INVALID_PROCESS_ID &&
3717-
state != eStateInvalid &&
3718-
state != eStateUnloaded &&
3719-
state != eStateExited &&
3720-
state != eStateDetached)
3721-
{
3722-
char error_str[1024];
3723-
if (signo)
3724-
{
3725-
const char *signal_cstr = process->GetUnixSignals()->GetSignalAsCString(signo);
3726-
if (signal_cstr)
3727-
::snprintf (error_str, sizeof (error_str), DEBUGSERVER_BASENAME " died with signal %s", signal_cstr);
3728-
else
3729-
::snprintf (error_str, sizeof (error_str), DEBUGSERVER_BASENAME " died with signal %i", signo);
3730-
}
3731-
else
3732-
{
3733-
::snprintf (error_str, sizeof (error_str), DEBUGSERVER_BASENAME " died with an exit status of 0x%8.8x", exit_status);
3734-
}
3735-
3736-
process->SetExitStatus (-1, error_str);
3737-
}
3738-
// Debugserver has exited we need to let our ProcessGDBRemote
3739-
// know that it no longer has a debugserver instance
3740-
process->m_debugserver_pid = LLDB_INVALID_PROCESS_ID;
3698+
const char *signal_cstr = process_sp->GetUnixSignals()->GetSignalAsCString(signo);
3699+
if (signal_cstr)
3700+
::snprintf(error_str, sizeof(error_str), DEBUGSERVER_BASENAME " died with signal %s", signal_cstr);
3701+
else
3702+
::snprintf(error_str, sizeof(error_str), DEBUGSERVER_BASENAME " died with signal %i", signo);
3703+
}
3704+
else
3705+
{
3706+
::snprintf(error_str, sizeof(error_str), DEBUGSERVER_BASENAME " died with an exit status of 0x%8.8x",
3707+
exit_status);
37413708
}
3709+
3710+
process_sp->SetExitStatus(-1, error_str);
37423711
}
3743-
return true;
3712+
// Debugserver has exited we need to let our ProcessGDBRemote
3713+
// know that it no longer has a debugserver instance
3714+
process_sp->m_debugserver_pid = LLDB_INVALID_PROCESS_ID;
3715+
return handled;
37443716
}
37453717

37463718
void

‎lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -405,7 +405,8 @@ class ProcessGDBRemote : public Process
405405
AsyncThread (void *arg);
406406

407407
static bool
408-
MonitorDebugserverProcess(ProcessGDBRemote *process, lldb::pid_t pid, bool exited, int signo, int exit_status);
408+
MonitorDebugserverProcess(std::weak_ptr<ProcessGDBRemote> process_wp, lldb::pid_t pid, bool exited, int signo,
409+
int exit_status);
409410

410411
lldb::StateType
411412
SetThreadStopInfo (StringExtractor& stop_packet);

0 commit comments

Comments
 (0)
Please sign in to comment.