From 3a14249525235d9d2bcbc0e931759d083bcdb0d1 Mon Sep 17 00:00:00 2001 From: Aaron Smith Date: Thu, 7 Feb 2019 18:46:25 +0000 Subject: [PATCH] [lldb-server] Improve support on Windows Summary: This commit contains the following changes: - Rewrite vfile close/read/write packet handlers with portable routines from lldb. This removes #if(s) and allows the handlers to work on Windows. - Fix a bug in File::Write. This is intended to write data at an offset to a file but actually writes at the current position of the file. - Add a default boolean argument 'should_close_fd' to FileSystem::Open to let the user decide whether to close the fd or not. Reviewers: zturner, llvm-commits, labath Reviewed By: zturner Subscribers: Hui, labath, abidh, lldb-commits Differential Revision: https://reviews.llvm.org/D56231 llvm-svn: 353446 --- lldb/include/lldb/Host/File.h | 2 + lldb/include/lldb/Host/FileSystem.h | 3 +- lldb/source/Host/common/File.cpp | 5 +- lldb/source/Host/common/FileSystem.cpp | 4 +- .../GDBRemoteCommunicationServerCommon.cpp | 61 +++++++++---------- 5 files changed, 38 insertions(+), 37 deletions(-) diff --git a/lldb/include/lldb/Host/File.h b/lldb/include/lldb/Host/File.h index 8e3d97178e7c..143ea829c5ec 100644 --- a/lldb/include/lldb/Host/File.h +++ b/lldb/include/lldb/Host/File.h @@ -14,6 +14,7 @@ #include "lldb/Utility/Status.h" #include "lldb/lldb-private.h" +#include #include #include #include @@ -420,6 +421,7 @@ protected: LazyBool m_is_interactive; LazyBool m_is_real_terminal; LazyBool m_supports_colors; + std::mutex offset_access_mutex; private: DISALLOW_COPY_AND_ASSIGN(File); diff --git a/lldb/include/lldb/Host/FileSystem.h b/lldb/include/lldb/Host/FileSystem.h index c8da1d1f1657..fe0386c601dd 100644 --- a/lldb/include/lldb/Host/FileSystem.h +++ b/lldb/include/lldb/Host/FileSystem.h @@ -64,7 +64,8 @@ public: int Open(const char *path, int flags, int mode); Status Open(File &File, const FileSpec &file_spec, uint32_t options, - uint32_t permissions = lldb::eFilePermissionsFileDefault); + uint32_t permissions = lldb::eFilePermissionsFileDefault, + bool should_close_fd = true); /// Get a directory iterator. /// @{ diff --git a/lldb/source/Host/common/File.cpp b/lldb/source/Host/common/File.cpp index dba6e08acb3b..512e7c587024 100644 --- a/lldb/source/Host/common/File.cpp +++ b/lldb/source/Host/common/File.cpp @@ -178,7 +178,7 @@ Status File::Close() { void File::Clear() { m_stream = nullptr; - m_descriptor = -1; + m_descriptor = kInvalidDescriptor; m_options = 0; m_own_stream = false; m_is_interactive = m_supports_colors = m_is_real_terminal = @@ -503,6 +503,7 @@ Status File::Read(void *buf, size_t &num_bytes, off_t &offset) { error.SetErrorString("invalid file handle"); } #else + std::lock_guard guard(offset_access_mutex); long cur = ::lseek(m_descriptor, 0, SEEK_CUR); SeekFromStart(offset); error = Read(buf, num_bytes); @@ -602,7 +603,9 @@ Status File::Write(const void *buf, size_t &num_bytes, off_t &offset) { num_bytes = bytes_written; } #else + std::lock_guard guard(offset_access_mutex); long cur = ::lseek(m_descriptor, 0, SEEK_CUR); + SeekFromStart(offset); error = Write(buf, num_bytes); long after = ::lseek(m_descriptor, 0, SEEK_CUR); diff --git a/lldb/source/Host/common/FileSystem.cpp b/lldb/source/Host/common/FileSystem.cpp index 6fbd7bc9f5aa..33159626acc0 100644 --- a/lldb/source/Host/common/FileSystem.cpp +++ b/lldb/source/Host/common/FileSystem.cpp @@ -408,7 +408,7 @@ static mode_t GetOpenMode(uint32_t permissions) { } Status FileSystem::Open(File &File, const FileSpec &file_spec, uint32_t options, - uint32_t permissions) { + uint32_t permissions, bool should_close_fd) { if (m_collector) m_collector->AddFile(file_spec); @@ -431,7 +431,7 @@ Status FileSystem::Open(File &File, const FileSpec &file_spec, uint32_t options, File.SetDescriptor(descriptor, false); error.SetErrorToErrno(); } else { - File.SetDescriptor(descriptor, true); + File.SetDescriptor(descriptor, should_close_fd); File.SetOptions(options); } return error; diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp index af5e66c22db2..13e7748ce0ab 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp @@ -10,7 +10,6 @@ #include - #ifdef __APPLE__ #include #endif @@ -214,8 +213,7 @@ GDBRemoteCommunicationServerCommon::Handle_qHostInfo( if (sub != LLDB_INVALID_CPUTYPE) response.Printf("cpusubtype:%u;", sub); - if (cpu == llvm::MachO::CPU_TYPE_ARM - || cpu == llvm::MachO::CPU_TYPE_ARM64) { + if (cpu == llvm::MachO::CPU_TYPE_ARM || cpu == llvm::MachO::CPU_TYPE_ARM64) { // Indicate the OS type. #if defined(TARGET_OS_TV) && TARGET_OS_TV == 1 response.PutCString("ostype:tvos;"); @@ -510,18 +508,19 @@ GDBRemoteCommunicationServerCommon::Handle_vFile_Open( packet.GetHexByteStringTerminatedBy(path, ','); if (!path.empty()) { if (packet.GetChar() == ',') { - uint32_t flags = - File::ConvertOpenOptionsForPOSIXOpen(packet.GetHexMaxU32(false, 0)); + uint32_t flags = packet.GetHexMaxU32(false, 0); if (packet.GetChar() == ',') { mode_t mode = packet.GetHexMaxU32(false, 0600); - Status error; FileSpec path_spec(path); FileSystem::Instance().Resolve(path_spec); - int fd = ::open(path_spec.GetCString(), flags, mode); - const int save_errno = fd == -1 ? errno : 0; + File file; + // Do not close fd. + Status error = + FileSystem::Instance().Open(file, path_spec, flags, mode, false); + const int save_errno = error.GetError(); StreamString response; response.PutChar('F'); - response.Printf("%i", fd); + response.Printf("%i", file.GetDescriptor()); if (save_errno) response.Printf(",%i", save_errno); return SendPacketNoLock(response.GetString()); @@ -536,12 +535,13 @@ GDBRemoteCommunicationServerCommon::Handle_vFile_Close( StringExtractorGDBRemote &packet) { packet.SetFilePos(::strlen("vFile:close:")); int fd = packet.GetS32(-1); - Status error; int err = -1; int save_errno = 0; if (fd >= 0) { - err = close(fd); - save_errno = err == -1 ? errno : 0; + File file(fd, true); + Status error = file.Close(); + err = 0; + save_errno = error.GetError(); } else { save_errno = EINVAL; } @@ -556,26 +556,23 @@ GDBRemoteCommunicationServerCommon::Handle_vFile_Close( GDBRemoteCommunication::PacketResult GDBRemoteCommunicationServerCommon::Handle_vFile_pRead( StringExtractorGDBRemote &packet) { -#ifdef _WIN32 - // Not implemented on Windows - return SendUnimplementedResponse( - "GDBRemoteCommunicationServerCommon::Handle_vFile_pRead() unimplemented"); -#else StreamGDBRemote response; packet.SetFilePos(::strlen("vFile:pread:")); int fd = packet.GetS32(-1); if (packet.GetChar() == ',') { - uint64_t count = packet.GetU64(UINT64_MAX); + size_t count = packet.GetU64(UINT64_MAX); if (packet.GetChar() == ',') { - uint64_t offset = packet.GetU64(UINT32_MAX); + off_t offset = packet.GetU64(UINT32_MAX); if (count == UINT64_MAX) { response.Printf("F-1:%i", EINVAL); return SendPacketNoLock(response.GetString()); } std::string buffer(count, 0); - const ssize_t bytes_read = ::pread(fd, &buffer[0], buffer.size(), offset); - const int save_errno = bytes_read == -1 ? errno : 0; + File file(fd, false); + Status error = file.Read(static_cast(&buffer[0]), count, offset); + const ssize_t bytes_read = error.Success() ? count : -1; + const int save_errno = error.GetError(); response.PutChar('F'); response.Printf("%zi", bytes_read); if (save_errno) @@ -588,17 +585,11 @@ GDBRemoteCommunicationServerCommon::Handle_vFile_pRead( } } return SendErrorResponse(21); - -#endif } GDBRemoteCommunication::PacketResult GDBRemoteCommunicationServerCommon::Handle_vFile_pWrite( StringExtractorGDBRemote &packet) { -#ifdef _WIN32 - return SendUnimplementedResponse("GDBRemoteCommunicationServerCommon::Handle_" - "vFile_pWrite() unimplemented"); -#else packet.SetFilePos(::strlen("vFile:pwrite:")); StreamGDBRemote response; @@ -610,9 +601,12 @@ GDBRemoteCommunicationServerCommon::Handle_vFile_pWrite( if (packet.GetChar() == ',') { std::string buffer; if (packet.GetEscapedBinaryData(buffer)) { - const ssize_t bytes_written = - ::pwrite(fd, buffer.data(), buffer.size(), offset); - const int save_errno = bytes_written == -1 ? errno : 0; + File file(fd, false); + size_t count = buffer.size(); + Status error = + file.Write(static_cast(&buffer[0]), count, offset); + const ssize_t bytes_written = error.Success() ? count : -1; + const int save_errno = error.GetError(); response.Printf("%zi", bytes_written); if (save_errno) response.Printf(",%i", save_errno); @@ -623,7 +617,6 @@ GDBRemoteCommunicationServerCommon::Handle_vFile_pWrite( } } return SendErrorResponse(27); -#endif } GDBRemoteCommunication::PacketResult @@ -971,7 +964,8 @@ GDBRemoteCommunicationServerCommon::Handle_QLaunchArch( const uint32_t bytes_left = packet.GetBytesLeft(); if (bytes_left > 0) { const char *arch_triple = packet.Peek(); - m_process_launch_info.SetArchitecture(HostInfo::GetAugmentedArchSpec(arch_triple)); + m_process_launch_info.SetArchitecture( + HostInfo::GetAugmentedArchSpec(arch_triple)); return SendOKResponse(); } return SendErrorResponse(13); @@ -1085,7 +1079,8 @@ GDBRemoteCommunicationServerCommon::Handle_qModuleInfo( StreamGDBRemote response; if (uuid_str.empty()) { - auto Result = llvm::sys::fs::md5_contents(matched_module_spec.GetFileSpec().GetPath()); + auto Result = llvm::sys::fs::md5_contents( + matched_module_spec.GetFileSpec().GetPath()); if (!Result) return SendErrorResponse(5); response.PutCString("md5:");