clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Great stuff! See inline comments.
A few new tests need to be added:
- test running lldb-vscode with invalid "--launch-target" executable path so
things fail to exec and verify we get as nice of an error message as possible
- test running lldb-vscode with valid "--launch-target" and no "--pid-file"
option to make sure lldb-vscode fails gracefully with an error and appropriate
message
- test running lldb-vscode with valid "--launch-target" and an invalid
"--pid-file" path with an invalid directory
- test running lldb-vscode with valid "--launch-target" and an invalid
"--pid-file" path that points somewhere we don't have permissions
All these tests will ensure we don't end up with a deadlock situation when
trying to launch in terminal.
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1454
+ // adaptor.
+#if !defined(_WIN32)
+ if (int err = mkfifo(pid_file.c_str(), 0666)) {
----------------
If windows is not supported, should we avoid calling this function
(CreateRunInTerminalPidFile) in the first place?
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1472
+ std::ifstream pid_file_reader(pid_file, std::ifstream::in);
+ pid_file_reader >> pid;
+ return pid;
----------------
What happens if the pid never gets written to the stream? Deadlock forever?
Also, what happens if an error happens during the read? Can we check if the
stream has an error after readind "pid" just in case? If there is error, (like
what if the other process wrote "hello" to the stream instead of "123"), what
ends up in "pid"? Is it set to zero? Or just left as a random number? If it is
left alone, we should initialize "pid" with the LLDB_INVALID_PROCESS_ID on line
1470
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1506
if (error.Success()) {
- // Wait for the attach stop event to happen or for a timeout.
- g_vsc.waiting_for_run_in_terminal = true;
- static std::mutex mutex;
- std::unique_lock<std::mutex> locker(mutex);
- g_vsc.request_in_terminal_cv.wait_for(locker, std::chrono::seconds(10));
+ lldb::pid_t pid = GetRunInTerminalDebuggeePid(pid_file);
----------------
Check for invalid LLDB_INVALID_PROCESS_ID here in case something went wrong
when trying to read the "pid" and return an error
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1517-1518
+ NotifyRunInTerminalDebuggeeWasAttached(pid_file);
+ // We resume the process to let stopOnEntry decide whether to stop the
+ // process or not.
+ g_vsc.target.GetProcess().Continue();
----------------
Do we need more to this comment? Aren't we continuing the lldb-vscode that will
exec into our program here? If we are, we should clearly explain what is
happening here.
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3003
int main(int argc, char *argv[]) {
+ g_vsc.debug_adaptor_path = argv[0];
+
----------------
This path can be relative, so we might need to resolve it using a llvm path
call? I can't remember if execv() can use a relative path or not.
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3011
+#if !defined(_WIN32)
+ if (auto *target_arg = input_args.getLastArg(OPT_launch_target)) {
+ std::string pid_file = input_args.getLastArg(OPT_pid_file)->getValue();
----------------
We need a nice beefy comment here as to what is going on explaining everything.
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3012
+ if (auto *target_arg = input_args.getLastArg(OPT_launch_target)) {
+ std::string pid_file = input_args.getLastArg(OPT_pid_file)->getValue();
+ std::ofstream pid_file_writer(pid_file, std::ofstream::out);
----------------
Does the llvm argument parser already guarantee that we have a valid value in
pid_file? If not, we need to make sure "pid_file" is not empty here and return
an error if it wasn't specified
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3013
+ std::string pid_file = input_args.getLastArg(OPT_pid_file)->getValue();
+ std::ofstream pid_file_writer(pid_file, std::ofstream::out);
+ pid_file_writer << getpid() << std::endl;
----------------
error check that the creation of the ofstream succeeded and return an error if
it fails and exit with EXIT_FAILURE
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3022
+ if (RunInTerminalDebugeeWasAttached(pid_file)) {
+ const char *target = target_arg->getValue();
+ execv(target, argv + target_arg->getIndex() + 2);
----------------
Should we verify that "target" is a valid file that exists prior to just
calling execv? We might be able to return a better error message. Or we can
check if the file exists after the "execv(...)" call and return a better error
message than "Couldn't launch target" like "Failed to launch
'/path/that/doesnt/exists/a.out'".
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3025
+ perror("Couldn't launch target");
+ exit(EXIT_FAILURE);
+ }
----------------
So is the code calling lldb-vscode with "--launch-target ... --pid-file ..."
watching for a non zero return value? If something goes wrong does this show up
in the terminal for the process?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93951/new/
https://reviews.llvm.org/D93951
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
