[Lldb-commits] [PATCH] D93951: [vscode] Improve runInTerminal and support linux

[Greg Clayton via Phabricator via lldb-commits]

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