llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Charles Zablit (charles-zablit) <details> <summary>Changes</summary> This patch adds support for the `integratedTerminal` flag on Windows. Using named pipes on Windows allows to match the `integratedTerminal` feature that is available on POSIX platforms which is implemented through the `mkfifo` command. This patch fixes the following issues: - fixes https://github.com/llvm/llvm-project/issues/174324 - fixes https://github.com/llvm/llvm-project/issues/62336 --- Patch is 38.42 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/174635.diff 11 Files Affected: - (modified) lldb/include/lldb/Host/windows/ProcessLauncherWindows.h (+72-9) - (modified) lldb/packages/Python/lldbsuite/test/dotest.py (+4-1) - (modified) lldb/source/Host/windows/ProcessLauncherWindows.cpp (+67-36) - (modified) lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py (+123-45) - (modified) lldb/tools/lldb-dap/FifoFiles.cpp (+71-9) - (modified) lldb/tools/lldb-dap/FifoFiles.h (+11-3) - (modified) lldb/tools/lldb-dap/Handler/RequestHandler.cpp (+2-2) - (modified) lldb/tools/lldb-dap/RunInTerminal.cpp (+20-3) - (modified) lldb/tools/lldb-dap/RunInTerminal.h (+3) - (modified) lldb/tools/lldb-dap/tool/lldb-dap.cpp (+112-12) - (modified) lldb/unittests/DAP/FifoFilesTest.cpp (+4-4) ``````````diff diff --git a/lldb/include/lldb/Host/windows/ProcessLauncherWindows.h b/lldb/include/lldb/Host/windows/ProcessLauncherWindows.h index 553263e2f5a72..6ebf764249a4b 100644 --- a/lldb/include/lldb/Host/windows/ProcessLauncherWindows.h +++ b/lldb/include/lldb/Host/windows/ProcessLauncherWindows.h @@ -9,10 +9,15 @@ #ifndef lldb_Host_windows_ProcessLauncherWindows_h_ #define lldb_Host_windows_ProcessLauncherWindows_h_ +#include "lldb/Host/ProcessLaunchInfo.h" #include "lldb/Host/ProcessLauncher.h" #include "lldb/Host/windows/windows.h" +#include "llvm/ADT/ScopeExit.h" +#include "llvm/ADT/StringRef.h" #include "llvm/Support/ErrorOr.h" +#include <optional> + namespace lldb_private { class ProcessLaunchInfo; @@ -22,9 +27,6 @@ class ProcessLauncherWindows : public ProcessLauncher { HostProcess LaunchProcess(const ProcessLaunchInfo &launch_info, Status &error) override; -protected: - HANDLE GetStdioHandle(const ProcessLaunchInfo &launch_info, int fd); - /// Get the list of Windows handles that should be inherited by the child /// process and update `STARTUPINFOEXW` with the handle list. /// @@ -35,12 +37,12 @@ class ProcessLauncherWindows : public ProcessLauncher { /// collected handles using `UpdateProcThreadAttribute`. On success, the /// vector of inherited handles is returned. /// - /// \param launch_info - /// The process launch configuration. - /// /// \param startupinfoex /// The extended STARTUPINFO structure for the process being created. /// + /// \param launch_info + /// The process launch configuration. + /// /// \param stdout_handle /// \param stderr_handle /// \param stdin_handle @@ -49,12 +51,73 @@ class ProcessLauncherWindows : public ProcessLauncher { /// \returns /// `std::vector<HANDLE>` containing all handles that the child must /// inherit. - llvm::ErrorOr<std::vector<HANDLE>> - GetInheritedHandles(const ProcessLaunchInfo &launch_info, - STARTUPINFOEXW &startupinfoex, + static llvm::ErrorOr<std::vector<HANDLE>> + GetInheritedHandles(STARTUPINFOEXW &startupinfoex, + const ProcessLaunchInfo *launch_info = nullptr, HANDLE stdout_handle = NULL, HANDLE stderr_handle = NULL, HANDLE stdin_handle = NULL); + + static HANDLE GetStdioHandle(const ProcessLaunchInfo &launch_info, int fd); + + /// Creates a file handle suitable for redirecting stdin, stdout, + /// or stderr of a child process. + /// + /// \param path The file path to open. If empty, returns NULL (no + /// redirection). + /// \param fd The file descriptor type: STDIN_FILENO, STDOUT_FILENO, or + /// STDERR_FILENO. + /// + /// \return A handle to the opened file, or NULL if the path is empty or the + /// file + /// cannot be opened (INVALID_HANDLE_VALUE is converted to NULL). + /// + /// Behavior by file descriptor: + /// - STDIN_FILENO: Opens existing file for reading (GENERIC_READ, + /// OPEN_EXISTING). + /// - STDOUT_FILENO: Creates/truncates file for writing (GENERIC_WRITE, + /// CREATE_ALWAYS). + /// - STDERR_FILENO: Creates/truncates file for writing with write-through + /// (FILE_FLAG_WRITE_THROUGH ensures immediate disk writes, + /// bypassing system cache for error messages). + /// + /// All handles are created with: + /// - Inheritance enabled (bInheritHandle = TRUE) so child processes can use + /// them. + /// - Shared read/write/delete access to allow other processes to access the + /// file. + static HANDLE GetStdioHandle(const llvm::StringRef path, int fd); }; + +/// Flattens an Args object into a Windows command-line wide string. +/// +/// Returns an empty string if args is empty. +/// +/// \param args The Args object to flatten. +/// \returns A wide string containing the flattened command line. +llvm::ErrorOr<std::wstring> GetFlattenedWindowsCommandStringW(Args args); + +/// Flattens an Args object into a Windows command-line wide string. +/// +/// Returns an empty string if args is empty. +/// +/// \param args The Args object to flatten. +/// \returns A wide string containing the flattened command line. +llvm::ErrorOr<std::wstring> GetFlattenedWindowsCommandStringW(char *args[]); + +/// Allocate and initialize a PROC_THREAD_ATTRIBUTE_LIST structure +/// that can be used with CreateProcess to specify extended process creation +/// attributes (such as inherited handles). +/// +/// \param[in] startupinfoex The STARTUPINFOEXW structure whose lpAttributeList +/// will +/// be initialized. +/// +/// \return On success, returns a scope_exit cleanup object that will +/// automatically +/// delete and free the attribute list when it goes out of scope. +/// On failure, returns the corresponding Windows error code. +llvm::ErrorOr<llvm::scope_exit<std::function<void()>>> +SetupProcThreadAttributeList(STARTUPINFOEXW &startupinfoex); } #endif diff --git a/lldb/packages/Python/lldbsuite/test/dotest.py b/lldb/packages/Python/lldbsuite/test/dotest.py index fcd60a1bc0db8..5b41fc44b35b6 100644 --- a/lldb/packages/Python/lldbsuite/test/dotest.py +++ b/lldb/packages/Python/lldbsuite/test/dotest.py @@ -566,7 +566,10 @@ def setupSysPath(): lldbDir = os.path.dirname(lldbtest_config.lldbExec) - lldbDAPExec = os.path.join(lldbDir, "lldb-dap") + lldbDAPExecName = "lldb-dap" + if sys.platform == "win32": + lldbDAPExecName += ".exe" + lldbDAPExec = os.path.join(lldbDir, lldbDAPExecName) if is_exe(lldbDAPExec): os.environ["LLDBDAP_EXEC"] = lldbDAPExec diff --git a/lldb/source/Host/windows/ProcessLauncherWindows.cpp b/lldb/source/Host/windows/ProcessLauncherWindows.cpp index 4b7cbec828dd8..3d986025756d0 100644 --- a/lldb/source/Host/windows/ProcessLauncherWindows.cpp +++ b/lldb/source/Host/windows/ProcessLauncherWindows.cpp @@ -8,11 +8,9 @@ #include "lldb/Host/windows/ProcessLauncherWindows.h" #include "lldb/Host/HostProcess.h" -#include "lldb/Host/ProcessLaunchInfo.h" #include "lldb/Host/windows/PseudoConsole.h" #include "lldb/Host/windows/windows.h" -#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/SmallVector.h" #include "llvm/Support/ConvertUTF.h" #include "llvm/Support/Program.h" @@ -65,14 +63,9 @@ static std::vector<wchar_t> CreateEnvironmentBufferW(const Environment &env) { return buffer; } -/// Flattens an Args object into a Windows command-line wide string. -/// -/// Returns an empty string if args is empty. -/// -/// \param args The Args object to flatten. -/// \returns A wide string containing the flattened command line. -static llvm::ErrorOr<std::wstring> -GetFlattenedWindowsCommandStringW(Args args) { +namespace lldb_private { + +llvm::ErrorOr<std::wstring> GetFlattenedWindowsCommandStringW(Args args) { if (args.empty()) return L""; @@ -83,6 +76,45 @@ GetFlattenedWindowsCommandStringW(Args args) { return llvm::sys::flattenWindowsCommandLine(args_ref); } +llvm::ErrorOr<std::wstring> GetFlattenedWindowsCommandStringW(char *args[]) { + std::vector<llvm::StringRef> args_ref; + for (int i = 0; args[i] != nullptr; ++i) { + args_ref.push_back(args[i]); + } + + return llvm::sys::flattenWindowsCommandLine(args_ref); +} + +llvm::ErrorOr<llvm::scope_exit<std::function<void()>>> +SetupProcThreadAttributeList(STARTUPINFOEXW &startupinfoex) { + SIZE_T attributelist_size = 0; + InitializeProcThreadAttributeList(/*lpAttributeList=*/nullptr, + /*dwAttributeCount=*/1, /*dwFlags=*/0, + &attributelist_size); + + startupinfoex.lpAttributeList = + static_cast<LPPROC_THREAD_ATTRIBUTE_LIST>(malloc(attributelist_size)); + + if (!startupinfoex.lpAttributeList) + return llvm::mapWindowsError(ERROR_OUTOFMEMORY); + + auto cleanup = llvm::scope_exit<std::function<void()>>([&startupinfoex] { + if (startupinfoex.lpAttributeList) { + DeleteProcThreadAttributeList(startupinfoex.lpAttributeList); + free(startupinfoex.lpAttributeList); + startupinfoex.lpAttributeList = nullptr; + } + }); + + if (!InitializeProcThreadAttributeList(startupinfoex.lpAttributeList, + /*dwAttributeCount=*/1, /*dwFlags=*/0, + &attributelist_size)) + return llvm::mapWindowsError(GetLastError()); + + return std::move(cleanup); +} +} // namespace lldb_private + HostProcess ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info, Status &error) { @@ -108,24 +140,13 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info, if (stderr_handle) ::CloseHandle(stderr_handle); }); - - SIZE_T attributelist_size = 0; - InitializeProcThreadAttributeList(/*lpAttributeList=*/nullptr, - /*dwAttributeCount=*/1, /*dwFlags=*/0, - &attributelist_size); - - startupinfoex.lpAttributeList = - static_cast<LPPROC_THREAD_ATTRIBUTE_LIST>(malloc(attributelist_size)); - llvm::scope_exit free_attributelist( - [&] { free(startupinfoex.lpAttributeList); }); - if (!InitializeProcThreadAttributeList(startupinfoex.lpAttributeList, - /*dwAttributeCount=*/1, /*dwFlags=*/0, - &attributelist_size)) { - error = Status(::GetLastError(), eErrorTypeWin32); + auto attributelist_cleanup_or_err = + lldb_private::SetupProcThreadAttributeList(startupinfoex); + if (!attributelist_cleanup_or_err) { + error = attributelist_cleanup_or_err.getError(); return HostProcess(); } - llvm::scope_exit delete_attributelist( - [&] { DeleteProcThreadAttributeList(startupinfoex.lpAttributeList); }); + auto attributelist_cleanup = std::move(*attributelist_cleanup_or_err); std::vector<HANDLE> inherited_handles; if (use_pty) { @@ -136,8 +157,9 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info, return HostProcess(); } } else { - auto inherited_handles_or_err = GetInheritedHandles( - launch_info, startupinfoex, stdout_handle, stderr_handle, stdin_handle); + auto inherited_handles_or_err = + GetInheritedHandles(startupinfoex, &launch_info, stdout_handle, + stderr_handle, stdin_handle); if (!inherited_handles_or_err) { error = Status(inherited_handles_or_err.getError()); return HostProcess(); @@ -210,7 +232,7 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info, } llvm::ErrorOr<std::vector<HANDLE>> ProcessLauncherWindows::GetInheritedHandles( - const ProcessLaunchInfo &launch_info, STARTUPINFOEXW &startupinfoex, + STARTUPINFOEXW &startupinfoex, const ProcessLaunchInfo *launch_info, HANDLE stdout_handle, HANDLE stderr_handle, HANDLE stdin_handle) { std::vector<HANDLE> inherited_handles; @@ -228,12 +250,13 @@ llvm::ErrorOr<std::vector<HANDLE>> ProcessLauncherWindows::GetInheritedHandles( if (startupinfoex.StartupInfo.hStdOutput) inherited_handles.push_back(startupinfoex.StartupInfo.hStdOutput); - for (size_t i = 0; i < launch_info.GetNumFileActions(); ++i) { - const FileAction *act = launch_info.GetFileActionAtIndex(i); - if (act->GetAction() == FileAction::eFileActionDuplicate && - act->GetFD() == act->GetActionArgument()) - inherited_handles.push_back(reinterpret_cast<HANDLE>(act->GetFD())); - } + if (launch_info) + for (size_t i = 0; i < launch_info->GetNumFileActions(); ++i) { + const FileAction *act = launch_info->GetFileActionAtIndex(i); + if (act->GetAction() == FileAction::eFileActionDuplicate && + act->GetFD() == act->GetActionArgument()) + inherited_handles.push_back(reinterpret_cast<HANDLE>(act->GetFD())); + } if (inherited_handles.empty()) return inherited_handles; @@ -254,6 +277,15 @@ ProcessLauncherWindows::GetStdioHandle(const ProcessLaunchInfo &launch_info, const FileAction *action = launch_info.GetFileActionForFD(fd); if (action == nullptr) return NULL; + const std::string path = action->GetFileSpec().GetPath(); + + return GetStdioHandle(path, fd); +} + +HANDLE ProcessLauncherWindows::GetStdioHandle(const llvm::StringRef path, + int fd) { + if (path.empty()) + return NULL; SECURITY_ATTRIBUTES secattr = {}; secattr.nLength = sizeof(SECURITY_ATTRIBUTES); secattr.bInheritHandle = TRUE; @@ -274,7 +306,6 @@ ProcessLauncherWindows::GetStdioHandle(const ProcessLaunchInfo &launch_info, flags = FILE_FLAG_WRITE_THROUGH; } - const std::string path = action->GetFileSpec().GetPath(); std::wstring wpath; llvm::ConvertUTF8toWide(path, wpath); HANDLE result = ::CreateFileW(wpath.c_str(), access, share, &secattr, create, 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 d7e0168239f34..adcb33cd658b2 100644 --- a/lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py +++ b/lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py @@ -2,6 +2,7 @@ Test lldb-dap runInTerminal reverse request """ +from contextlib import contextmanager from lldbsuite.test.decorators import * from lldbsuite.test.lldbtest import line_number import lldbdap_testcase @@ -10,24 +11,90 @@ import json +@contextmanager +def fifo(*args, **kwargs): + if sys.platform == "win32": + import ctypes + + comm_file = r"\\.\pipe\lldb-dap-run-in-terminal-comm" + PIPE_ACCESS_DUPLEX = 0x00000003 + PIPE_TYPE_BYTE = 0x00000000 + PIPE_READMODE_BYTE = 0x00000000 + PIPE_WAIT = 0x00000000 + PIPE_UNLIMITED_INSTANCES = 255 + kernel32 = ctypes.windll.kernel32 + + pipe = kernel32.CreateNamedPipeW( + comm_file, + PIPE_ACCESS_DUPLEX, + PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT, + PIPE_UNLIMITED_INSTANCES, + 4096, + 4096, + 0, + None, + ) + else: + comm_file = os.path.join(kwargs["directory"], "comm-file") + pipe = None + os.mkfifo(comm_file) + + try: + yield comm_file, pipe + finally: + if pipe is not None: + kernel32.DisconnectNamedPipe(pipe) + kernel32.CloseHandle(pipe) + + @skipIfBuildType(["debug"]) class TestDAP_runInTerminal(lldbdap_testcase.DAPTestCaseBase): - def read_pid_message(self, fifo_file): - with open(fifo_file, "r") as file: - self.assertIn("pid", file.readline()) + def read_pid_message(self, fifo_file, pipe): + if sys.platform == "win32": + import ctypes + + buffer = ctypes.create_string_buffer(4096) + bytes_read = ctypes.wintypes.DWORD() + kernel32 = ctypes.windll.kernel32 + kernel32.ConnectNamedPipe(pipe, None) + kernel32.ReadFile(pipe, buffer, 4096, ctypes.byref(bytes_read), None) + self.assertIn("pid", buffer.value.decode()) + else: + with open(fifo_file, "r") as file: + self.assertIn("pid", file.readline()) @staticmethod - def send_did_attach_message(fifo_file): - with open(fifo_file, "w") as file: - file.write(json.dumps({"kind": "didAttach"}) + "\n") + def send_did_attach_message(fifo_file, pipe=None): + message = json.dumps({"kind": "didAttach"}) + "\n" + if sys.platform == "win32": + import ctypes + + kernel32 = ctypes.windll.kernel32 + bytes_written = ctypes.wintypes.DWORD() + kernel32.ConnectNamedPipe(pipe, None) + kernel32.WriteFile( + pipe, message.encode(), len(message), ctypes.byref(bytes_written), None + ) + else: + with open(fifo_file, "w") as file: + file.write(message) @staticmethod - def read_error_message(fifo_file): - with open(fifo_file, "r") as file: - return file.readline() + def read_error_message(fifo_file, pipe=None): + if sys.platform == "win32": + import ctypes + + buffer = ctypes.create_string_buffer(4096) + bytes_read = ctypes.wintypes.DWORD() + kernel32 = ctypes.windll.kernel32 + kernel32.ConnectNamedPipe(pipe, None) + kernel32.ReadFile(pipe, buffer, 4096, ctypes.byref(bytes_read), None) + return buffer.value.decode() + else: + with open(fifo_file, "r") as file: + return file.readline() @skipIfAsan - @skipIfWindows def test_runInTerminal(self): """ Tests the "runInTerminal" reverse request. It makes sure that the IDE can @@ -74,7 +141,6 @@ def test_runInTerminal(self): self.continue_to_exit() @skipIfAsan - @skipIfWindows def test_runInTerminalWithObjectEnv(self): """ Tests the "runInTerminal" reverse request. It makes sure that the IDE can @@ -97,7 +163,6 @@ def test_runInTerminalWithObjectEnv(self): self.continue_to_exit() - @skipIfWindows def test_runInTerminalInvalidTarget(self): self.build_and_create_debug_adapter() response = self.launch( @@ -113,7 +178,6 @@ def test_runInTerminalInvalidTarget(self): response["body"]["error"]["format"], ) - @skipIfWindows def test_missingArgInRunInTerminalLauncher(self): proc = subprocess.run( [self.lldbDAPExec, "--launch-target", "INVALIDPROGRAM"], @@ -149,44 +213,58 @@ def test_FakeAttachedRunInTerminalLauncherWithInvalidProgram(self): _, stderr = proc.communicate() self.assertIn("No such file or directory", stderr) - @skipIfWindows - def test_FakeAttachedRunInTerminalLauncherWithValidProgram(self): - 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, - ) + @skipUnlessWindows + def test_FakeAttachedRunInTerminalLauncherWithInvalidProgramWindows(self): + with fifo(directory=self.getBuildDir()) as (comm_file, pipe): + subprocess.Popen( + [ + self.lldbDAPExec, + "--comm-file", + comm_file, + "--launch-target", + "INVALIDPROGRAM", + ], + universal_newlines=True, + stderr=subprocess.PIPE, + ) + + self.assertIn( + "Failed to launch target process", + self.read_error_message(comm_file, pipe), + ) - self.read_pid_message(comm_file) - self.send_did_attach_message(comm_file) + def test_FakeAttachedRunInTerminalLauncherWithValidProgram(self): + with fifo(directory=self.getBuildDir()) as (comm_file, pipe): + proc = subprocess.Popen( + [ + self.lldbDAPExec, + "--comm-file", + comm_file, + "--launch-target", + "echo", + "foo", + ], + universal_newlines=True, + stdout=subprocess.PIPE, + ) + + self.read_pid_message(comm_file, pipe) + self.send_did_attach_message(comm_file, pipe) stdout, _ = proc.communicate() self.assertIn("foo", stdout) - @skipIfWindows def test_FakeAttachedRunInTerminalLauncherAndCheckEnvironment(self): - 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"}, - ) - - ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/174635 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
