This revision was automatically updated to reflect the committed changes.
Closed by commit rGa2c267e0c9d9: [lldb] Fix race condition between lldb-vscode
and stop hooks executor (authored by ilya-nozhkin, committed by labath).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119548/new/
https://reviews.llvm.org/D119548
Files:
lldb/include/lldb/Target/Process.h
lldb/packages/Python/lldbsuite/test/lldbtest.py
lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
lldb/source/Target/Process.cpp
lldb/source/Target/Target.cpp
lldb/test/API/tools/lldb-vscode/stop-hooks/Makefile
lldb/test/API/tools/lldb-vscode/stop-hooks/TestVSCode_stop_hooks.py
lldb/test/API/tools/lldb-vscode/stop-hooks/main.c
Index: lldb/test/API/tools/lldb-vscode/stop-hooks/main.c
===================================================================
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stop-hooks/main.c
@@ -0,0 +1 @@
+int main() { return 0; }
Index: lldb/test/API/tools/lldb-vscode/stop-hooks/TestVSCode_stop_hooks.py
===================================================================
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stop-hooks/TestVSCode_stop_hooks.py
@@ -0,0 +1,35 @@
+"""
+Test stop hooks
+"""
+
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+import lldbvscode_testcase
+
+
+class TestVSCode_stop_hooks(lldbvscode_testcase.VSCodeTestCaseBase):
+
+ mydir = TestBase.compute_mydir(__file__)
+
+ @skipIfRemote
+ def test_stop_hooks_before_run(self):
+ '''
+ Test that there is no race condition between lldb-vscode and
+ stop hooks executor
+ '''
+ program = self.getBuildArtifact("a.out")
+ preRunCommands = ['target stop-hook add -o help']
+ self.build_and_launch(program, stopOnEntry=True, preRunCommands=preRunCommands)
+
+ # The first stop is on entry.
+ self.continue_to_next_stop()
+
+ breakpoint_ids = self.set_function_breakpoints(['main'])
+ # This request hangs if the race happens, because, in that case, the
+ # command interpreter is in synchronous mode while lldb-vscode expects
+ # it to be in asynchronous mode, so, the process doesn't send the stop
+ # event to "lldb.Debugger" listener (which is monitored by lldb-vscode).
+ self.continue_to_breakpoints(breakpoint_ids)
+
+ self.continue_to_exit()
Index: lldb/test/API/tools/lldb-vscode/stop-hooks/Makefile
===================================================================
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/stop-hooks/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
Index: lldb/source/Target/Target.cpp
===================================================================
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -3026,6 +3026,14 @@
if (!launch_info.GetArchitecture().IsValid())
launch_info.GetArchitecture() = GetArchitecture();
+ // Hijacking events of the process to be created to be sure that all events
+ // until the first stop are intercepted (in case if platform doesn't define
+ // its own hijacking listener or if the process is created by the target
+ // manually, without the platform).
+ if (!launch_info.GetHijackListener())
+ launch_info.SetHijackListener(
+ Listener::MakeListener("lldb.Target.Launch.hijack"));
+
// If we're not already connected to the process, and if we have a platform
// that can launch a process for debugging, go ahead and do that here.
if (state != eStateConnected && platform_sp &&
@@ -3057,8 +3065,10 @@
}
// Since we didn't have a platform launch the process, launch it here.
- if (m_process_sp)
+ if (m_process_sp) {
+ m_process_sp->HijackProcessEvents(launch_info.GetHijackListener());
error = m_process_sp->Launch(launch_info);
+ }
}
if (!m_process_sp && error.Success())
@@ -3067,35 +3077,35 @@
if (!error.Success())
return error;
- auto at_exit =
- llvm::make_scope_exit([&]() { m_process_sp->RestoreProcessEvents(); });
+ bool rebroadcast_first_stop =
+ !synchronous_execution &&
+ launch_info.GetFlags().Test(eLaunchFlagStopAtEntry);
- if (!synchronous_execution &&
- launch_info.GetFlags().Test(eLaunchFlagStopAtEntry))
- return error;
+ assert(launch_info.GetHijackListener());
+
+ EventSP first_stop_event_sp;
+ state = m_process_sp->WaitForProcessToStop(llvm::None, &first_stop_event_sp,
+ rebroadcast_first_stop,
+ launch_info.GetHijackListener());
+ m_process_sp->RestoreProcessEvents();
- ListenerSP hijack_listener_sp(launch_info.GetHijackListener());
- if (!hijack_listener_sp) {
- hijack_listener_sp = Listener::MakeListener("lldb.Target.Launch.hijack");
- launch_info.SetHijackListener(hijack_listener_sp);
- m_process_sp->HijackProcessEvents(hijack_listener_sp);
+ if (rebroadcast_first_stop) {
+ assert(first_stop_event_sp);
+ m_process_sp->BroadcastEvent(first_stop_event_sp);
+ return error;
}
- switch (m_process_sp->WaitForProcessToStop(llvm::None, nullptr, false,
- hijack_listener_sp, nullptr)) {
+ switch (state) {
case eStateStopped: {
if (launch_info.GetFlags().Test(eLaunchFlagStopAtEntry))
break;
- if (synchronous_execution) {
+ if (synchronous_execution)
// Now we have handled the stop-from-attach, and we are just
// switching to a synchronous resume. So we should switch to the
// SyncResume hijacker.
- m_process_sp->RestoreProcessEvents();
m_process_sp->ResumeSynchronous(stream);
- } else {
- m_process_sp->RestoreProcessEvents();
+ else
error = m_process_sp->PrivateResume();
- }
if (!error.Success()) {
Status error2;
error2.SetErrorStringWithFormat(
Index: lldb/source/Target/Process.cpp
===================================================================
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -2429,6 +2429,39 @@
}
Status Process::Launch(ProcessLaunchInfo &launch_info) {
+ StateType state_after_launch = eStateInvalid;
+ EventSP first_stop_event_sp;
+ Status status =
+ LaunchPrivate(launch_info, state_after_launch, first_stop_event_sp);
+ if (status.Fail())
+ return status;
+
+ if (state_after_launch != eStateStopped &&
+ state_after_launch != eStateCrashed)
+ return Status();
+
+ // Note, the stop event was consumed above, but not handled. This
+ // was done to give DidLaunch a chance to run. The target is either
+ // stopped or crashed. Directly set the state. This is done to
+ // prevent a stop message with a bunch of spurious output on thread
+ // status, as well as not pop a ProcessIOHandler.
+ SetPublicState(state_after_launch, false);
+
+ if (PrivateStateThreadIsValid())
+ ResumePrivateStateThread();
+ else
+ StartPrivateStateThread();
+
+ // Target was stopped at entry as was intended. Need to notify the
+ // listeners about it.
+ if (launch_info.GetFlags().Test(eLaunchFlagStopAtEntry))
+ HandlePrivateEvent(first_stop_event_sp);
+
+ return Status();
+}
+
+Status Process::LaunchPrivate(ProcessLaunchInfo &launch_info, StateType &state,
+ EventSP &event_sp) {
Status error;
m_abi_sp.reset();
m_dyld_up.reset();
@@ -2445,7 +2478,7 @@
// be a way to express this path, without actually having a module.
// The way to do that is to set the ExecutableFile in the LaunchInfo.
// Figure that out here:
-
+
FileSpec exe_spec_to_use;
if (!exe_module) {
if (!launch_info.GetExecutableFile()) {
@@ -2455,7 +2488,7 @@
exe_spec_to_use = launch_info.GetExecutableFile();
} else
exe_spec_to_use = exe_module->GetFileSpec();
-
+
if (exe_module && FileSystem::Instance().Exists(exe_module->GetFileSpec())) {
// Install anything that might need to be installed prior to launching.
// For host systems, this will do nothing, but if we are connected to a
@@ -2464,6 +2497,7 @@
if (error.Fail())
return error;
}
+
// Listen and queue events that are broadcasted during the process launch.
ListenerSP listener_sp(Listener::MakeListener("LaunchEventHijack"));
HijackProcessEvents(listener_sp);
@@ -2473,93 +2507,81 @@
PausePrivateStateThread();
error = WillLaunch(exe_module);
- if (error.Success()) {
- const bool restarted = false;
- SetPublicState(eStateLaunching, restarted);
- m_should_detach = false;
+ if (error.Fail()) {
+ std::string local_exec_file_path = exe_spec_to_use.GetPath();
+ return Status("file doesn't exist: '%s'", local_exec_file_path.c_str());
+ }
- if (m_public_run_lock.TrySetRunning()) {
- // Now launch using these arguments.
- error = DoLaunch(exe_module, launch_info);
- } else {
- // This shouldn't happen
- error.SetErrorString("failed to acquire process run lock");
- }
+ const bool restarted = false;
+ SetPublicState(eStateLaunching, restarted);
+ m_should_detach = false;
- if (error.Fail()) {
- if (GetID() != LLDB_INVALID_PROCESS_ID) {
- SetID(LLDB_INVALID_PROCESS_ID);
- const char *error_string = error.AsCString();
- if (error_string == nullptr)
- error_string = "launch failed";
- SetExitStatus(-1, error_string);
- }
- } else {
- EventSP event_sp;
+ if (m_public_run_lock.TrySetRunning()) {
+ // Now launch using these arguments.
+ error = DoLaunch(exe_module, launch_info);
+ } else {
+ // This shouldn't happen
+ error.SetErrorString("failed to acquire process run lock");
+ }
- // Now wait for the process to launch and return control to us, and then
- // call DidLaunch:
- StateType state = WaitForProcessStopPrivate(event_sp, seconds(10));
-
- if (state == eStateInvalid || !event_sp) {
- // We were able to launch the process, but we failed to catch the
- // initial stop.
- error.SetErrorString("failed to catch stop after launch");
- SetExitStatus(0, "failed to catch stop after launch");
- Destroy(false);
- } else if (state == eStateStopped || state == eStateCrashed) {
- DidLaunch();
-
- DynamicLoader *dyld = GetDynamicLoader();
- if (dyld)
- dyld->DidLaunch();
-
- GetJITLoaders().DidLaunch();
-
- SystemRuntime *system_runtime = GetSystemRuntime();
- if (system_runtime)
- system_runtime->DidLaunch();
-
- if (!m_os_up)
- LoadOperatingSystemPlugin(false);
-
- // We successfully launched the process and stopped, now it the
- // right time to set up signal filters before resuming.
- UpdateAutomaticSignalFiltering();
-
- // Note, the stop event was consumed above, but not handled. This
- // was done to give DidLaunch a chance to run. The target is either
- // stopped or crashed. Directly set the state. This is done to
- // prevent a stop message with a bunch of spurious output on thread
- // status, as well as not pop a ProcessIOHandler.
- // We are done with the launch hijack listener, and this stop should
- // go to the public state listener:
- RestoreProcessEvents();
- SetPublicState(state, false);
-
- if (PrivateStateThreadIsValid())
- ResumePrivateStateThread();
- else
- StartPrivateStateThread();
-
- // Target was stopped at entry as was intended. Need to notify the
- // listeners about it.
- if (state == eStateStopped &&
- launch_info.GetFlags().Test(eLaunchFlagStopAtEntry))
- HandlePrivateEvent(event_sp);
- } else if (state == eStateExited) {
- // We exited while trying to launch somehow. Don't call DidLaunch
- // as that's not likely to work, and return an invalid pid.
- HandlePrivateEvent(event_sp);
- }
+ if (error.Fail()) {
+ if (GetID() != LLDB_INVALID_PROCESS_ID) {
+ SetID(LLDB_INVALID_PROCESS_ID);
+ const char *error_string = error.AsCString();
+ if (error_string == nullptr)
+ error_string = "launch failed";
+ SetExitStatus(-1, error_string);
}
- } else {
- std::string local_exec_file_path = exe_spec_to_use.GetPath();
- error.SetErrorStringWithFormat("file doesn't exist: '%s'",
- local_exec_file_path.c_str());
+ return error;
}
- return error;
+ // Now wait for the process to launch and return control to us, and then
+ // call DidLaunch:
+ state = WaitForProcessStopPrivate(event_sp, seconds(10));
+
+ if (state == eStateInvalid || !event_sp) {
+ // We were able to launch the process, but we failed to catch the
+ // initial stop.
+ error.SetErrorString("failed to catch stop after launch");
+ SetExitStatus(0, error.AsCString());
+ Destroy(false);
+ return error;
+ }
+
+ if (state == eStateExited) {
+ // We exited while trying to launch somehow. Don't call DidLaunch
+ // as that's not likely to work, and return an invalid pid.
+ HandlePrivateEvent(event_sp);
+ return Status();
+ }
+
+ if (state == eStateStopped || state == eStateCrashed) {
+ DidLaunch();
+
+ DynamicLoader *dyld = GetDynamicLoader();
+ if (dyld)
+ dyld->DidLaunch();
+
+ GetJITLoaders().DidLaunch();
+
+ SystemRuntime *system_runtime = GetSystemRuntime();
+ if (system_runtime)
+ system_runtime->DidLaunch();
+
+ if (!m_os_up)
+ LoadOperatingSystemPlugin(false);
+
+ // We successfully launched the process and stopped, now it the
+ // right time to set up signal filters before resuming.
+ UpdateAutomaticSignalFiltering();
+ return Status();
+ }
+
+ return Status("Unexpected process state after the launch: %s, expected %s, "
+ "%s, %s or %s",
+ StateAsCString(state), StateAsCString(eStateInvalid),
+ StateAsCString(eStateExited), StateAsCString(eStateStopped),
+ StateAsCString(eStateCrashed));
}
Status Process::LoadCore() {
Index: lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
===================================================================
--- lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
+++ lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
@@ -428,6 +428,8 @@
"gdb-remote", nullptr, true);
if (process_sp) {
+ process_sp->HijackProcessEvents(launch_info.GetHijackListener());
+
error = process_sp->ConnectRemote(connect_url.c_str());
// Retry the connect remote one time...
if (error.Fail())
Index: lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
===================================================================
--- lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -488,18 +488,20 @@
// This is a process attach. Don't need to launch anything.
ProcessAttachInfo attach_info(launch_info);
return Attach(attach_info, debugger, &target, error);
- } else {
- ProcessSP process_sp = target.CreateProcess(
- launch_info.GetListener(), launch_info.GetProcessPluginName(), nullptr,
- false);
+ }
- // We need to launch and attach to the process.
- launch_info.GetFlags().Set(eLaunchFlagDebug);
- if (process_sp)
- error = process_sp->Launch(launch_info);
+ ProcessSP process_sp =
+ target.CreateProcess(launch_info.GetListener(),
+ launch_info.GetProcessPluginName(), nullptr, false);
- return process_sp;
- }
+ process_sp->HijackProcessEvents(launch_info.GetHijackListener());
+
+ // We need to launch and attach to the process.
+ launch_info.GetFlags().Set(eLaunchFlagDebug);
+ if (process_sp)
+ error = process_sp->Launch(launch_info);
+
+ return process_sp;
}
lldb::ProcessSP PlatformWindows::Attach(ProcessAttachInfo &attach_info,
Index: lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
===================================================================
--- lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
+++ lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
@@ -217,11 +217,12 @@
launch_info.GetListener(),
process_gdb_remote::ProcessGDBRemote::GetPluginNameStatic(), nullptr,
true);
+ if (!process_sp) {
+ error.SetErrorString("Failed to create GDB process");
+ return nullptr;
+ }
- ListenerSP listener_sp =
- Listener::MakeListener("lldb.platform_qemu_user.debugprocess");
- launch_info.SetHijackListener(listener_sp);
- Process::ProcessEventHijacker hijacker(*process_sp, listener_sp);
+ process_sp->HijackProcessEvents(launch_info.GetHijackListener());
error = process_sp->ConnectRemote(("unix-connect://" + socket_path).str());
if (error.Fail())
@@ -232,7 +233,6 @@
process_sp->SetSTDIOFileDescriptor(
launch_info.GetPTY().ReleasePrimaryFileDescriptor());
- process_sp->WaitForProcessToStop(llvm::None, nullptr, false, listener_sp);
return process_sp;
}
Index: lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
===================================================================
--- lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
+++ lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
@@ -443,9 +443,8 @@
// Now create the gdb-remote process.
LLDB_LOG(log, "having target create process with gdb-remote plugin");
- process_sp =
- target.CreateProcess(launch_info.GetListener(), "gdb-remote", nullptr,
- true);
+ process_sp = target.CreateProcess(launch_info.GetListener(), "gdb-remote",
+ nullptr, true);
if (!process_sp) {
error.SetErrorString("CreateProcess() failed for gdb-remote process");
@@ -454,15 +453,8 @@
}
LLDB_LOG(log, "successfully created process");
- // Adjust launch for a hijacker.
- ListenerSP listener_sp;
- if (!launch_info.GetHijackListener()) {
- LLDB_LOG(log, "setting up hijacker");
- listener_sp =
- Listener::MakeListener("lldb.PlatformLinux.DebugProcess.hijack");
- launch_info.SetHijackListener(listener_sp);
- process_sp->HijackProcessEvents(listener_sp);
- }
+
+ process_sp->HijackProcessEvents(launch_info.GetHijackListener());
// Log file actions.
if (log) {
@@ -480,14 +472,6 @@
// Do the launch.
error = process_sp->Launch(launch_info);
if (error.Success()) {
- // Handle the hijacking of process events.
- if (listener_sp) {
- const StateType state = process_sp->WaitForProcessToStop(
- llvm::None, nullptr, false, listener_sp);
-
- LLDB_LOG(log, "pid {0} state {0}", process_sp->GetID(), state);
- }
-
// Hook up process PTY if we have one (which we should for local debugging
// with llgs).
int pty_fd = launch_info.GetPTY().ReleasePrimaryFileDescriptor();
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
===================================================================
--- lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
@@ -11,8 +11,8 @@
def create_debug_adaptor(self, lldbVSCodeEnv=None):
'''Create the Visual Studio Code debug adaptor'''
- self.assertTrue(os.path.exists(self.lldbVSCodeExec),
- 'lldb-vscode must exist')
+ self.assertTrue(is_exe(self.lldbVSCodeExec),
+ 'lldb-vscode must exist and be executable')
log_file_path = self.getBuildArtifact('vscode.txt')
self.vscode = vscode.DebugAdaptor(
executable=self.lldbVSCodeExec, init_commands=self.setUpCommands(),
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===================================================================
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -231,6 +231,11 @@
def is_exe(fpath):
"""Returns true if fpath is an executable."""
+ if fpath == None:
+ return False
+ if sys.platform == 'win32':
+ if not fpath.endswith(".exe"):
+ fpath += ".exe"
return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
Index: lldb/include/lldb/Target/Process.h
===================================================================
--- lldb/include/lldb/Target/Process.h
+++ lldb/include/lldb/Target/Process.h
@@ -3073,6 +3073,9 @@
void ControlPrivateStateThread(uint32_t signal);
+ Status LaunchPrivate(ProcessLaunchInfo &launch_info, lldb::StateType &state,
+ lldb::EventSP &event_sp);
+
Process(const Process &) = delete;
const Process &operator=(const Process &) = delete;
};
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
