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

Greg Clayton via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed Jan 6 17:34:34 PST 2021


clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

So I like that we are using files in the file system for things, but we are using 3 of them and that seems excessive. See inlined comments and see if we can get down to using just one? Also, what is stopping us from being able to support windows here? Just mkfifo?



================
Comment at: lldb/tools/lldb-vscode/RunInTerminal.cpp:29
+static llvm::Error CreateFifoFile(const std::string &path) {
+  if (int err = mkfifo(path.c_str(), 0666))
+    return llvm::createStringError(
----------------
These permissions seem bad, probably should be 0600 to only allow current user to access for security purposes.

Also, any reason we are using mkfifo over just a normal temp file? Is this the only thing that keeps us from using this on windows? Could we have windows support if just create a normal file like std::ofstream and std::ifstream?


================
Comment at: lldb/tools/lldb-vscode/RunInTerminal.h:32-38
+  /// File the runInTerminal launcher can write its pid to
+  std::string pid_file;
+  /// File the runInTerminal launcher can write error messages to
+  std::string err_file;
+  /// File whose existance indicates that the debug adaptor has attached to the
+  /// runInTerminal launcher.
+  std::string did_attach_file;
----------------
Can we avoid using more than one file here? Might be nice to require just one file that gets a JSON dictionary as the contents?
```
{ "pid": 123 }
```
or
```
{ "error" : "<error message>" }
```
Then we need to read data until we get correctly formed JSON or timeout.


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1377-1381
+#if !defined(_WIN32)
   body.try_emplace("supportsRunInTerminalRequest", true);
+#else
+  body.try_emplace("supportsRunInTerminalRequest", false);
+#endif
----------------
Reverse this to avoid the negate?


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1442-1446
+#if defined(_WIN32)
+  return llvm::createStringError(
+      llvm::inconvertibleErrorCode(),
+      "\"runInTerminal\" is not supported on Windows");
+#else
----------------
We shouldn't need any #if define(_WIN32) since we didn't enable this feature in the response to "initialize" right?


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2982
+// emitted to the debug adaptor.
+[[noreturn]] void LaunchRunInTerminalTarget(llvm::opt::Arg &target_arg,
+                                            llvm::StringRef comm_files_prefix,
----------------
Is "[[noreturn]]" supported by all compilers that compile LLDB? There might be some LLVM define that we should use?


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2994
+  // signal to prevent being paused forever.
+  const char *timeout_env_var = getenv("LLDB_VSCODE_RIT_TIMEOUT_IN_MS");
+  int timeout_in_ms =
----------------
Why are we using env vars for some things and options values for others? We should probably pass this in as an argument. We are also polluting the env vars of the new process we will exec if we leave this in the environment


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3001
+      const char *target = target_arg.getValue();
+      execv(target, argv + target_arg.getIndex() + 2);
+
----------------
I assume env vars are being inherited from this process?


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3018
+  llvm::SmallString<256> program_path(argv[0]);
+  llvm::sys::fs::make_absolute(program_path);
+  g_vsc.debug_adaptor_path = program_path.str().str();
----------------
Might be worth checking if there is a llvm file system call to determine the program path already?


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3026
+
+  if (auto *target_arg = input_args.getLastArg(OPT_launch_target)) {
+    if (auto *comm_file_prefix = input_args.getLastArg(OPT_comm_files_prefix))
----------------
Don't use auto


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:3027
+  if (auto *target_arg = input_args.getLastArg(OPT_launch_target)) {
+    if (auto *comm_file_prefix = input_args.getLastArg(OPT_comm_files_prefix))
+      LaunchRunInTerminalTarget(*target_arg, comm_file_prefix->getValue(),
----------------
don't use auto 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93951/new/

https://reviews.llvm.org/D93951



More information about the lldb-commits mailing list