llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Charles Zablit (charles-zablit) <details> <summary>Changes</summary> This patch refactors the FileAction class and its callsites to properly handle Windows file handles. Before this patch, we were casting Windows file handles to `int` to convert them to file descriptors and casting them back to a Handle when using them. This is error prone. This patch refactors the callsites casting a Handle to an (int) to make sure they are properly converted to file descriptors and back. --- Full diff: https://github.com/llvm/llvm-project/pull/179274.diff 6 Files Affected: - (modified) lldb/include/lldb/Host/FileAction.h (+61-6) - (modified) lldb/source/Host/common/FileAction.cpp (+18) - (modified) lldb/source/Host/windows/ProcessLauncherWindows.cpp (+17-11) - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp (+12-2) - (modified) lldb/tools/lldb-server/lldb-platform.cpp (+6-2) - (modified) lldb/unittests/Host/HostTest.cpp (+6-2) ``````````diff diff --git a/lldb/include/lldb/Host/FileAction.h b/lldb/include/lldb/Host/FileAction.h index b2cc8be32d296..7d66dbe7fa440 100644 --- a/lldb/include/lldb/Host/FileAction.h +++ b/lldb/include/lldb/Host/FileAction.h @@ -10,10 +10,16 @@ #define LLDB_HOST_FILEACTION_H #include "lldb/Utility/FileSpec.h" +#include "lldb/lldb-types.h" #include <string> +#include <variant> namespace lldb_private { +/// Represents a file descriptor action to be performed during process launch. +/// +/// FileAction encapsulates operations like opening, closing, or duplicating +/// file descriptors that should be applied when spawning a new process. class FileAction { public: enum Action { @@ -25,30 +31,79 @@ class FileAction { FileAction(); + /// Reset this FileAction to its default state. void Clear(); + /// Configure this action to close a file descriptor. bool Close(int fd); - bool Duplicate(int fd, int dup_fd); + /// Configure this action to duplicate a file descriptor. + /// + /// \param[in] fd + /// The file descriptor to duplicate. + /// \param[in] dup_file + /// The target file descriptor number. + bool Duplicate(int fd, int dup_file); + +#ifdef _WIN32 + /// Configure this action to open a file (Windows handle version). + /// + /// This method will open a CRT file descriptor to the handle and + /// store that descriptor internally. + /// + /// \param[in] fh + /// The file handle to use for the opened file. + /// \param[in] file_spec + /// The file to open. + /// \param[in] read + /// Open for reading. + /// \param[in] write + /// Open for writing. + bool Open(void *fh, const FileSpec &file_spec, bool read, bool write); +#endif + /// Configure this action to open a file. + /// + /// \param[in] fd + /// The file descriptor to use for the opened file. + /// \param[in] file_spec + /// The file to open. + /// \param[in] read + /// Open for reading. + /// \param[in] write + /// Open for writing. bool Open(int fd, const FileSpec &file_spec, bool read, bool write); + /// Get the file descriptor this action applies to. int GetFD() const { return m_fd; } +#ifdef _WIN32 + /// Get the Windows handle for this file descriptor. + /// + /// The handle is converted from the file descriptor which is stored internally. + /// The initial file descriptor must have been registered in the CRT before. + void* GetHandle() const; +#endif + + /// Get the type of action. Action GetAction() const { return m_action; } + /// Get the action-specific argument. + /// + /// For eFileActionOpen, returns the open flags (O_RDONLY, etc.). + /// For eFileActionDuplicate, returns the target fd to duplicate to. int GetActionArgument() const { return m_arg; } + /// Get the file specification for open actions. const FileSpec &GetFileSpec() const; void Dump(Stream &stream) const; protected: - Action m_action = eFileActionNone; // The action for this file - int m_fd = -1; // An existing file descriptor - int m_arg = -1; // oflag for eFileActionOpen*, dup_fd for eFileActionDuplicate - FileSpec - m_file_spec; // A file spec to use for opening after fork or posix_spawn + Action m_action = eFileActionNone; ///< The action for this file. + int m_fd = -1; ///< An existing file descriptor. + int m_arg = -1; ///< oflag for eFileActionOpen, dup_fd for eFileActionDuplicate. + FileSpec m_file_spec; ///< File spec to use for opening after fork or posix_spawn. }; } // namespace lldb_private diff --git a/lldb/source/Host/common/FileAction.cpp b/lldb/source/Host/common/FileAction.cpp index ec271f7b920d8..5ae9eead2bb96 100644 --- a/lldb/source/Host/common/FileAction.cpp +++ b/lldb/source/Host/common/FileAction.cpp @@ -12,6 +12,10 @@ #include "lldb/Host/PosixApi.h" #include "lldb/Utility/Stream.h" +#ifdef _WIN32 +#include "lldb/Host/windows/windows.h" +#endif + using namespace lldb_private; // FileAction member functions @@ -25,8 +29,22 @@ void FileAction::Clear() { m_file_spec.Clear(); } +#ifdef _WIN32 +HANDLE FileAction::GetHandle() const { + return (HANDLE)_get_osfhandle(m_fd); +#endif +} + const FileSpec &FileAction::GetFileSpec() const { return m_file_spec; } +#ifdef _WIN32 +bool FileAction::Open(HANDLE fh, const FileSpec &file_spec, bool read, + bool write) { + int fd = _open_osfhandle((intptr_t)fh, NULL); + return Open(fd, file_spec, read, write); +} +#endif + bool FileAction::Open(int fd, const FileSpec &file_spec, bool read, bool write) { if ((read || write) && fd >= 0 && file_spec) { diff --git a/lldb/source/Host/windows/ProcessLauncherWindows.cpp b/lldb/source/Host/windows/ProcessLauncherWindows.cpp index ec1a20ebb2200..8afbf1bf1c38b 100644 --- a/lldb/source/Host/windows/ProcessLauncherWindows.cpp +++ b/lldb/source/Host/windows/ProcessLauncherWindows.cpp @@ -223,7 +223,7 @@ ProcessLauncherWindows::LaunchProcess(const ProcessLaunchInfo &launch_info, llvm::ErrorOr<std::vector<HANDLE>> ProcessLauncherWindows::GetInheritedHandles( const ProcessLaunchInfo &launch_info, STARTUPINFOEXW &startupinfoex, HANDLE stdout_handle, HANDLE stderr_handle, HANDLE stdin_handle) { - std::vector<HANDLE> inherited_handles; + std::set<HANDLE> unique_handles; startupinfoex.StartupInfo.hStdError = stderr_handle ? stderr_handle : GetStdHandle(STD_ERROR_HANDLE); @@ -233,21 +233,23 @@ llvm::ErrorOr<std::vector<HANDLE>> ProcessLauncherWindows::GetInheritedHandles( stdout_handle ? stdout_handle : GetStdHandle(STD_OUTPUT_HANDLE); if (startupinfoex.StartupInfo.hStdError) - inherited_handles.push_back(startupinfoex.StartupInfo.hStdError); + unique_handles.insert(startupinfoex.StartupInfo.hStdError); if (startupinfoex.StartupInfo.hStdInput) - inherited_handles.push_back(startupinfoex.StartupInfo.hStdInput); + unique_handles.insert(startupinfoex.StartupInfo.hStdInput); if (startupinfoex.StartupInfo.hStdOutput) - inherited_handles.push_back(startupinfoex.StartupInfo.hStdOutput); + unique_handles.insert(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())); + unique_handles.insert(act->GetHandle()); } + std::vector<HANDLE> inherited_handles(unique_handles.begin(), unique_handles.end()); + if (inherited_handles.empty()) - return inherited_handles; + return std::vector<HANDLE>{}; if (!UpdateProcThreadAttribute( startupinfoex.lpAttributeList, /*dwFlags=*/0, @@ -273,16 +275,20 @@ ProcessLauncherWindows::GetStdioHandle(const ProcessLaunchInfo &launch_info, DWORD share = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE; DWORD create = 0; DWORD flags = 0; - if (fd == STDIN_FILENO) { + switch (fd) { + case STDIN_FILENO: access = GENERIC_READ; create = OPEN_EXISTING; flags = FILE_ATTRIBUTE_READONLY; - } - if (fd == STDOUT_FILENO || fd == STDERR_FILENO) { + break; + case STDERR_FILENO: + flags = FILE_FLAG_WRITE_THROUGH; + case STDOUT_FILENO: access = GENERIC_WRITE; create = CREATE_ALWAYS; - if (fd == STDERR_FILENO) - flags = FILE_FLAG_WRITE_THROUGH; + break; + default: + break; } const std::string path = action->GetFileSpec().GetPath(); diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp index d7e4b2b9546b2..264b860165b28 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp @@ -860,7 +860,12 @@ Status GDBRemoteCommunication::StartDebugserverProcess( debugserver_args.AppendArgument(llvm::formatv("--fd={0}", *comm_fd).str()); // Send "comm_fd" down to the inferior so it can use it to communicate back // with this process. - launch_info.AppendDuplicateFileAction((int64_t)*comm_fd, (int64_t)*comm_fd); + #ifdef _WIN32 + int fd = _open_osfhandle((intptr_t)*comm_fd, NULL); + #else + int fd = *commd_fd; + #endif + launch_info.AppendDuplicateFileAction(fd, fd); } else { llvm::StringRef url = std::get<llvm::StringRef>(comm); LLDB_LOG(log, "debugserver listens on: {0}", url); @@ -886,7 +891,12 @@ Status GDBRemoteCommunication::StartDebugserverProcess( pipe_t write = socket_pipe.GetWritePipe(); debugserver_args.AppendArgument(llvm::StringRef("--pipe")); debugserver_args.AppendArgument(llvm::to_string(write)); - launch_info.AppendDuplicateFileAction((int64_t)write, (int64_t)write); + #ifdef _WIN32 + int write_fd = _open_osfhandle((intptr_t)*comm_fd, NULL); + #else + int write_fd = *commd_fd; + #endif + launch_info.AppendDuplicateFileAction(write_fd, write_fd); #endif } diff --git a/lldb/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp index 59b1eb419bc2b..1493c149a4eb2 100644 --- a/lldb/tools/lldb-server/lldb-platform.cpp +++ b/lldb/tools/lldb-server/lldb-platform.cpp @@ -326,8 +326,12 @@ static Status spawn_process(const char *progname, const FileSpec &prog, self_args.AppendArgument(llvm::StringRef("platform")); self_args.AppendArgument(llvm::StringRef("--child-platform-fd")); self_args.AppendArgument(llvm::to_string(shared_socket.GetSendableFD())); - launch_info.AppendDuplicateFileAction((int64_t)shared_socket.GetSendableFD(), - (int64_t)shared_socket.GetSendableFD()); + #ifdef _WIN32 + int fd = _open_osfhandle((intptr_t)shared_socket.GetSendableFD(), NULL); + #else + int fd = shared_socket.GetSendableFD(); + #endif + launch_info.AppendDuplicateFileAction(fd, fd); if (gdb_port) { self_args.AppendArgument(llvm::StringRef("--gdbserver-port")); self_args.AppendArgument(llvm::to_string(gdb_port)); diff --git a/lldb/unittests/Host/HostTest.cpp b/lldb/unittests/Host/HostTest.cpp index c88c764f24646..2429989218508 100644 --- a/lldb/unittests/Host/HostTest.cpp +++ b/lldb/unittests/Host/HostTest.cpp @@ -157,8 +157,12 @@ TEST(Host, LaunchProcessDuplicatesHandle) { "--gtest_filter=Host.LaunchProcessDuplicatesHandle"); info.GetArguments().AppendArgument( ("--test-arg=" + llvm::Twine((uint64_t)pipe.GetWritePipe())).str()); - info.AppendDuplicateFileAction((uint64_t)pipe.GetWritePipe(), - (uint64_t)pipe.GetWritePipe()); + #ifdef _WIN32 + int fd = _open_osfhandle((intptr_t)pipe.GetWritePipe(), NULL); + #else + int fd = pipe.GetWritePipe(); + #endif + info.AppendDuplicateFileAction(fd, fd); info.SetMonitorProcessCallback(&ProcessLaunchInfo::NoOpMonitorCallback); ASSERT_THAT_ERROR(Host::LaunchProcess(info).takeError(), llvm::Succeeded()); pipe.CloseWriteFileDescriptor(); `````````` </details> https://github.com/llvm/llvm-project/pull/179274 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
