[lldb-server] Improve support on Windows

[asmith]

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.

[labath]

Nice to see some action on the lldb-server for windows front. It looks like it should be easy to add a unit test for the File::Write bug you fixed. Can you add something like that?

[Hui]

With all the aaron's pending reviews on lldb-server, I could try the patch with the following platform apis.

Seems to me is working on Windows and no regression on Linux side. Some difference (performance) might be

(1) Previous, the vfile:write packet has a maximum as 1024 bytes, now it turns to 16384 byte from observations.
(2) FileSystem::Instance().Open will introduce some performance bumps however I think it is minor along with the ctor/dtor of the File

I think the python tests already cover the changes in this commit, especially for Linux.

Not applicable for Windows I think, since we need the availability of lldb-server.exe.

Remote:
./lldb-server.exe p --listen *:2000 --log-channels="lldb all:gdb-remote all:windows all"

LLDB:
platform select remote-windows
platform connect <URL>
file a.exe
r

source/Host/common/File.cpp

608

I think this line makes Windows equivalence since posix part is using 'pwrite' which means to write from a set offset in the file.

[zturner]

source/Host/common/File.cpp

607–609

Be careful here. pwrite on posix is atomic, which means that if multiple threads both use pwrite at different offsets, there is no race condition. The only way to do a similar atomic write-at-offset on Windows is with overlapped i/o., and it's a completely different API / interface.

I mention this because in a previous review Pavel suggested to change lldb-server to spawn 1 thread / connection instead of 1 process / connection, so this can be a real problem in such a scenario if they are racing for the same file. Probably they won't be, since each connection will use different output files, but we should at least be aware of (and perhaps document) the different in atomicity semantics here.

Alternatively, we could just err on the side of safety and put this in a mutex.

[asmith]

Added lock suggested in review

[asmith]

Any other comments on this one?

include/lldb/Host/File.h

 //===-- File.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 liblldb_File_h_
 #define liblldb_File_h_
 
 #include "lldb/Host/PosixApi.h"
 #include "lldb/Utility/IOObject.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/lldb-private.h"
 
+#include <mutex>
 #include <stdarg.h>
 #include <stdio.h>
 #include <sys/types.h>
 
 namespace lldb_private {
 
 //----------------------------------------------------------------------
 /// @class File File.h "lldb/Host/File.h"
protected:
   //------------------------------------------------------------------
   int m_descriptor;
   FILE *m_stream;
   uint32_t m_options;
   bool m_own_stream;
   LazyBool m_is_interactive;
   LazyBool m_is_real_terminal;
   LazyBool m_supports_colors;
+  std::mutex offset_access_mutex;
 
 private:
   DISALLOW_COPY_AND_ASSIGN(File);
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_File_h_

include/lldb/Host/FileSystem.h

public:
 
   /// Wraps ::fopen in a platform-independent way.
   FILE *Fopen(const char *path, const char *mode);
 
   /// Wraps ::open in a platform-independent way.
   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.
   /// @{
   llvm::vfs::directory_iterator DirBegin(const FileSpec &file_spec,
                                          std::error_code &ec);
   llvm::vfs::directory_iterator DirBegin(const llvm::Twine &dir,
                                          std::error_code &ec);
   /// @}

source/Host/common/File.cpp

Status File::Close() {
   m_should_close_fd = false;
   m_is_interactive = eLazyBoolCalculate;
   m_is_real_terminal = eLazyBoolCalculate;
   return error;
 }
 
 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 =
       eLazyBoolCalculate;
 }
 
 Status File::GetFileSpec(FileSpec &file_spec) const {
   Status error;
if (bytes_read < 0) {
       offset += bytes_read;
       num_bytes = bytes_read;
     }
   } else {
     num_bytes = 0;
     error.SetErrorString("invalid file handle");
   }
 #else
+  std::lock_guard<std::mutex> guard(offset_access_mutex);
   long cur = ::lseek(m_descriptor, 0, SEEK_CUR);
   SeekFromStart(offset);
   error = Read(buf, num_bytes);
   if (!error.Fail())
     SeekFromStart(cur);
 #endif
   return error;
 }
#ifndef _WIN32
     if (bytes_written < 0) {
       num_bytes = 0;
       error.SetErrorToErrno();
     } else {
       offset += bytes_written;
       num_bytes = bytes_written;
     }
 #else
+    std::lock_guard<std::mutex> 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);
 
     if (!error.Fail())
       SeekFromStart(cur);
 
     offset = after;
 #endif
   } else {

source/Host/common/FileSystem.cpp

static mode_t GetOpenMode(uint32_t permissions) {
   if (permissions & lldb::eFilePermissionsWorldWrite)
     mode |= S_IWOTH;
   if (permissions & lldb::eFilePermissionsWorldExecute)
     mode |= S_IXOTH;
   return mode;
 }
 
 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);
 
   if (File.IsValid())
     File.Close();
 
   const int open_flags = GetOpenFlags(options);
   const mode_t open_mode =
       (open_flags & O_CREAT) ? GetOpenMode(permissions) : 0;
 
   auto path = GetExternalPath(file_spec);
   if (!path)
     return Status(path.getError());
 
   int descriptor = llvm::sys::RetryAfterSignal(
       -1, OpenWithFS, *this, path->c_str(), open_flags, open_mode);
 
   Status error;
   if (!File::DescriptorIsValid(descriptor)) {
     File.SetDescriptor(descriptor, false);
     error.SetErrorToErrno();
   } else {
-    File.SetDescriptor(descriptor, true);
+    File.SetDescriptor(descriptor, should_close_fd);
     File.SetOptions(options);
   }
   return error;
 }
 
 ErrorOr<std::string> FileSystem::GetExternalPath(const llvm::Twine &path) {
   if (!m_mapped)
     return path.str();

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp

 //===-- GDBRemoteCommunicationServerCommon.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 "GDBRemoteCommunicationServerCommon.h"
 
 #include <errno.h>
 
-
 #ifdef __APPLE__
 #include <TargetConditionals.h>
 #endif
 
 #include <chrono>
 #include <cstring>
 
 #include "lldb/Core/ModuleSpec.h"
#if defined(__APPLE__)
   // Send out MachO info.
   uint32_t cpu = host_arch.GetMachOCPUType();
   uint32_t sub = host_arch.GetMachOCPUSubType();
   if (cpu != LLDB_INVALID_CPUTYPE)
     response.Printf("cputype:%u;", cpu);
   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;");
 #elif defined(TARGET_OS_WATCH) && TARGET_OS_WATCH == 1
     response.PutCString("ostype:watchos;");
 #elif defined(TARGET_OS_BRIDGE) && TARGET_OS_BRIDGE == 1
     response.PutCString("ostype:bridgeos;");
 #else

 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerCommon::Handle_vFile_Open(
     StringExtractorGDBRemote &packet) {
   packet.SetFilePos(::strlen("vFile:open:"));
   std::string path;
   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());
       }
     }
   }
   return SendErrorResponse(18);
 }
 
 GDBRemoteCommunication::PacketResult
 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;
   }
   StreamString response;
   response.PutChar('F');
   response.Printf("%i", err);
   if (save_errno)
     response.Printf(",%i", save_errno);
   return SendPacketNoLock(response.GetString());
 }
 
 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<void *>(&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)
         response.Printf(",%i", save_errno);
       else {
         response.PutChar(';');
         response.PutEscapedBytes(&buffer[0], bytes_read);
       }
       return SendPacketNoLock(response.GetString());
     }
   }
   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;
   response.PutChar('F');
 
   int fd = packet.GetU32(UINT32_MAX);
   if (packet.GetChar() == ',') {
     off_t offset = packet.GetU64(UINT32_MAX);
     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<const void *>(&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);
       } else {
         response.Printf("-1,%i", EINVAL);
       }
       return SendPacketNoLock(response.GetString());
     }
   }
   return SendErrorResponse(27);
-#endif
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerCommon::Handle_vFile_Size(
     StringExtractorGDBRemote &packet) {
   packet.SetFilePos(::strlen("vFile:size:"));
   std::string path;
   packet.GetHexByteString(path);

 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerCommon::Handle_QLaunchArch(
     StringExtractorGDBRemote &packet) {
   packet.SetFilePos(::strlen("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);
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerCommon::Handle_A(StringExtractorGDBRemote &packet) {
   // The 'A' packet is the most over designed packet ever here with redundant
GDBRemoteCommunicationServerCommon::Handle_qModuleInfo(
 
   const auto file_offset = matched_module_spec.GetObjectOffset();
   const auto file_size = matched_module_spec.GetObjectSize();
   const auto uuid_str = matched_module_spec.GetUUID().GetAsString("");
 
   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:");
     response.PutCStringAsRawHex8(Result->digest().c_str());
   } else {
     response.PutCString("uuid:");
     response.PutCStringAsRawHex8(uuid_str.c_str());
   }