Convert FileSystem::Open() to return Expected<FileUP>

[lawrence_danna]

Summary

This patch converts FileSystem::Open from this prototype:

Status
Open(File &File, const FileSpec &file_spec, ...);

to this one:

llvm::Expected<std::unique_ptr<File>>
Open(const FileSpec &file_spec, ...);

This is beneficial on its own, as llvm::Expected is a more modern
and recommended error type than Status. It is also a necessary step
towards https://reviews.llvm.org/D67891, and further developments
for lldb_private::File.

[labath]

Thanks for splitting this up. Despite the number of comments, I think this is looking pretty good. The main theme of the comments is making sure that we satisfy the contract of the Expected class, which is a bit... unexpected. This is something that every new contributor gets burned by so don't worry about that. I've tried to annotate each place and suggest the possible way to handle it.

Additionally, I think it would be good to remove the ability to swap the file from underneath the stream if it is not too difficult.

lldb/include/lldb/Core/StreamFile.h

48–53

Could we remove this method? It would make things easier to reason about if we could disallow swapping of a File object backing a stream midway through its lifetime. Looking at the existing callers, it does not seem it should be hard to do that -- this is always called immediately after a stream is constructed via patterns like:

auto *stream = new StreamFile();
auto file = create_a_file();
if (file.is_ok()) {
  stream->SetFile(file);
  use(stream);
}

It should be easy to change that so that the stream is constructed only after we have a valid File object. It would also avoid the need to construct a fake File object just to guarantee it is always initialized.

lldb/source/API/SBStream.cpp

93

This is the trickiest thing about the Expected class. The idea behind it is it forces you to do something with the error. This means that this code would trigger an assertion if the opening of the file ever fails, as you don't "handle" the error in any way.
Unfortunately, here there is nothing we can do with the error (this is probably a deficiency of this API, but that's also another thing we can't change here), so we need to either explicitly throw it away (else consumeError(file.takeError())), or log it else LLDB_LOG_ERROR(GetLogIfAllCategoriesSet(LIBLLDB_LOG_API), file.takeError(), "Cannot open {1}: {0}", path)

120

why not assign this to the m_opaque_up (and avoid having an unowned pointer floating around)?

143

same here.

lldb/source/Commands/CommandObjectMemory.cpp

807–808

Include the error message here, which will also set the "checked" flag inside the expected object.

lldb/source/Core/StreamFile.cpp

40

We'll need to ignore/log the error here. Maybe leave a TODO to refactor this so that the caller is able to know whether the file was successfully created or not.

49

same here.

lldb/source/Expression/REPL.cpp

412

I guess the error can be written to error_sp here.

lldb/source/Host/common/FileCache.cpp

33

add: error = std::move(file.takeError());

lldb/source/Host/windows/Host.cpp

40

return errorToBool(imageBinaryUP.takeError())

lldb/source/Interpreter/CommandInterpreter.cpp

2326–2329

You could change this into something like result.AppendErrorWithFormatv("error: an error occurred read file '{0}': {1}\n", cmd_file_path, fmt_consume(file.takeError()))

lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp

2659

add the actual error to the error message, setting the checked flag

4595

add the actual error here

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

6279

this is good :)

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

107

ignore the error, I guess?

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp

958

ignore/log

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

913

add the error to the error message (you'll probably need to check each variable separately)

lldb/source/Target/Platform.cpp

1232

return Status(source_file.takeError())

lldb/unittests/Host/FileSystemTest.cpp

292–322

What's the point of having both of these tests? The error code should be the same no matter how you retrieve it from the expected object, so this is more of a unit test for the Expected class, than anything else...

lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

586

ASSERT_THAT_EXPECTED(file, llvm::Succeeded())

[lawrence_danna]

wow, I didn't realize the part about actually consuming the error. I thought the asserts only checked that you checked if there was an error. Uploading fixes momentarily

lldb/unittests/Host/FileSystemTest.cpp

292–322

GDBRemoteCommunicationServerCommon.cpp collects and errno value and sends it over the network. I wanted to confirm FileSystem::Open() was returning and Error value that could be correctly converted into errno.

[lawrence_danna]

Fixed according to reviewer comments.

[lawrence_danna]

got rid of StreamFile::SetFile

[lawrence_danna]

lldb/include/lldb/Core/StreamFile.h

48–53

Yea, it wasn't' too complicated to get rid of it.

[labath]

looks good to me, a extra couple of small comments inline. Thank you for taking your time to do this.

lldb/include/lldb/Core/StreamFile.h

38

assert file is not null?

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp

1042

This only has one caller, so it should be pretty simple to make it return the file as an actual return value (and it looks like it could be returning a unique_ptr).

lldb/unittests/Host/FileSystemTest.cpp

292–322

Yes, I got that part (and it's great that you've added it). But why test the same thing two ways? My point was that these two tests should always either both succeed, or both fail (any other result would be a bug in the Expected class).

BTW, if you wanted to be really fancy, you could write this as something like

EXPECT_THAT_EXPECTED(fs.Open(spec, File::eOpenOptionRead, 0, true), 
  llvm::Failed<ECError>(testing::Property(&ECError::convertToErrorCode, 
                                         std::error_code(ENOENT, std::system_category())));

[lawrence_danna]

fixed the remaining comments

[lawrence_danna]

done.

lldb/include/lldb/Core/StreamFile.h

public:
 
   StreamFile(const char *path);
 
   StreamFile(const char *path, uint32_t options,
              uint32_t permissions = lldb::eFilePermissionsFileDefault);
 
   StreamFile(FILE *fh, bool transfer_ownership);
 
+  StreamFile(std::shared_ptr<File> file) : StreamFile() { SetFile(file); }
+
   ~StreamFile() override;
 
-  File &GetFile() { return m_file; }
+  File &GetFile() { return *m_file_sp; }
+
+  const File &GetFile() const { return *m_file_sp; }
+
+  std::shared_ptr<File> GetFileSP() { return m_file_sp; }
 
-  const File &GetFile() const { return m_file; }
+  void SetFile(std::shared_ptr<File> file) {
+    if (file)
+      m_file_sp = file;
+    else
+      m_file_sp = std::make_shared<File>();
+  }
 
   void Flush() override;
 
 
 protected:
   // Classes that inherit from StreamFile can see and modify these
-  File m_file;
+  std::shared_ptr<File> m_file_sp; // never NULL
   size_t WriteImpl(const void *s, size_t length) override;
 
 private:
   DISALLOW_COPY_AND_ASSIGN(StreamFile);
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_StreamFile_h_

lldb/include/lldb/Host/FileCache.h

 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Status.h"
 
 namespace lldb_private {
 class FileCache {
 private:
   FileCache() {}
 
-  typedef std::map<lldb::user_id_t, lldb::FileSP> FDToFileMap;
+  typedef std::map<lldb::user_id_t, lldb::FileUP> FDToFileMap;
 
 public:
   static FileCache &GetInstance();
 
   lldb::user_id_t OpenFile(const FileSpec &file_spec, uint32_t flags,
                            uint32_t mode, Status &error);
   bool CloseFile(lldb::user_id_t fd, Status &error);
 

lldb/include/lldb/Host/FileSystem.h

public:
   Status ResolveSymbolicLink(const FileSpec &src, FileSpec &dst);
 
   /// 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,
-              bool should_close_fd = true);
+  llvm::Expected<std::unique_ptr<File>>
+  Open(const FileSpec &file_spec, uint32_t options,
 
   /// 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);
   /// \}

lldb/include/lldb/lldb-forward.h

 typedef std::unique_ptr<lldb_private::DynamicLoader> DynamicLoaderUP;
 typedef std::shared_ptr<lldb_private::Event> EventSP;
 typedef std::shared_ptr<lldb_private::EventData> EventDataSP;
 typedef std::shared_ptr<lldb_private::EventDataStructuredData>
     EventDataStructuredDataSP;
 typedef std::shared_ptr<lldb_private::ExecutionContextRef>
     ExecutionContextRefSP;
 typedef std::shared_ptr<lldb_private::ExpressionVariable> ExpressionVariableSP;
+typedef std::unique_ptr<lldb_private::File> FileUP;
 typedef std::shared_ptr<lldb_private::File> FileSP;
 typedef std::shared_ptr<lldb_private::Function> FunctionSP;
 typedef std::shared_ptr<lldb_private::FunctionCaller> FunctionCallerSP;
 typedef std::shared_ptr<lldb_private::FuncUnwinders> FuncUnwindersSP;
 typedef std::shared_ptr<lldb_private::InlineFunctionInfo> InlineFunctionInfoSP;
 typedef std::shared_ptr<lldb_private::Instruction> InstructionSP;
 typedef std::shared_ptr<lldb_private::InstrumentationRuntime>
     InstrumentationRuntimeSP;

lldb/scripts/Python/python-typemaps.swig

else if (!lldb_private::PythonFile::Check($input)) {
       } else {
          PyErr_SetString(PyExc_TypeError,"not a file-like object");
          return nullptr;
       }
    }
    else
    {
       PythonFile py_file(PyRefType::Borrowed, $input);
-      File file;
+      lldb::FileSP file;
       if (!py_file.GetUnderlyingFile(file))
          return nullptr;
 
-      $1 = file.GetStream();
+      $1 = file->GetStream();
       if ($1)
-         file.Clear();
+         file->Clear();
     }
 }
 
 %typemap(out) FILE * {
    char mode[4] = {0};
 %#ifdef __APPLE__
    int i = 0;
    if ($1)

lldb/source/API/SBStream.cpp

void SBStream::RedirectToFile(const char *path, bool append) {
 
   std::string local_data;
   if (m_opaque_up) {
     // See if we have any locally backed data. If so, copy it so we can then
     // redirect it to the file so we don't lose the data
     if (!m_is_file)
       local_data = static_cast<StreamString *>(m_opaque_up.get())->GetString();
   }
-  StreamFile *stream_file = new StreamFile;
   uint32_t open_options = File::eOpenOptionWrite | File::eOpenOptionCanCreate;
   if (append)
     open_options |= File::eOpenOptionAppend;
   else
     open_options |= File::eOpenOptionTruncate;
 
-  FileSystem::Instance().Open(stream_file->GetFile(), FileSpec(path),
-                              open_options);
-  m_opaque_up.reset(stream_file);
-
-  if (m_opaque_up) {
-    m_is_file = true;
+  llvm::Expected<FileUP> file =
+      FileSystem::Instance().Open(FileSpec(path), open_options);
+  if (file) {
+    StreamFile *newStream = new StreamFile(std::move(file.get()));
 
     // If we had any data locally in our StreamString, then pass that along to
     // the to new file we are redirecting to.
     if (!local_data.empty())
-      m_opaque_up->Write(&local_data[0], local_data.size());
-  } else
-    m_is_file = false;
+      newStream->Write(&local_data[0], local_data.size());
+
+    m_opaque_up.reset(newStream);
+    m_is_file = true;
+  }
 }
 
 void SBStream::RedirectToFileHandle(FILE *fh, bool transfer_fh_ownership) {
   LLDB_RECORD_METHOD(void, SBStream, RedirectToFileHandle, (FILE *, bool), fh,
                      transfer_fh_ownership);
 
   if (fh == nullptr)
     return;
 
   std::string local_data;
   if (m_opaque_up) {
     // See if we have any locally backed data. If so, copy it so we can then
     // redirect it to the file so we don't lose the data
     if (!m_is_file)
       local_data = static_cast<StreamString *>(m_opaque_up.get())->GetString();
   }
-  m_opaque_up.reset(new StreamFile(fh, transfer_fh_ownership));
-
-  if (m_opaque_up) {
-    m_is_file = true;
+  StreamFile *newStream = new StreamFile(fh, transfer_fh_ownership);
 
-    // If we had any data locally in our StreamString, then pass that along to
-    // the to new file we are redirecting to.
-    if (!local_data.empty())
-      m_opaque_up->Write(&local_data[0], local_data.size());
-  } else
-    m_is_file = false;
+    newStream->Write(&local_data[0], local_data.size());
+
+  m_opaque_up.reset(newStream);
+  m_is_file = true;
 }
 
 void SBStream::RedirectToFileDescriptor(int fd, bool transfer_fh_ownership) {
   LLDB_RECORD_METHOD(void, SBStream, RedirectToFileDescriptor, (int, bool), fd,
                      transfer_fh_ownership);
 
   std::string local_data;
   if (m_opaque_up) {
     // See if we have any locally backed data. If so, copy it so we can then
     // redirect it to the file so we don't lose the data
     if (!m_is_file)
       local_data = static_cast<StreamString *>(m_opaque_up.get())->GetString();
   }
 
-  m_opaque_up.reset(new StreamFile(::fdopen(fd, "w"), transfer_fh_ownership));
-  if (m_opaque_up) {
-    m_is_file = true;
+  StreamFile *newStream = new StreamFile(fd, transfer_fh_ownership);
 
-    // If we had any data locally in our StreamString, then pass that along to
-    // the to new file we are redirecting to.
-    if (!local_data.empty())
-      m_opaque_up->Write(&local_data[0], local_data.size());
-  } else
-    m_is_file = false;
+    newStream->Write(&local_data[0], local_data.size());
+
+  m_opaque_up.reset(newStream);
+  m_is_file = true;
 }
 
 lldb_private::Stream *SBStream::operator->() { return m_opaque_up.get(); }
 
 lldb_private::Stream *SBStream::get() { return m_opaque_up.get(); }
 
 lldb_private::Stream &SBStream::ref() {
   if (m_opaque_up == nullptr)

lldb/source/Commands/CommandObjectMemory.cpp

#undef ALL_KEYWORDS
     if (outfile_spec) {
 
       uint32_t open_options =
           File::eOpenOptionWrite | File::eOpenOptionCanCreate;
       const bool append = m_outfile_options.GetAppend().GetCurrentValue();
       if (append)
         open_options |= File::eOpenOptionAppend;
 
-      Status error = FileSystem::Instance().Open(outfile_stream.GetFile(),
-                                                 outfile_spec, open_options);
-      if (error.Success()) {
+      auto outfile = FileSystem::Instance().Open(outfile_spec, open_options);
+
+      if (outfile) {
+        outfile_stream.SetFile(std::move(outfile.get()));
         if (m_memory_options.m_output_as_binary) {
           const size_t bytes_written =
               outfile_stream.Write(data_sp->GetBytes(), bytes_read);
           if (bytes_written > 0) {
             result.GetOutputStream().Printf(
                 "%zi bytes %s to '%s'\n", bytes_written,
                 append ? "appended" : "written", path.c_str());
             return true;
           } else {
             result.AppendErrorWithFormat("Failed to write %" PRIu64
                                          " bytes to '%s'.\n",
                                          (uint64_t)bytes_read, path.c_str());
             result.SetStatus(eReturnStatusFailed);
             return false;
           }
         } else {
           // We are going to write ASCII to the file just point the
           // output_stream to our outfile_stream...
           output_stream = &outfile_stream;
         }
       } else {
         result.AppendErrorWithFormat("Failed to open file '%s' for %s.\n",
                                      path.c_str(), append ? "append" : "write");
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
     } else {
       output_stream = &result.GetOutputStream();
     }
 
     ExecutionContextScope *exe_scope = m_exe_ctx.GetBestExecutionContextScope();

lldb/source/Core/StreamFile.cpp

 #include "lldb/Host/FileSystem.h"
 
 #include <stdio.h>
 
 using namespace lldb;
 using namespace lldb_private;
 
 // StreamFile constructor
-StreamFile::StreamFile() : Stream(), m_file() {}
+StreamFile::StreamFile() : Stream() { m_file_sp = std::make_shared<File>(); }
 
 StreamFile::StreamFile(uint32_t flags, uint32_t addr_size, ByteOrder byte_order)
-    : Stream(flags, addr_size, byte_order), m_file() {}
+    : Stream(flags, addr_size, byte_order) {
+  m_file_sp = std::make_shared<File>();
+}
 
-StreamFile::StreamFile(int fd, bool transfer_ownership)
-    : Stream(), m_file(fd, File::eOpenOptionWrite, transfer_ownership) {}
+StreamFile::StreamFile(int fd, bool transfer_ownership) : Stream() {
+  m_file_sp =
+      std::make_shared<File>(fd, File::eOpenOptionWrite, transfer_ownership);
+}
 
-StreamFile::StreamFile(FILE *fh, bool transfer_ownership)
-    : Stream(), m_file(fh, transfer_ownership) {}
+StreamFile::StreamFile(FILE *fh, bool transfer_ownership) : Stream() {
+  m_file_sp = std::make_shared<File>(fh, transfer_ownership);
+}
 
-StreamFile::StreamFile(const char *path) : Stream(), m_file() {
-  FileSystem::Instance().Open(m_file, FileSpec(path),
-                              File::eOpenOptionWrite |
-                                  File::eOpenOptionCanCreate |
-                                  File::eOpenOptionCloseOnExec);
+StreamFile::StreamFile(const char *path) : Stream() {
+  auto file = FileSystem::Instance().Open(
+      FileSpec(path), File::eOpenOptionWrite | File::eOpenOptionCanCreate |
+  if (file)
+    m_file_sp = std::move(file.get());
+  else
+    m_file_sp = std::make_shared<File>();
 }
 
 StreamFile::StreamFile(const char *path, uint32_t options, uint32_t permissions)
-    : Stream(), m_file() {
-
-  FileSystem::Instance().Open(m_file, FileSpec(path), options, permissions);
+    : Stream() {
+  auto file = FileSystem::Instance().Open(FileSpec(path), options, permissions);
+  if (file)
+    m_file_sp = std::move(file.get());
+  else
+    m_file_sp = std::make_shared<File>();
 }
 
 StreamFile::~StreamFile() {}
 
-void StreamFile::Flush() { m_file.Flush(); }
+void StreamFile::Flush() { m_file_sp->Flush(); }
 
 size_t StreamFile::WriteImpl(const void *s, size_t length) {
-  m_file.Write(s, length);
+  m_file_sp->Write(s, length);
   return length;
 }

lldb/source/Expression/REPL.cpp

if (code[0] == ':') {
 
         if (add_to_code) {
           const uint32_t new_default_line = m_code.GetSize() + 1;
 
           m_code.SplitIntoLines(code);
 
           // Update our code on disk
           if (!m_repl_source_path.empty()) {
-            lldb_private::File file;
-            FileSystem::Instance().Open(file, FileSpec(m_repl_source_path),
+            auto file = FileSystem::Instance().Open(FileSpec(m_repl_source_path),
                                         File::eOpenOptionWrite |
                                             File::eOpenOptionTruncate |
                                             File::eOpenOptionCanCreate,
                                         lldb::eFilePermissionsFileDefault);
+            if (file) {
               std::string code(m_code.CopyList());
               code.append(1, '\n');
               size_t bytes_written = code.size();
-            file.Write(code.c_str(), bytes_written);
-            file.Close();
+              file.get()->Write(code.c_str(), bytes_written);
+              file.get()->Close();
+            }
 
             // Now set the default file and line to the REPL source file
             m_target.GetSourceManager().SetDefaultFileAndLine(
                 FileSpec(m_repl_source_path), new_default_line);
           }
           static_cast<IOHandlerEditline &>(io_handler)
               .SetBaseLineNumber(m_code.GetSize() + 1);
         }

lldb/source/Host/common/FileCache.cpp

 }
 
 lldb::user_id_t FileCache::OpenFile(const FileSpec &file_spec, uint32_t flags,
                                     uint32_t mode, Status &error) {
   if (!file_spec) {
     error.SetErrorString("empty path");
     return UINT64_MAX;
   }
-  FileSP file_sp(new File());
-  error = FileSystem::Instance().Open(*file_sp, file_spec, flags, mode);
-  if (!file_sp->IsValid())
+  auto file = FileSystem::Instance().Open(file_spec, flags, mode);
+  if (!file)
     return UINT64_MAX;
-  lldb::user_id_t fd = file_sp->GetDescriptor();
-  m_cache[fd] = file_sp;
+  lldb::user_id_t fd = file.get()->GetDescriptor();
+  m_cache[fd] = std::move(file.get());
   return fd;
 }
 
 bool FileCache::CloseFile(lldb::user_id_t fd, Status &error) {
   if (fd == UINT64_MAX) {
     error.SetErrorString("invalid file descriptor");
     return false;
   }
   FDToFileMap::iterator pos = m_cache.find(fd);
   if (pos == m_cache.end()) {
     error.SetErrorStringWithFormat("invalid host file descriptor %" PRIu64, fd);
     return false;
   }
-  FileSP file_sp = pos->second;
-  if (!file_sp) {
+  FileUP &file_up = pos->second;
+  if (!file_up) {
     error.SetErrorString("invalid host backing file");
     return false;
   }
-  error = file_sp->Close();
+  error = file_up->Close();
   m_cache.erase(pos);
   return error.Success();
 }
 
 uint64_t FileCache::WriteFile(lldb::user_id_t fd, uint64_t offset,
                               const void *src, uint64_t src_len,
                               Status &error) {
   if (fd == UINT64_MAX) {
     error.SetErrorString("invalid file descriptor");
     return UINT64_MAX;
   }
   FDToFileMap::iterator pos = m_cache.find(fd);
   if (pos == m_cache.end()) {
     error.SetErrorStringWithFormat("invalid host file descriptor %" PRIu64, fd);
     return false;
   }
-  FileSP file_sp = pos->second;
-  if (!file_sp) {
+  FileUP &file_up = pos->second;
+  if (!file_up) {
     error.SetErrorString("invalid host backing file");
     return UINT64_MAX;
   }
-  if (static_cast<uint64_t>(file_sp->SeekFromStart(offset, &error)) != offset ||
+  if (static_cast<uint64_t>(file_up->SeekFromStart(offset, &error)) != offset ||
       error.Fail())
     return UINT64_MAX;
   size_t bytes_written = src_len;
-  error = file_sp->Write(src, bytes_written);
+  error = file_up->Write(src, bytes_written);
   if (error.Fail())
     return UINT64_MAX;
   return bytes_written;
 }
 
 uint64_t FileCache::ReadFile(lldb::user_id_t fd, uint64_t offset, void *dst,
                              uint64_t dst_len, Status &error) {
   if (fd == UINT64_MAX) {
     error.SetErrorString("invalid file descriptor");
     return UINT64_MAX;
   }
   FDToFileMap::iterator pos = m_cache.find(fd);
   if (pos == m_cache.end()) {
     error.SetErrorStringWithFormat("invalid host file descriptor %" PRIu64, fd);
     return false;
   }
-  FileSP file_sp = pos->second;
-  if (!file_sp) {
+  FileUP &file_up = pos->second;
+  if (!file_up) {
     error.SetErrorString("invalid host backing file");
     return UINT64_MAX;
   }
-  if (static_cast<uint64_t>(file_sp->SeekFromStart(offset, &error)) != offset ||
+  if (static_cast<uint64_t>(file_up->SeekFromStart(offset, &error)) != offset ||
       error.Fail())
     return UINT64_MAX;
   size_t bytes_read = dst_len;
-  error = file_sp->Read(dst, bytes_read);
+  error = file_up->Read(dst, bytes_read);
   if (error.Fail())
     return UINT64_MAX;
   return bytes_read;
 }

lldb/source/Host/common/FileSystem.cpp

if (permissions & lldb::eFilePermissionsWorldRead)
     mode |= S_IROTH;
   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,
+Expected<FileUP> FileSystem::Open(const FileSpec &file_spec, uint32_t options,
                                   uint32_t permissions, bool should_close_fd) {
   if (m_collector)
     m_collector->addFile(file_spec.GetPath());
 
-  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());
+    return errorCodeToError(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(-1, options, false);
-    error.SetErrorToErrno();
-  } else {
-    File.SetDescriptor(descriptor, options, should_close_fd);
-  }
-  return error;
+  if (!File::DescriptorIsValid(descriptor))
+    return llvm::errorCodeToError(
+        std::error_code(errno, std::system_category()));
+
+  auto file = std::make_unique<File>(descriptor, options, should_close_fd);
+  assert(file->IsValid());
+  return file;
 }
 
 ErrorOr<std::string> FileSystem::GetExternalPath(const llvm::Twine &path) {
   if (!m_mapped)
     return path.str();
 
   // If VFS mapped we know the underlying FS is a RedirectingFileSystem.
   ErrorOr<vfs::RedirectingFileSystem::Entry *> E =

lldb/source/Host/windows/Host.cpp

 
 using namespace lldb;
 using namespace lldb_private;
 
 namespace {
 bool GetTripleForProcess(const FileSpec &executable, llvm::Triple &triple) {
   // Open the PE File as a binary file, and parse just enough information to
   // determine the machine type.
-  File imageBinary;
-  FileSystem::Instance().Open(imageBinary, executable, File::eOpenOptionRead,
-                              lldb::eFilePermissionsUserRead);
+  auto imageBinaryP = FileSystem::Instance().Open(
+      executable, File::eOpenOptionRead, lldb::eFilePermissionsUserRead);
+  if (!imageBinaryP)
+    return false;
+  File &imageBinary = *imageBinaryP.get();
   imageBinary.SeekFromStart(0x3c);
   int32_t peOffset = 0;
   uint32_t peHead = 0;
   uint16_t machineType = 0;
   size_t readSize = sizeof(peOffset);
   imageBinary.Read(&peOffset, readSize);
   imageBinary.SeekFromStart(peOffset);
   imageBinary.Read(&peHead, readSize);

lldb/source/Interpreter/CommandInterpreter.cpp

result.AppendErrorWithFormat(
         "Error reading commands from file %s - file not found.\n",
         cmd_file.GetFilename().AsCString("<Unknown>"));
     result.SetStatus(eReturnStatusFailed);
     return;
   }
 
   StreamFileSP input_file_sp(new StreamFile());
   std::string cmd_file_path = cmd_file.GetPath();
-  Status error = FileSystem::Instance().Open(input_file_sp->GetFile(), cmd_file,
-                                             File::eOpenOptionRead);
 
-  if (error.Fail()) {
+  auto file = FileSystem::Instance().Open(cmd_file, File::eOpenOptionRead);
+
+  if (file) {
+    input_file_sp->SetFile(std::move(file.get()));
+  } else {
+    std::string error = llvm::toString(file.takeError());
     result.AppendErrorWithFormat(
         "error: an error occurred read file '%s': %s\n", cmd_file_path.c_str(),
-        error.AsCString());
+        error.c_str());
     result.SetStatus(eReturnStatusFailed);
     return;
   }
 
   Debugger &debugger = GetDebugger();
 
   uint32_t flags = 0;
 

lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp

assert(alloc->data_ptr.isValid() && alloc->element.type.isValid() &&
          alloc->element.type_vec_size.isValid() &&
          alloc->element.datum_size.get() &&
          alloc->element.type_kind.isValid() && alloc->dimension.isValid() &&
          "Allocation information not available");
 
   // Check we can create writable file
   FileSpec file_spec(path);
   FileSystem::Instance().Resolve(file_spec);
-  File file;
-  FileSystem::Instance().Open(file, file_spec,
-                              File::eOpenOptionWrite |
-                                  File::eOpenOptionCanCreate |
-                                  File::eOpenOptionTruncate);
+  auto file = FileSystem::Instance().Open(
+      file_spec, File::eOpenOptionWrite | File::eOpenOptionCanCreate |
 
   if (!file) {
     strm.Printf("Error: Failed to open '%s' for writing", path);
     strm.EOL();
     return false;
   }
 
   // Read allocation into buffer of heap memory
   const std::shared_ptr<uint8_t> buffer = GetAllocationData(alloc, frame_ptr);
   if (!buffer) {
     strm.Printf("Error: Couldn't read allocation data into buffer");
bool RenderScriptRuntime::SaveAllocation(Stream &strm, const uint32_t alloc_id,
   head.hdr_size = static_cast<uint16_t>(sizeof(AllocationDetails::FileHeader) +
                                         element_header_size);
 
   // Write the file header
   size_t num_bytes = sizeof(AllocationDetails::FileHeader);
   LLDB_LOGF(log, "%s - writing File Header, 0x%" PRIx64 " bytes", __FUNCTION__,
             (uint64_t)num_bytes);
 
-  Status err = file.Write(&head, num_bytes);
+  Status err = file.get()->Write(&head, num_bytes);
   if (!err.Success()) {
     strm.Printf("Error: '%s' when writing to file '%s'", err.AsCString(), path);
     strm.EOL();
     return false;
   }
 
   // Create the headers describing the element type of the allocation.
   std::shared_ptr<uint8_t> element_header_buffer(
       new uint8_t[element_header_size]);
   if (element_header_buffer == nullptr) {
     strm.Printf("Internal Error: Couldn't allocate %" PRIu64
                 " bytes on the heap",
                 (uint64_t)element_header_size);
     strm.EOL();
     return false;
   }
 
   PopulateElementHeaders(element_header_buffer, 0, alloc->element);
 
   // Write headers for allocation element type to file
   num_bytes = element_header_size;
   LLDB_LOGF(log, "%s - writing element headers, 0x%" PRIx64 " bytes.",
             __FUNCTION__, (uint64_t)num_bytes);
 
-  err = file.Write(element_header_buffer.get(), num_bytes);
+  err = file.get()->Write(element_header_buffer.get(), num_bytes);
   if (!err.Success()) {
     strm.Printf("Error: '%s' when writing to file '%s'", err.AsCString(), path);
     strm.EOL();
     return false;
   }
 
   // Write allocation data to file
   num_bytes = static_cast<size_t>(*alloc->size.get());
   LLDB_LOGF(log, "%s - writing 0x%" PRIx64 " bytes", __FUNCTION__,
             (uint64_t)num_bytes);
 
-  err = file.Write(buffer.get(), num_bytes);
+  err = file.get()->Write(buffer.get(), num_bytes);
   if (!err.Success()) {
     strm.Printf("Error: '%s' when writing to file '%s'", err.AsCString(), path);
     strm.EOL();
     return false;
   }
 
   strm.Printf("Allocation written to file '%s'", path);
   strm.EOL();
bool DoExecute(Args &command, CommandReturnObject &result) override {
 
     Stream *output_strm = nullptr;
     StreamFile outfile_stream;
     const FileSpec &outfile_spec =
         m_options.m_outfile; // Dump allocation to file instead
     if (outfile_spec) {
       // Open output file
       std::string path = outfile_spec.GetPath();
-      auto error = FileSystem::Instance().Open(
-          outfile_stream.GetFile(), outfile_spec,
-          File::eOpenOptionWrite | File::eOpenOptionCanCreate);
-      if (error.Success()) {
+      auto file = FileSystem::Instance().Open(
+          outfile_spec, File::eOpenOptionWrite | File::eOpenOptionCanCreate);
+      if (file) {
+        outfile_stream.SetFile(std::move(file.get()));
         output_strm = &outfile_stream;
         result.GetOutputStream().Printf("Results written to '%s'",
                                         path.c_str());
         result.GetOutputStream().EOL();
       } else {
         result.AppendErrorWithFormat("Couldn't open file '%s'", path.c_str());
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
     } else
       output_strm = &result.GetOutputStream();
 
     assert(output_strm != nullptr);
     bool dumped =

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

if (target_triple.getVendor() == llvm::Triple::Apple &&
               buffer.PutHex32(static_cast<uint32_t>(segment.filesize));
             }
             buffer.PutHex32(segment.maxprot);
             buffer.PutHex32(segment.initprot);
             buffer.PutHex32(segment.nsects);
             buffer.PutHex32(segment.flags);
           }
 
-          File core_file;
           std::string core_file_path(outfile.GetPath());
-          error = FileSystem::Instance().Open(core_file, outfile,
-                                              File::eOpenOptionWrite |
-                                                  File::eOpenOptionTruncate |
-                                                  File::eOpenOptionCanCreate);
-          if (error.Success()) {
+          auto core_file = FileSystem::Instance().Open(
+              outfile, File::eOpenOptionWrite | File::eOpenOptionTruncate |
+          if (!core_file) {
+            error = core_file.takeError();
+          } else {
             // Read 1 page at a time
             uint8_t bytes[0x1000];
             // Write the mach header and load commands out to the core file
             size_t bytes_written = buffer.GetString().size();
-            error = core_file.Write(buffer.GetString().data(), bytes_written);
+            error = core_file.get()->Write(buffer.GetString().data(),
+                                           bytes_written);
             if (error.Success()) {
               // Now write the file data for all memory segments in the process
               for (const auto &segment : segment_load_commands) {
-                if (core_file.SeekFromStart(segment.fileoff) == -1) {
+                if (core_file.get()->SeekFromStart(segment.fileoff) == -1) {
                   error.SetErrorStringWithFormat(
                       "unable to seek to offset 0x%" PRIx64 " in '%s'",
                       segment.fileoff, core_file_path.c_str());
                   break;
                 }
 
                 printf("Saving %" PRId64
                        " bytes of data for memory region at 0x%" PRIx64 "\n",
                        segment.vmsize, segment.vmaddr);
                 addr_t bytes_left = segment.vmsize;
                 addr_t addr = segment.vmaddr;
                 Status memory_read_error;
                 while (bytes_left > 0 && error.Success()) {
                   const size_t bytes_to_read =
                       bytes_left > sizeof(bytes) ? sizeof(bytes) : bytes_left;
 
                   // In a savecore setting, we don't really care about caching,
                   // as the data is dumped and very likely never read again,
                   // so we call ReadMemoryFromInferior to bypass it.
                   const size_t bytes_read = process_sp->ReadMemoryFromInferior(
                       addr, bytes, bytes_to_read, memory_read_error);
 
                   if (bytes_read == bytes_to_read) {
                     size_t bytes_written = bytes_read;
-                    error = core_file.Write(bytes, bytes_written);
+                    error = core_file.get()->Write(bytes, bytes_written);
                     bytes_left -= bytes_read;
                     addr += bytes_read;
                   } else {
                     // Some pages within regions are not readable, those should
                     // be zero filled
                     memset(bytes, 0, bytes_to_read);
                     size_t bytes_written = bytes_to_read;
-                    error = core_file.Write(bytes, bytes_written);
+                    error = core_file.get()->Write(bytes, bytes_written);
                     bytes_left -= bytes_to_read;
                     addr += bytes_to_read;
                   }
                 }
               }
             }
           }
         } else {

lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm

bool CoreSimulatorSupport::Device::Shutdown(Status &err) {
   } else {
     err.SetErrorString([[nserror description] UTF8String]);
     return false;
   }
 }
 
 static Status HandleFileAction(ProcessLaunchInfo &launch_info,
                                NSMutableDictionary *options, NSString *key,
-                               const int fd, File &file) {
+                               const int fd, lldb::FileSP &file) {
   Status error;
   const FileAction *file_action = launch_info.GetFileActionForFD(fd);
   if (file_action) {
     switch (file_action->GetAction()) {
     case FileAction::eFileActionNone:
       break;
 
     case FileAction::eFileActionClose:
case FileAction::eFileActionOpen: {
         int created_fd =
             open(file_spec.GetPath().c_str(), oflag, S_IRUSR | S_IWUSR);
         if (created_fd >= 0) {
           uint32_t file_options = 0;
           if ((oflag & O_RDWR) || (oflag & O_RDONLY))
             file_options |= File::eOpenOptionRead;
           if ((oflag & O_RDWR) || (oflag & O_RDONLY))
             file_options |= File::eOpenOptionWrite;
-          file.SetDescriptor(created_fd, file_options, true);
+          file = std::make_shared<File>(created_fd, file_options, true);
           [options setValue:[NSNumber numberWithInteger:created_fd] forKey:key];
           return error; // Success
         } else {
           posix_error.SetErrorToErrno();
           error.SetErrorStringWithFormat("unable to open file '%s': %s",
                                          file_spec.GetPath().c_str(),
                                          posix_error.AsCString());
         }
for (const auto &KV : launch_info.GetEnvironment()) {
     NSString *value_ns = [NSString stringWithUTF8String:KV.second.c_str()];
 
     [env_dict setValue:value_ns forKey:key_ns];
   }
 
   [options setObject:env_dict forKey:kSimDeviceSpawnEnvironment];
 
   Status error;
-  File stdin_file;
-  File stdout_file;
-  File stderr_file;
+  lldb::FileSP stdin_file;
+  lldb::FileSP stdout_file;
+  lldb::FileSP stderr_file;
   error = HandleFileAction(launch_info, options, kSimDeviceSpawnStdin,
                            STDIN_FILENO, stdin_file);
 
   if (error.Fail())
     return CoreSimulatorSupport::Process(error);
 
   error = HandleFileAction(launch_info, options, kSimDeviceSpawnStdout,
                            STDOUT_FILENO, stdout_file);

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

GDBRemoteCommunicationServerCommon::Handle_vFile_Open(
   packet.GetHexByteStringTerminatedBy(path, ',');
   if (!path.empty()) {
     if (packet.GetChar() == ',') {
       uint32_t flags = packet.GetHexMaxU32(false, 0);
       if (packet.GetChar() == ',') {
         mode_t mode = packet.GetHexMaxU32(false, 0600);
         FileSpec path_spec(path);
         FileSystem::Instance().Resolve(path_spec);
-        File file;
         // Do not close fd.
-        Status error =
-            FileSystem::Instance().Open(file, path_spec, flags, mode, false);
-        const int save_errno = error.GetError();
+        auto file = FileSystem::Instance().Open(path_spec, flags, mode, false);
+
+        int save_errno = 0;
+        int descriptor = File::kInvalidDescriptor;
+        if (file) {
+          descriptor = file.get()->GetDescriptor();
+        } else {
+          std::error_code code = errorToErrorCode(file.takeError());
+          if (code.category() == std::system_category()) {
+            save_errno = code.value();
+          }
+        }
+
         StreamString response;
         response.PutChar('F');
-        response.Printf("%i", file.GetDescriptor());
+        response.Printf("%i", descriptor);
         if (save_errno)
           response.Printf(",%i", save_errno);
         return SendPacketNoLock(response.GetString());
       }
     }
   }
   return SendErrorResponse(18);
 }

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

 namespace lldb {
 // Provide a function that can easily dump the packet history if we know a
 // ProcessGDBRemote * value (which we can get from logs or from debugging). We
 // need the function in the lldb namespace so it makes it into the final
 // executable since the LLDB shared library only exports stuff in the lldb
 // namespace. This allows you to attach with a debugger and call this function
 // and get the packet history dumped to a file.
 void DumpProcessGDBRemotePacketHistory(void *p, const char *path) {
-  StreamFile strm;
-  Status error = FileSystem::Instance().Open(strm.GetFile(), FileSpec(path),
-                                             File::eOpenOptionWrite |
-                                                 File::eOpenOptionCanCreate);
-  if (error.Success())
-    ((ProcessGDBRemote *)p)->GetGDBRemote().DumpHistory(strm);
+  auto file = FileSystem::Instance().Open(
+      FileSpec(path), File::eOpenOptionWrite | File::eOpenOptionCanCreate);
+  if (file) {
+    StreamFile stream(std::move(file.get()));
+    ((ProcessGDBRemote *)p)->GetGDBRemote().DumpHistory(stream);
+  }
 }
 } // namespace lldb
 
 namespace {
 
 #define LLDB_PROPERTIES_processgdbremote
 #include "ProcessGDBRemoteProperties.inc"
 

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h

public:
 
   using PythonObject::Reset;
 
   void Reset(PyRefType type, PyObject *py_obj) override;
   void Reset(File &file, const char *mode);
 
   static uint32_t GetOptionsFromMode(llvm::StringRef mode);
 
-  bool GetUnderlyingFile(File &file) const;
+  bool GetUnderlyingFile(lldb::FileSP &file) const;
 };
 
 } // namespace lldb_private
 
 #endif
 
 #endif // LLDB_PLUGINS_SCRIPTINTERPRETER_PYTHON_PYTHONDATAOBJECTS_H

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp

return PythonObject(PyRefType::Owned,
                       PyObject_CallObject(m_py_obj, arg_tuple.get()));
 }
 
 PythonFile::PythonFile() : PythonObject() {}
 
 PythonFile::PythonFile(File &file, const char *mode) { Reset(file, mode); }
 
 PythonFile::PythonFile(const char *path, const char *mode) {
-  lldb_private::File file;
-  FileSystem::Instance().Open(file, FileSpec(path), GetOptionsFromMode(mode));
-  Reset(file, mode);
+  auto file =
+      FileSystem::Instance().Open(FileSpec(path), GetOptionsFromMode(mode));
+  if (file)
+    Reset(*file.get(), mode);
+  else
+    Reset();
 }
 
 PythonFile::PythonFile(PyRefType type, PyObject *o) { Reset(type, o); }
 
 PythonFile::~PythonFile() {}
 
 bool PythonFile::Check(PyObject *py_obj) {
 #if PY_MAJOR_VERSION < 3
return llvm::StringSwitch<uint32_t>(mode.str())
       .Case("r+", File::eOpenOptionRead | File::eOpenOptionWrite)
       .Case("w+", File::eOpenOptionRead | File::eOpenOptionWrite |
                       File::eOpenOptionCanCreate | File::eOpenOptionTruncate)
       .Case("a+", File::eOpenOptionRead | File::eOpenOptionWrite |
                       File::eOpenOptionAppend | File::eOpenOptionCanCreate)
       .Default(0);
 }
 
-bool PythonFile::GetUnderlyingFile(File &file) const {
+bool PythonFile::GetUnderlyingFile(FileSP &file) const {
   if (!IsValid())
     return false;
 
-  file.Close();
+  if (file)
+    file->Close();
+
   // We don't own the file descriptor returned by this function, make sure the
   // File object knows about that.
   PythonString py_mode = GetAttributeValue("mode").AsType<PythonString>();
   auto options = PythonFile::GetOptionsFromMode(py_mode.GetString());
-  file.SetDescriptor(PyObject_AsFileDescriptor(m_py_obj), options, false);
-  return file.IsValid();
+  file = std::make_shared<File>(PyObject_AsFileDescriptor(m_py_obj), options,
+                                false);
+  return file->IsValid();
 }
 
 #endif

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

#endif
                 debugger.GetErrorFile()->GetFile().GetStream());
           }
         }
       }
       if (!input_file_sp || !output_file_sp || !error_file_sp)
         debugger.AdoptTopIOHandlerFilesIfInvalid(input_file_sp, output_file_sp,
                                                  error_file_sp);
     } else {
-      input_file_sp = std::make_shared<StreamFile>();
-      FileSystem::Instance().Open(input_file_sp->GetFile(),
+      auto nullin = FileSystem::Instance().Open(
                                   FileSpec(FileSystem::DEV_NULL),
                                   File::eOpenOptionRead);
-
-      output_file_sp = std::make_shared<StreamFile>();
-      FileSystem::Instance().Open(output_file_sp->GetFile(),
+      auto nullout = FileSystem::Instance().Open(
                                   FileSpec(FileSystem::DEV_NULL),
                                   File::eOpenOptionWrite);
-
-      error_file_sp = output_file_sp;
+      if (!nullin || !nullout) {
+        result->AppendError("failed to open /dev/null!\n");
+        return false;
+      }
+      input_file_sp = std::make_shared<StreamFile>(std::move(nullin.get()));
+      error_file_sp = output_file_sp = std::make_shared<StreamFile>(std::move(nullout.get()));
     }
 
     FILE *in_file = input_file_sp->GetFile().GetStream();
     FILE *out_file = output_file_sp->GetFile().GetStream();
     FILE *err_file = error_file_sp->GetFile().GetStream();
     bool success = false;
     {
       // WARNING!  It's imperative that this RAII scope be as tight as

lldb/source/Target/ModuleCache.cpp

if ((result[i] >= 1 && result[i] <= 31) ||
         strchr(kFSIllegalChars, result[i]) != nullptr)
       result[i] = '_';
   }
   return result;
 }
 
 class ModuleLock {
 private:
-  File m_file;
+  FileUP m_file_up;
   std::unique_ptr<lldb_private::LockFile> m_lock;
   FileSpec m_file_spec;
 
 public:
   ModuleLock(const FileSpec &root_dir_spec, const UUID &uuid, Status &error);
   void Delete();
 };
 

 ModuleLock::ModuleLock(const FileSpec &root_dir_spec, const UUID &uuid,
                        Status &error) {
   const auto lock_dir_spec = JoinPath(root_dir_spec, kLockDirName);
   error = MakeDirectory(lock_dir_spec);
   if (error.Fail())
     return;
 
   m_file_spec = JoinPath(lock_dir_spec, uuid.GetAsString().c_str());
-  FileSystem::Instance().Open(m_file, m_file_spec,
-                              File::eOpenOptionWrite |
-                                  File::eOpenOptionCanCreate |
-                                  File::eOpenOptionCloseOnExec);
-  if (!m_file) {
-    error.SetErrorToErrno();
+
+  auto file = FileSystem::Instance().Open(
+      m_file_spec, File::eOpenOptionWrite | File::eOpenOptionCanCreate |
+  if (file)
+    m_file_up = std::move(file.get());
+  else {
+    m_file_up.reset();
+    error = Status(file.takeError());
     return;
   }
 
-  m_lock.reset(new lldb_private::LockFile(m_file.GetDescriptor()));
+  m_lock.reset(new lldb_private::LockFile(m_file_up->GetDescriptor()));
   error = m_lock->WriteLock(0, 1);
   if (error.Fail())
     error.SetErrorStringWithFormat("Failed to lock file: %s",
                                    error.AsCString());
 }
 
 void ModuleLock::Delete() {
-  if (!m_file)
+  if (!m_file_up)
     return;
 
-  m_file.Close();
+  m_file_up->Close();
+  m_file_up.reset();
   llvm::sys::fs::remove(m_file_spec.GetPath());
 }
 
 /////////////////////////////////////////////////////////////////////////
 
 Status ModuleCache::Put(const FileSpec &root_dir_spec, const char *hostname,
                         const ModuleSpec &module_spec, const FileSpec &tmp_file,
                         const FileSpec &target_file) {

lldb/source/Target/Platform.cpp

Status Platform::PutFile(const FileSpec &source, const FileSpec &destination,
   LLDB_LOGF(log, "[PutFile] Using block by block transfer....\n");
 
   uint32_t source_open_options =
       File::eOpenOptionRead | File::eOpenOptionCloseOnExec;
   namespace fs = llvm::sys::fs;
   if (fs::is_symlink_file(source.GetPath()))
     source_open_options |= File::eOpenOptionDontFollowSymlinks;
 
-  File source_file;
-  Status error = FileSystem::Instance().Open(
-      source_file, source, source_open_options, lldb::eFilePermissionsUserRW);
-  uint32_t permissions = source_file.GetPermissions(error);
+  auto source_file = FileSystem::Instance().Open(
+      source, source_open_options, lldb::eFilePermissionsUserRW);
+  if (!source_file)
+    return Status("PutFile: unable to open source file");
+  Status error;
+  uint32_t permissions = source_file.get()->GetPermissions(error);
   if (permissions == 0)
     permissions = lldb::eFilePermissionsFileDefault;
 
-  if (!source_file.IsValid())
-    return Status("PutFile: unable to open source file");
   lldb::user_id_t dest_file = OpenFile(
       destination, File::eOpenOptionCanCreate | File::eOpenOptionWrite |
                        File::eOpenOptionTruncate | File::eOpenOptionCloseOnExec,
       permissions, error);
   LLDB_LOGF(log, "dest_file = %" PRIu64 "\n", dest_file);
 
   if (error.Fail())
     return error;
   if (dest_file == UINT64_MAX)
     return Status("unable to open target file");
   lldb::DataBufferSP buffer_sp(new DataBufferHeap(1024 * 16, 0));
   uint64_t offset = 0;
   for (;;) {
     size_t bytes_read = buffer_sp->GetByteSize();
-    error = source_file.Read(buffer_sp->GetBytes(), bytes_read);
+    error = source_file.get()->Read(buffer_sp->GetBytes(), bytes_read);
     if (error.Fail() || bytes_read == 0)
       break;
 
     const uint64_t bytes_written =
         WriteFile(dest_file, offset, buffer_sp->GetBytes(), bytes_read, error);
     if (error.Fail())
       break;
 
     offset += bytes_written;
     if (bytes_written != bytes_read) {
       // We didn't write the correct number of bytes, so adjust the file
       // position in the source file we are reading from...
-      source_file.SeekFromStart(offset);
+      source_file.get()->SeekFromStart(offset);
     }
   }
   CloseFile(dest_file, error);
 
   if (uid == UINT32_MAX && gid == UINT32_MAX)
     return error;
 
   // TODO: ChownFile?

lldb/unittests/Host/FileSystemTest.cpp

TEST(FileSystemTest, EnumerateDirectory) {
   constexpr bool find_other = true;
 
   fs.EnumerateDirectory("/", find_directories, find_files, find_other,
                         VFSCallback, &visited);
 
   EXPECT_THAT(visited,
               testing::UnorderedElementsAre("/foo", "/bar", "/baz", "/qux"));
 }
+
+TEST(FileSystemTest, OpenErrno) {
+#ifdef _WIN32
+  FileSpec spec("C:\\FILE\\THAT\\DOES\\NOT\\EXIST.TXT");
+#else
+  FileSpec spec("/file/that/does/not/exist.txt");
+#endif
+  FileSystem fs;
+  auto file = fs.Open(spec, File::eOpenOptionRead, 0, true);
+  ASSERT_FALSE(file);
+  std::error_code code = errorToErrorCode(file.takeError());
+  EXPECT_EQ(code.category(), std::system_category());
+  EXPECT_EQ(code.value(), ENOENT);
+}
+
+TEST(FileSystemTest, OpenErrnoHandler) {
+#ifdef _WIN32
+  FileSpec spec("C:\\FILE\\THAT\\DOES\\NOT\\EXIST.TXT");
+#else
+  FileSpec spec("/file/that/does/not/exist.txt");
+#endif
+  FileSystem fs;
+  auto file = fs.Open(spec, File::eOpenOptionRead, 0, true);
+  ASSERT_FALSE(file);
+  std::error_code code;
+  Error error = handleErrors(file.takeError(), [&](const ECError &E) {
+    code = E.convertToErrorCode();
+  });
+  ASSERT_FALSE(error);
+  EXPECT_EQ(code.category(), std::system_category());
+  EXPECT_EQ(code.value(), ENOENT);
+}

lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

TEST_F(PythonDataObjectsTest, TestPythonCallableInvoke) {
   auto list_result = result.AsType<PythonList>();
   EXPECT_EQ(3U, list_result.GetSize());
   EXPECT_EQ(one.get(), list_result.GetItemAtIndex(0).get());
   EXPECT_EQ(two.get(), list_result.GetItemAtIndex(1).get());
   EXPECT_EQ(three.get(), list_result.GetItemAtIndex(2).get());
 }
 
 TEST_F(PythonDataObjectsTest, TestPythonFile) {
-  File file;
-  FileSystem::Instance().Open(file, FileSpec(FileSystem::DEV_NULL),
+  auto file = FileSystem::Instance().Open(FileSpec(FileSystem::DEV_NULL),
                                           File::eOpenOptionRead);
-  PythonFile py_file(file, "r");
+  ASSERT_TRUE(!!file);
+  PythonFile py_file(*file.get(), "r");
   EXPECT_TRUE(PythonFile::Check(py_file.get()));
 }
 
 TEST_F(PythonDataObjectsTest, TestObjectAttributes) {
   PythonInteger py_int(42);
   EXPECT_TRUE(py_int.HasAttribute("numerator"));
   EXPECT_FALSE(py_int.HasAttribute("this_should_not_exist"));