[Lldb-commits] [PATCH] D59114: [lldb-vscode] Don't hang indefinitely on invalid program
labath added a comment. A test case? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59114/new/ https://reviews.llvm.org/D59114 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D59114: [lldb-vscode] Don't hang indefinitely on invalid program
This revision was automatically updated to reflect the committed changes.
Closed by commit rL355656: [lldb-vscode] Report an error if an invalid program
is specified. (authored by zturner, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D59114?vs=189788&id=189799#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59114/new/
https://reviews.llvm.org/D59114
Files:
lldb/trunk/source/Target/Process.cpp
lldb/trunk/tools/lldb-vscode/lldb-vscode.cpp
Index: lldb/trunk/source/Target/Process.cpp
===
--- lldb/trunk/source/Target/Process.cpp
+++ lldb/trunk/source/Target/Process.cpp
@@ -2519,108 +2519,112 @@
m_process_input_reader.reset();
Module *exe_module = GetTarget().GetExecutableModulePointer();
- if (exe_module) {
-char local_exec_file_path[PATH_MAX];
-char platform_exec_file_path[PATH_MAX];
-exe_module->GetFileSpec().GetPath(local_exec_file_path,
- sizeof(local_exec_file_path));
-exe_module->GetPlatformFileSpec().GetPath(platform_exec_file_path,
- sizeof(platform_exec_file_path));
-if (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
- // remote platform it will install any needed binaries
- error = GetTarget().Install(&launch_info);
- if (error.Fail())
-return error;
+ if (!exe_module) {
+error.SetErrorString("executable module does not exist");
+return error;
+ }
- if (PrivateStateThreadIsValid())
-PausePrivateStateThread();
+ char local_exec_file_path[PATH_MAX];
+ char platform_exec_file_path[PATH_MAX];
+ exe_module->GetFileSpec().GetPath(local_exec_file_path,
+sizeof(local_exec_file_path));
+ exe_module->GetPlatformFileSpec().GetPath(platform_exec_file_path,
+sizeof(platform_exec_file_path));
+ if (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
+// remote platform it will install any needed binaries
+error = GetTarget().Install(&launch_info);
+if (error.Fail())
+ return error;
- error = WillLaunch(exe_module);
- if (error.Success()) {
-const bool restarted = false;
-SetPublicState(eStateLaunching, restarted);
-m_should_detach = false;
+if (PrivateStateThreadIsValid())
+ PausePrivateStateThread();
-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");
-}
+error = WillLaunch(exe_module);
+if (error.Success()) {
+ 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)
- LoadOperatingSystemPlugi
[Lldb-commits] [PATCH] D59114: [lldb-vscode] Don't hang indefinitely on invalid program
zturner created this revision.
zturner added a reviewer: clayborg.
When you configure your launch settings, if you pass a path to an invalid
program, we would previously hand indefinitely. This is because a combination
of two bugs working together. The first is that we did not attempt to detect
when handling the request whether the file existed, and we would just pass it
through to the `Process::Launch` method. The second is that the
`Process::Launch` method did not properly report an error in the case where the
file does not exist. It actually reported that the launch succeeded, which
would then cause LLDB to wait on the broadcaster to receive some events, which
would obviously never come.
Although fixing this in either place independently will get lldb-vscode working
properly when an invalid executable is specified, I'm fixing it in both places
because it seems like the right thing to do. Note that for the first fix (the
one in `lldb-vscode.cpp`) we were previously checking the value of
`error.Fail()`, but the previous call did not actually communicate an error via
an `SBError` return, instead it communicated an error via a null `SBModule`
return. So this condition is changed.
https://reviews.llvm.org/D59114
Files:
lldb/source/Target/Process.cpp
lldb/tools/lldb-vscode/lldb-vscode.cpp
Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -1227,20 +1227,23 @@
// Grab the name of the program we need to debug and set it as the first
// argument that will be passed to the program we will debug.
- const auto program = GetString(arguments, "program");
+ llvm::StringRef program = GetString(arguments, "program");
if (!program.empty()) {
lldb::SBFileSpec program_fspec(program.data(), true /*resolve_path*/);
-
g_vsc.launch_info.SetExecutableFile(program_fspec,
true /*add_as_first_arg*/);
const char *target_triple = nullptr;
const char *uuid_cstr = nullptr;
// Stand alone debug info file if different from executable
const char *symfile = nullptr;
-g_vsc.target.AddModule(program.data(), target_triple, uuid_cstr, symfile);
-if (error.Fail()) {
+lldb::SBModule module = g_vsc.target.AddModule(
+program.data(), target_triple, uuid_cstr, symfile);
+if (!module.IsValid()) {
response["success"] = llvm::json::Value(false);
- EmplaceSafeString(response, "message", std::string(error.GetCString()));
+
+ EmplaceSafeString(
+ response, "message",
+ llvm::formatv("Could not load program '{0}'.", program).str());
g_vsc.SendJSON(llvm::json::Value(std::move(response)));
}
}
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -2519,108 +2519,112 @@
m_process_input_reader.reset();
Module *exe_module = GetTarget().GetExecutableModulePointer();
- if (exe_module) {
-char local_exec_file_path[PATH_MAX];
-char platform_exec_file_path[PATH_MAX];
-exe_module->GetFileSpec().GetPath(local_exec_file_path,
- sizeof(local_exec_file_path));
-exe_module->GetPlatformFileSpec().GetPath(platform_exec_file_path,
- sizeof(platform_exec_file_path));
-if (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
- // remote platform it will install any needed binaries
- error = GetTarget().Install(&launch_info);
- if (error.Fail())
-return error;
+ if (!exe_module) {
+error.SetErrorString("executable module does not exist");
+return error;
+ }
- if (PrivateStateThreadIsValid())
-PausePrivateStateThread();
+ char local_exec_file_path[PATH_MAX];
+ char platform_exec_file_path[PATH_MAX];
+ exe_module->GetFileSpec().GetPath(local_exec_file_path,
+sizeof(local_exec_file_path));
+ exe_module->GetPlatformFileSpec().GetPath(platform_exec_file_path,
+sizeof(platform_exec_file_path));
+ if (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
+// remote platform it will install any needed binaries
+error = GetTarget().Install(&launch_info);
+if (error.Fail())
+ return error;
- error = WillLaunch(exe_module);
- if (error.Success()) {
-const bool restarted = false;
-SetPublicState(eStateLaunching, restarted);
-
