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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
