[lldb-dap] Reimplement runInTerminal with signals
#142374
Open
+147
−763
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Hu Jialun (SuibianP) Changes
Instead, take advantage of
With this, the entirety of Refs: #121269 (comment) Patch is 36.38 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/142374.diff 11 Files Affected:
diff --git a/lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py b/lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py
index 65c931210d400..3b6571621e541 100644
--- a/lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py
+++ b/lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py
@@ -131,120 +131,3 @@ def test_runInTerminalInvalidTarget(self):
"'INVALIDPROGRAM' does not exist",
response["body"]["error"]["format"],
)
-
- @skipIfWindows
- @skipIf(oslist=["linux"], archs=no_match(["x86_64"]))
- def test_missingArgInRunInTerminalLauncher(self):
- if not self.isTestSupported():
- return
- proc = subprocess.run(
- [self.lldbDAPExec, "--launch-target", "INVALIDPROGRAM"],
- capture_output=True,
- universal_newlines=True,
- )
- self.assertNotEqual(proc.returncode, 0)
- self.assertIn(
- '"--launch-target" requires "--comm-file" to be specified', proc.stderr
- )
-
- @skipIfWindows
- @skipIf(oslist=["linux"], archs=no_match(["x86_64"]))
- def test_FakeAttachedRunInTerminalLauncherWithInvalidProgram(self):
- if not self.isTestSupported():
- return
- comm_file = os.path.join(self.getBuildDir(), "comm-file")
- os.mkfifo(comm_file)
-
- proc = subprocess.Popen(
- [
- self.lldbDAPExec,
- "--comm-file",
- comm_file,
- "--launch-target",
- "INVALIDPROGRAM",
- ],
- universal_newlines=True,
- stderr=subprocess.PIPE,
- )
-
- self.readPidMessage(comm_file)
- self.sendDidAttachMessage(comm_file)
- self.assertIn("No such file or directory", self.readErrorMessage(comm_file))
-
- _, stderr = proc.communicate()
- self.assertIn("No such file or directory", stderr)
-
- @skipIfWindows
- @skipIf(oslist=["linux"], archs=no_match(["x86_64"]))
- def test_FakeAttachedRunInTerminalLauncherWithValidProgram(self):
- if not self.isTestSupported():
- return
- comm_file = os.path.join(self.getBuildDir(), "comm-file")
- os.mkfifo(comm_file)
-
- proc = subprocess.Popen(
- [
- self.lldbDAPExec,
- "--comm-file",
- comm_file,
- "--launch-target",
- "echo",
- "foo",
- ],
- universal_newlines=True,
- stdout=subprocess.PIPE,
- )
-
- self.readPidMessage(comm_file)
- self.sendDidAttachMessage(comm_file)
-
- stdout, _ = proc.communicate()
- self.assertIn("foo", stdout)
-
- @skipIfWindows
- @skipIf(oslist=["linux"], archs=no_match(["x86_64"]))
- def test_FakeAttachedRunInTerminalLauncherAndCheckEnvironment(self):
- if not self.isTestSupported():
- return
- comm_file = os.path.join(self.getBuildDir(), "comm-file")
- os.mkfifo(comm_file)
-
- proc = subprocess.Popen(
- [self.lldbDAPExec, "--comm-file", comm_file, "--launch-target", "env"],
- universal_newlines=True,
- stdout=subprocess.PIPE,
- env={**os.environ, "FOO": "BAR"},
- )
-
- self.readPidMessage(comm_file)
- self.sendDidAttachMessage(comm_file)
-
- stdout, _ = proc.communicate()
- self.assertIn("FOO=BAR", stdout)
-
- @skipIfWindows
- @skipIf(oslist=["linux"], archs=no_match(["x86_64"]))
- def test_NonAttachedRunInTerminalLauncher(self):
- if not self.isTestSupported():
- return
- comm_file = os.path.join(self.getBuildDir(), "comm-file")
- os.mkfifo(comm_file)
-
- proc = subprocess.Popen(
- [
- self.lldbDAPExec,
- "--comm-file",
- comm_file,
- "--launch-target",
- "echo",
- "foo",
- ],
- universal_newlines=True,
- stderr=subprocess.PIPE,
- env={**os.environ, "LLDB_DAP_RIT_TIMEOUT_IN_MS": "1000"},
- )
-
- self.readPidMessage(comm_file)
-
- _, stderr = proc.communicate()
- self.assertIn("Timed out trying to get messages from the debug adapter", stderr)
diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt
index 40cba16c141f0..3e7e88afa9ced 100644
--- a/lldb/tools/lldb-dap/CMakeLists.txt
+++ b/lldb/tools/lldb-dap/CMakeLists.txt
@@ -14,7 +14,6 @@ add_lldb_library(lldbDAP
DAPLog.cpp
EventHelper.cpp
ExceptionBreakpoint.cpp
- FifoFiles.cpp
FunctionBreakpoint.cpp
InstructionBreakpoint.cpp
JSONUtils.cpp
@@ -22,7 +21,6 @@ add_lldb_library(lldbDAP
OutputRedirector.cpp
ProgressEvent.cpp
ProtocolUtils.cpp
- RunInTerminal.cpp
SourceBreakpoint.cpp
Transport.cpp
Variables.cpp
diff --git a/lldb/tools/lldb-dap/FifoFiles.cpp b/lldb/tools/lldb-dap/FifoFiles.cpp
deleted file mode 100644
index 1f1bba80bd3b1..0000000000000
--- a/lldb/tools/lldb-dap/FifoFiles.cpp
+++ /dev/null
@@ -1,101 +0,0 @@
-//===-- FifoFiles.cpp -------------------------------------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "FifoFiles.h"
-#include "JSONUtils.h"
-
-#if !defined(_WIN32)
-#include <sys/stat.h>
-#include <sys/types.h>
-#include <unistd.h>
-#endif
-
-#include <chrono>
-#include <fstream>
-#include <future>
-#include <optional>
-
-using namespace llvm;
-
-namespace lldb_dap {
-
-FifoFile::FifoFile(StringRef path) : m_path(path) {}
-
-FifoFile::~FifoFile() {
-#if !defined(_WIN32)
- unlink(m_path.c_str());
-#endif
-}
-
-Expected<std::shared_ptr<FifoFile>> CreateFifoFile(StringRef path) {
-#if defined(_WIN32)
- return createStringError(inconvertibleErrorCode(), "Unimplemented");
-#else
- if (int err = mkfifo(path.data(), 0600))
- return createStringError(std::error_code(err, std::generic_category()),
- "Couldn't create fifo file: %s", path.data());
- return std::make_shared<FifoFile>(path);
-#endif
-}
-
-FifoFileIO::FifoFileIO(StringRef fifo_file, StringRef other_endpoint_name)
- : m_fifo_file(fifo_file), m_other_endpoint_name(other_endpoint_name) {}
-
-Expected<json::Value> FifoFileIO::ReadJSON(std::chrono::milliseconds timeout) {
- // We use a pointer for this future, because otherwise its normal destructor
- // would wait for the getline to end, rendering the timeout useless.
- std::optional<std::string> line;
- std::future<void> *future =
- new std::future<void>(std::async(std::launch::async, [&]() {
- std::ifstream reader(m_fifo_file, std::ifstream::in);
- std::string buffer;
- std::getline(reader, buffer);
- if (!buffer.empty())
- line = buffer;
- }));
- if (future->wait_for(timeout) == std::future_status::timeout || !line)
- // Indeed this is a leak, but it's intentional. "future" obj destructor
- // will block on waiting for the worker thread to join. And the worker
- // thread might be stuck in blocking I/O. Intentionally leaking the obj
- // as a hack to avoid blocking main thread, and adding annotation to
- // supress static code inspection warnings
-
- // coverity[leaked_storage]
- return createStringError(inconvertibleErrorCode(),
- "Timed out trying to get messages from the " +
- m_other_endpoint_name);
- delete future;
- return json::parse(*line);
-}
-
-Error FifoFileIO::SendJSON(const json::Value &json,
- std::chrono::milliseconds timeout) {
- bool done = false;
- std::future<void> *future =
- new std::future<void>(std::async(std::launch::async, [&]() {
- std::ofstream writer(m_fifo_file, std::ofstream::out);
- writer << JSONToString(json) << std::endl;
- done = true;
- }));
- if (future->wait_for(timeout) == std::future_status::timeout || !done) {
- // Indeed this is a leak, but it's intentional. "future" obj destructor will
- // block on waiting for the worker thread to join. And the worker thread
- // might be stuck in blocking I/O. Intentionally leaking the obj as a hack
- // to avoid blocking main thread, and adding annotation to supress static
- // code inspection warnings"
-
- // coverity[leaked_storage]
- return createStringError(inconvertibleErrorCode(),
- "Timed out trying to send messages to the " +
- m_other_endpoint_name);
- }
- delete future;
- return Error::success();
-}
-
-} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/FifoFiles.h b/lldb/tools/lldb-dap/FifoFiles.h
deleted file mode 100644
index 633ebeb2aedd4..0000000000000
--- a/lldb/tools/lldb-dap/FifoFiles.h
+++ /dev/null
@@ -1,85 +0,0 @@
-//===-- FifoFiles.h ---------------------------------------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLDB_TOOLS_LLDB_DAP_FIFOFILES_H
-#define LLDB_TOOLS_LLDB_DAP_FIFOFILES_H
-
-#include "llvm/Support/Error.h"
-#include "llvm/Support/JSON.h"
-
-#include <chrono>
-
-namespace lldb_dap {
-
-/// Struct that controls the life of a fifo file in the filesystem.
-///
-/// The file is destroyed when the destructor is invoked.
-struct FifoFile {
- FifoFile(llvm::StringRef path);
-
- ~FifoFile();
-
- std::string m_path;
-};
-
-/// Create a fifo file in the filesystem.
-///
-/// \param[in] path
-/// The path for the fifo file.
-///
-/// \return
-/// A \a std::shared_ptr<FifoFile> if the file could be created, or an
-/// \a llvm::Error in case of failures.
-llvm::Expected<std::shared_ptr<FifoFile>> CreateFifoFile(llvm::StringRef path);
-
-class FifoFileIO {
-public:
- /// \param[in] fifo_file
- /// The path to an input fifo file that exists in the file system.
- ///
- /// \param[in] other_endpoint_name
- /// A human readable name for the other endpoint that will communicate
- /// using this file. This is used for error messages.
- FifoFileIO(llvm::StringRef fifo_file, llvm::StringRef other_endpoint_name);
-
- /// Read the next JSON object from the underlying input fifo file.
- ///
- /// The JSON object is expected to be a single line delimited with \a
- /// std::endl.
- ///
- /// \return
- /// An \a llvm::Error object indicating the success or failure of this
- /// operation. Failures arise if the timeout is hit, the next line of text
- /// from the fifo file is not a valid JSON object, or is it impossible to
- /// read from the file.
- llvm::Expected<llvm::json::Value> ReadJSON(std::chrono::milliseconds timeout);
-
- /// Serialize a JSON object and write it to the underlying output fifo file.
- ///
- /// \param[in] json
- /// The JSON object to send. It will be printed as a single line delimited
- /// with \a std::endl.
- ///
- /// \param[in] timeout
- /// A timeout for how long we should until for the data to be consumed.
- ///
- /// \return
- /// An \a llvm::Error object indicating whether the data was consumed by
- /// a reader or not.
- llvm::Error SendJSON(
- const llvm::json::Value &json,
- std::chrono::milliseconds timeout = std::chrono::milliseconds(20000));
-
-private:
- std::string m_fifo_file;
- std::string m_other_endpoint_name;
-};
-
-} // namespace lldb_dap
-
-#endif // LLDB_TOOLS_LLDB_DAP_FIFOFILES_H
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
index 93bc80a38e29d..cfcbabbb16a93 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
@@ -14,10 +14,12 @@
#include "LLDBUtils.h"
#include "Protocol/ProtocolBase.h"
#include "Protocol/ProtocolRequests.h"
-#include "RunInTerminal.h"
#include "lldb/API/SBDefines.h"
#include "lldb/API/SBEnvironment.h"
#include "llvm/Support/Error.h"
+#include <cerrno>
+#include <csignal>
+#include <limits>
#include <mutex>
#if !defined(_WIN32)
@@ -51,6 +53,70 @@ static uint32_t SetLaunchFlag(uint32_t flags, bool flag,
return flags;
}
+// Assume single thread
+class PidReceiver {
+private:
+ inline static volatile std::sig_atomic_t pid;
+ sigset_t oldmask;
+ struct sigaction oldaction;
+
+ static_assert(std::numeric_limits<volatile sig_atomic_t>::max() >=
+ std::numeric_limits<pid_t>::max(),
+ "sig_atomic_t must be able to hold a pid_t value");
+
+ static void sig_handler(int, siginfo_t *info, void *) {
+ // Store the PID from the signal info
+ pid = info->si_pid;
+ }
+
+ PidReceiver(const PidReceiver &) = delete;
+ PidReceiver &operator=(const PidReceiver &) = delete;
+ PidReceiver(PidReceiver &&) = delete;
+
+public:
+ PidReceiver() {
+ pid = LLDB_INVALID_PROCESS_ID;
+ // sigprocmask and sigaction can only fail by programmer error
+ // Defer SIGUSR1, otherwise we might receive it out of pselect and hang
+ sigset_t mask;
+ sigemptyset(&mask);
+ sigaddset(&mask, SIGUSR1);
+ assert(!sigprocmask(SIG_BLOCK, &mask, &oldmask));
+
+ // Set up sigaction for SIGUSR1 with SA_SIGINFO
+ struct sigaction sa;
+ std::memset(&sa, 0, sizeof(sa));
+ sa.sa_sigaction = sig_handler;
+ sa.sa_flags = SA_SIGINFO;
+ sigemptyset(&sa.sa_mask);
+ assert(!sigaction(SIGUSR1, &sa, &oldaction));
+ }
+
+ ~PidReceiver() {
+ // Restore the old signal mask
+ assert(!sigprocmask(SIG_SETMASK, &oldmask, nullptr));
+ assert(!sigaction(SIGUSR1, &oldaction, nullptr));
+ }
+
+ llvm::Expected<lldb::pid_t> WaitForPid() {
+ // Wait for the signal to be received
+ while (pid == LLDB_INVALID_PROCESS_ID) {
+ struct timespec timeout;
+ timeout.tv_sec = 5;
+ timeout.tv_nsec = 0;
+ auto ret = pselect(0, nullptr, nullptr, nullptr, &timeout, &oldmask);
+ if (ret == -1 && errno != EINTR) {
+ return llvm::createStringError(
+ std::error_code(errno, std::generic_category()), "pselect failed");
+ } else if (ret == 0) {
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Timed out waiting for signal");
+ }
+ }
+ return pid;
+ }
+};
+
static llvm::Error
RunInTerminal(DAP &dap, const protocol::LaunchRequestArguments &arguments) {
if (!dap.clientFeatures.contains(
@@ -65,26 +131,19 @@ RunInTerminal(DAP &dap, const protocol::LaunchRequestArguments &arguments) {
dap.is_attach = true;
lldb::SBAttachInfo attach_info;
- llvm::Expected<std::shared_ptr<FifoFile>> comm_file_or_err =
- CreateRunInTerminalCommFile();
- if (!comm_file_or_err)
- return comm_file_or_err.takeError();
- FifoFile &comm_file = *comm_file_or_err.get();
-
- RunInTerminalDebugAdapterCommChannel comm_channel(comm_file.m_path);
-
lldb::pid_t debugger_pid = LLDB_INVALID_PROCESS_ID;
#if !defined(_WIN32)
debugger_pid = getpid();
#endif
+ auto pid_receiver = PidReceiver();
llvm::json::Object reverse_request = CreateRunInTerminalReverseRequest(
arguments.configuration.program, arguments.args, arguments.env,
- arguments.cwd, comm_file.m_path, debugger_pid);
+ arguments.cwd, debugger_pid);
dap.SendReverseRequest<LogFailureResponseHandler>("runInTerminal",
std::move(reverse_request));
- if (llvm::Expected<lldb::pid_t> pid = comm_channel.GetLauncherPid())
+ if (llvm::Expected<lldb::pid_t> pid = pid_receiver.WaitForPid())
attach_info.SetProcessID(*pid);
else
return pid.takeError();
@@ -95,13 +154,7 @@ RunInTerminal(DAP &dap, const protocol::LaunchRequestArguments &arguments) {
if (error.Fail())
return llvm::createStringError(llvm::inconvertibleErrorCode(),
- "Failed to attach to the target process. %s",
- comm_channel.GetLauncherError().c_str());
- // This will notify the runInTerminal launcher that we attached.
- // We have to make this async, as the function won't return until the launcher
- // resumes and reads the data.
- std::future<lldb::SBError> did_attach_message_success =
- comm_channel.NotifyDidAttach();
+ "Failed to attach to the target process.");
// We just attached to the runInTerminal launcher, which was waiting to be
// attached. We now resume it, so it can receive the didAttach notification
@@ -115,15 +168,7 @@ RunInTerminal(DAP &dap, const protocol::LaunchRequestArguments &arguments) {
// we return the debugger to its sync state.
scope_sync_mode.reset();
- // If sending the notification failed, the launcher should be dead by now and
- // the async didAttach notification should have an error message, so we
- // return it. Otherwise, everything was a success.
- did_attach_message_success.wait();
- error = did_attach_message_success.get();
- if (error.Success())
- return llvm::Error::success();
- return llvm::createStringError(llvm::inconvertibleErrorCode(),
- error.GetCString());
+ return llvm::Error::success();
}
void BaseRequestHandler::Run(const Request &request) {
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 573f3eba00f62..bf10742e18bb8 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -1238,15 +1238,14 @@ llvm::json::Value CreateCompileUnit(lldb::SBCompileUnit &unit) {
llvm::json::Object CreateRunInTerminalReverseRequest(
llvm::StringRef program, const std::vector<std::string> &args,
const llvm::StringMap<std::string> &env, llvm::StringRef cwd,
- llvm::StringRef comm_file, lldb::pid_t debugger_pid) {
+ lldb::pid_t debugger_pid) {
llvm::json::Object run_in_terminal_args;
// This indicates the IDE to open an embedded terminal, instead of opening
// the terminal in a new window.
run_in_terminal_args.try_emplace("kind", "integrated");
// The program path must be the first entry in the "args" field
- std::vector<std::string> req_args = {DAP::debug_adapter_path.str(),
- "--comm-file", comm_file.str()};
+ std::vector<std::string> req_args = {DAP::debug_adapter_path.str()};
if (debugger_pid != LLDB_INVALID_PROCESS_ID) {
req_args.push_back("--debugger-pid");
req_args.push_back(std::to_string(debugger_pid));
diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h
index 08699a94bbd87..4ed66e6992be2 100644
--- a/lldb/tools/lldb-dap/JSONUtils.h
+++ b/lldb/tools/lldb-dap/JSONUtils.h
@@ -466,9 +466,6 @@ llvm::json::Value CreateCompileUnit(lldb::SBCompileUnit &unit);
/// \param[in] cwd
/// The working directory for the run in terminal request.
///
-/// \param[in] comm_file
-/// The fifo file used to communicate the with the target launcher.
-///
/// \param[in] debugger_pid
/// The PID of the lldb-dap instance that will attach to the target. The
/// launcher uses it on Linux tell the kernel that it should allow the
@@ -480,7 +477,7 @@ llvm::json::Value CreateCompileUnit(lldb::SBCompileUnit &unit);
llvm::json::Object CreateRunInTerminalReverseRequest(
llvm::StringRef program, const std::vector<std::string> &args,
const llvm::StringMap<std::string> &env, llvm::StringRef cwd,
- llvm::StringRef comm_file, lldb::pid_t debugger_pid);
+ lldb::pid_t debugger_pid);
/// Create a "Terminated" JSON object that contains statistics
///
diff --git a/lldb/tools/lldb-dap/Options.td b/lldb/tools/lldb-dap/Options.td
index aecf91797ac70..669d6a7c0df05 100644
--- a/lldb/tools/lldb-dap/Options.td
+++ b/lldb/tools/lldb-dap/Options.td
@@ -31,16 +31,11 @@ def connection
"Connections are specified like 'tcp://[host]:port' or "
"'unix:///path'.">;
-def launch_target: S<"laun...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
SuibianP
force-pushed
the
lldb-dap-runinterminal-signal
branch
3 times, most recently
from
b71032a to
7555bcf
Compare
runInTerminal is currently implemented using JSON messages over FIFOs, which feels too clunky for the simple job of passing a PID. Instead, take advantage of si_pid available from sa_sigaction handler, and send a user signal instead. Both synchronisation and timeout are preserved with the protocol below, 1. Debugger waits for SIGUSR1 under timeout with pselect 2. Launcher forks into child, sends SIGUSR1 to debugger and suspends 3. Debugger receives SIGUSR1, captures the sender (launcher child) PID, attaches to and resumes it 4. Launcher child resumes and exec's into target 5. Launcher parent waitpid's and kills irresponsive child after timeout With this, the entirety of FifoFiles and RunInTerminal can be dropped. Refs: llvm#121269 (comment)
SuibianP
force-pushed
the
lldb-dap-runinterminal-signal
branch
from
7555bcf to
098fb7f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
runInTerminalis currently implemented using JSON messages over FIFOs, which feels too clunky for the simple job of passing a PID.Instead, take advantage of
si_pidavailable fromsa_sigactionhandler, and send a user signal instead. Both synchronisation and timeout are preserved with the protocol below,SIGUSR1under timeout with pselectSIGUSR1to debugger and suspendsSIGUSR1, captures the sender (launcher child) PID, attaches to and resumes itexec's into targetwaitpid's and kills irresponsive child after timeoutWith this, the entirety of
FifoFilesandRunInTerminalcan be dropped.Refs: #121269 (comment)