SBFile::GetFile: convert SBFile back into python native files.

[lawrence_danna]

Summary

This makes SBFile::GetFile public and adds a SWIG typemap to convert
the result back into a python native file.

If the underlying File itself came from a python file, it is returned
identically. Otherwise a new python file object is created using
the file descriptor.

[labath]

It looks pretty straight-forward. The two main comments I have are:

• implement rtti to get rid of the GetPythonObject method on the base class
• it looks like the implementations of new apis you're adding (GetOptions, mainly) have to go out of their way to ignore errors, only for their callers to synthesize new Error objects. It seems like it would be better to just use Expecteds throughout.

lldb/include/lldb/Host/File.h

65

change the argument type to OpenOptions. For some reason, lldb has historically used integer types for passing enumerations around, but we're now slowly changing that..

326

We've spend a lot of time removing python from the lowest layers, and this let's it back in through the back door. It would be way better to add support for llvm-style rtti to this class, so that the python code can dyn_cast to a PythonFile when it needs to (re)construct the python object. You can take a look at CPPLanguageRuntime::classof (and friend) for how to implement this in a plugin-friendly manner.

338

How about returning Expected<OpenOptions> from here?

340

and Expected<const char*> from here?

lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py

775

So, if I understand correctly, these tests check that you can feed a file opened by python into lldb, and then pump it back out to python. Are there any tests which do the opposite (have lldb open a file, move it to python and then reinject it into lldb)?

lldb/scripts/interface/SBFile.i

81

I am somewhat surprised that there's no ownership handling in this patch (whereas ownership transfer was a fairly big chunk of the conversion in the other direction). Could you maybe explain what are the ownership expectations of the file returned through here (and what happens when we close it for instance)?

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

674–682

It looks like there's just one caller of this constructor (the "out" typemap for FILE*). Can we just inline this stuff there and delete this constructor?

[lawrence_danna]

lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py

775

Yea, foo is bar in python is essentially a pointer comparison between the PyObject* pointers, so this is testing that you get the same identical file object back in the situations where you should.

There's no test going the other way, because going the other way isn't implemented. I didn't write anything that could stash an arbitrary lldb_private::File in a python object . If you convert a non-python File, you will get a new python file based on the descriptor, if it's available, or the conversion will fail if one is not. We do test that here, on line 801, we're testing that a NativeFile is converted to a working python file and the file descriptors match.

We could, in a follow-on patch make the other direction work with identity too, but right now I can't think of what it would be useful for.

lldb/scripts/interface/SBFile.i

81

Oh, yea good point. GetFile returns a borrowed file, it shouldn't be closed. I'll add a comment explaining.

[lawrence_danna]

lldb/include/lldb/Host/File.h

326

Ah, thanks. I orginally wrote this using dyn_cast but I found myself putting classnames of python things in an enum in File.h, so I thought "what's the point?" -- and I converted it to just using the virtual method. The static char ID trick is nice.

[lawrence_danna]

lldb/include/lldb/Host/File.h

65

Are you sure that's the right thing to do? eOpenOptionRead | eOpenOptionWrite has type unsigned int, not OpenOptions.

[lawrence_danna]

review fixes

[lawrence_danna]

I split off the OpenOptions enum stuff into a separate patch, because changing it from uint32_t to the enum is kind of viral

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

674–682

It's also called in some parts of ScriptInterpreterPython that are broken for other reasons, and are getting fixed later in my patch queue. I'll just put a fixme here and delete it when all the other code that uses it also gets deleted, ok?

[labath]

lldb/include/lldb/Host/File.h

65

Ah, right, that was the reason... Regardless, I do thing that's right -- I'll continue the discussion on the other patch.

334–338

Please move this somewhere else -- somewhere near the top or bottom of the class maybe.. It does not make sense to have it in the middle of the open options stuff.

lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py

775

Right, sorry, that came out a bit wrong. While I think it would be cool to have the other direction be an "identity" too, I don't think that is really necessary. Nevertheless, taking a File out of lldb and then back in will do _something_ right now, and that "something" could probably use a test. I suppose you could construct an SBFile from a path, convert it to a python file and back, and then ensure that reading/writing on those two SBFiles does something reasonable.

lldb/scripts/interface/SBFile.i

81

Cool, thanks. This description is very helpful.

93

s/descirptor/descriptor

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

1555–1556

if (auto *simple = ...)

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

674–682

Sounds good.

[lawrence_danna]

review fixes

lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py

775

I was gonna say that this test already covers it, but then I thought, "oh whatever I'll just write another test". And the test I wrote uncovered a bug 😐.

[lawrence_danna]

rebased

[lawrence_danna]

@labath, how's this look now?

[labath]

Looks good, just a couple of quick questions

lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py

775

:)

781

I find this comment confusing. Does that refer to the write call above? If so, I don't see how that is relevant here..

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

1576–1579

What would you say to passing fflush here directly? The signature is compatible with fclose, and fclose is documented to return (a superset of) the errors also reported by fflush. So, it seems like it might be useful to report flushing errors instead of swallowing them. Since accessing the FILE* after a fclose call (even if it fails) is UB, and the python code thinks it's calling fclose, it shouldn't mess up the FILE state in any way even if the flushing fails..

[lawrence_danna]

lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py

781

When we check the output on the next line we have to strip out the 7, which is the number of bytes written.

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

1576–1579

Oh yea good point

[lawrence_danna]

pass ::flush to PyFile_FromFile

[labath]

lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py

781

Ah, right. Thanks for clearing that up.

[mgorny]

Well, I found another problem with these series, specifically starting at this commit. Any use of script in LLDB ends up with:

(lldb) script True
True
close failed in file object destructor:
IOError: [Errno 9] Bad file descriptor

I'd suspect the same fd is using in more than one file but that's only a guess.

The curious part is that I've tried ktruss-ing the process and I don't see any failed close() syscalls.

[lawrence_danna]

In D68737#1722181, @mgorny wrote:

Well, I found another problem with these series, specifically starting at this commit. Any use of script in LLDB ends up with:

(lldb) script True
True
close failed in file object destructor:
IOError: [Errno 9] Bad file descriptor

I'd suspect the same fd is using in more than one file but that's only a guess.

The curious part is that I've tried ktruss-ing the process and I don't see any failed close() syscalls.

I just tested it on commit ef7a154d17f2e38ba3c8bfa33f240b60464e4cc7 (from Thurday), on 8.1_STABLE, And I don't see that happening.

netbsd# ./bin/lldb
(lldb) script True
True
(lldb) exit

[mgorny]

Hmm, that's weird. It happened on the buildbot which is running some recent 8.x kernel, and on my testing machine with 9.x.

[mgorny]

Which Python version are you using? I've just hacked Python 2.7 a bit and found that the failing close() is actually fclose() on FILE* object with fd=0. I can reproduce EBADF only when the same fd is closed twice, so I suspect that something else is closing stdin first, then some extra Python file created for stdin is being closed.

[mgorny]

I've just tried on Linux and it seems that Python closes the same descriptors. So somehow closing stdin works here on Linux, and fails on NetBSD. Curious enough, with a simple test I can verify that both NetBSD and Linux return EBADF when trying to close stdin twice. So unless there's another reason EBADF is returned here, it seems that stdin is closed somewhere earlier on NetBSD, and the same thing doesn't happen on Linux.

However, I think this is only a symptom of a more generic problem. I don't think we should really be creating a second system of file objects outside Python, and injecting it into Python file objects like this. I have a bad feeling for e.g. creating low-level a new file object for stdin, and then having Python destroy it alongside its own stdin object.

[lawrence_danna]

In D68737#1722255, @mgorny wrote:

However, I think this is only a symptom of a more generic problem. I don't think we should really be creating a second system of file objects outside Python, and injecting it into Python file objects like this. I have a bad feeling for e.g. creating low-level a new file object for stdin, and then having Python destroy it alongside its own stdin object.

What do you mean "a second system of file objects outside Python, and injecting it into Python file objects"? That's not what it does. We have lldb_private::File proxies for python files, not the other way around.

[lawrence_danna]

In D68737#1722556, @mgorny wrote:

Which Python version are you using?

3.7. I'll try it with 2.7.

[mgorny]

In D68737#1722575, @lawrence_danna wrote:

In D68737#1722255, @mgorny wrote:

However, I think this is only a symptom of a more generic problem. I don't think we should really be creating a second system of file objects outside Python, and injecting it into Python file objects like this. I have a bad feeling for e.g. creating low-level a new file object for stdin, and then having Python destroy it alongside its own stdin object.

What do you mean "a second system of file objects outside Python, and injecting it into Python file objects"? That's not what it does. We have lldb_private::File proxies for python files, not the other way around.

Hmm, I guess I've been reading that code wrong. I guess I'm out of clues then.

[lawrence_danna]

In D68737#1722620, @mgorny wrote:

In D68737#1722575, @lawrence_danna wrote:

In D68737#1722255, @mgorny wrote:

However, I think this is only a symptom of a more generic problem. I don't think we should really be creating a second system of file objects outside Python, and injecting it into Python file objects like this. I have a bad feeling for e.g. creating low-level a new file object for stdin, and then having Python destroy it alongside its own stdin object.

What do you mean "a second system of file objects outside Python, and injecting it into Python file objects"? That's not what it does. We have lldb_private::File proxies for python files, not the other way around.

Hmm, I guess I've been reading that code wrong. I guess I'm out of clues then.

I can reproduce it with python 2.7. I'll post a fix.

[mgorny]

Great, thanks! So it's not just my computer going crazy after all ;-).

lldb/include/lldb/API/SBFile.h

public:
   SBError Write(const uint8_t *buf, size_t num_bytes, size_t *bytes_written);
   SBError Flush();
   bool IsValid() const;
   SBError Close();
 
   operator bool() const;
   bool operator!() const;
 
+  FileSP GetFile() const;
+
 private:
   FileSP m_opaque_sp;
 };
 
 } // namespace lldb
 
 #endif // LLDB_SBFile_h_

lldb/include/lldb/Host/File.h

enum OpenOptions : uint32_t {
     eOpenOptionCloseOnExec =
         (1u << 8), // Close the file when executing a new process
     LLVM_MARK_AS_BITMASK_ENUM(/* largest_value= */ eOpenOptionCloseOnExec)
   };
 
   static mode_t ConvertOpenOptionsForPOSIXOpen(OpenOptions open_options);
   static llvm::Expected<OpenOptions> GetOptionsFromMode(llvm::StringRef mode);
   static bool DescriptorIsValid(int descriptor) { return descriptor >= 0; };
+  static llvm::Expected<const char *>
+  GetStreamOpenModeFromOptions(OpenOptions options);
 
   File()
       : IOObject(eFDTypeFile), m_is_interactive(eLazyBoolCalculate),
         m_is_real_terminal(eLazyBoolCalculate),
         m_supports_colors(eLazyBoolCalculate){};
 
   /// Read bytes from a file from the current file position into buf.
   ///
public:
   /// \param[in] format
   ///     A printf style format string.
   ///
   /// \param[in] args
   ///     Variable arguments that are needed for the printf style
   ///     format string \a format.
   virtual size_t PrintfVarArg(const char *format, va_list args);
 
+  /// Return the OpenOptions for this file.
+  ///
+  /// Some options like eOpenOptionDontFollowSymlinks only make
+  /// sense when a file is being opened (or not at all)
+  /// and may not be preserved for this method.  But any valid
+  /// File should return either or both of eOpenOptionRead and
+  /// eOpenOptionWrite here.
+  ///
+  /// \return
+  ///    OpenOptions flags for this file, or an error.
+  virtual llvm::Expected<OpenOptions> GetOptions() const;
+
+  llvm::Expected<const char *> GetOpenMode() const {
+    auto opts = GetOptions();
+    if (!opts)
+      return opts.takeError();
+    return GetStreamOpenModeFromOptions(opts.get());
+  }
+
   /// Get the permissions for a this file.
   ///
   /// \return
   ///     Bits logical OR'ed together from the permission bits defined
   ///     in lldb_private::File::Permissions.
   uint32_t GetPermissions(Status &error) const;
 
   /// Return true if this file is interactive.
public:
   /// \return
   ///    True iff this is a terminal and it supports colors.
   bool GetIsTerminalWithColors();
 
   operator bool() const { return IsValid(); };
 
   bool operator!() const { return !IsValid(); };
 
+  static char ID;
+  virtual bool isA(const void *classID) const { return classID == &ID; }
+  static bool classof(const File *file) { return file->isA(&ID); }
+
 protected:
   LazyBool m_is_interactive;
   LazyBool m_is_real_terminal;
   LazyBool m_supports_colors;
 
   void CalculateInteractiveAndTerminal();
 
 private:
public:
   off_t SeekFromStart(off_t offset, Status *error_ptr = nullptr) override;
   off_t SeekFromCurrent(off_t offset, Status *error_ptr = nullptr) override;
   off_t SeekFromEnd(off_t offset, Status *error_ptr = nullptr) override;
   Status Read(void *dst, size_t &num_bytes, off_t &offset) override;
   Status Write(const void *src, size_t &num_bytes, off_t &offset) override;
   Status Flush() override;
   Status Sync() override;
   size_t PrintfVarArg(const char *format, va_list args) override;
+  llvm::Expected<OpenOptions> GetOptions() const override;
+
+  static char ID;
+  virtual bool isA(const void *classID) const override {
+    return classID == &ID || File::isA(classID);
+  }
+  static bool classof(const File *file) { return file->isA(&ID); }
 
 protected:
   bool DescriptorIsValid() const {
     return File::DescriptorIsValid(m_descriptor);
   }
   bool StreamIsValid() const { return m_stream != kInvalidStream; }
 
   // Member variables

lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py

def test_stdout(self):
         f = io.StringIO()
         status = self.debugger.SetOutputFile(f)
         self.assertTrue(status.Success())
         self.handleCmd(r"script sys.stdout.write('foobar\n')")
         self.assertEqual(f.getvalue().strip().split(), ["foobar", "7"])
 
 
     @add_test_categories(['pyapi'])
-    @expectedFailureAll() # FIXME implement SBFile::GetFile
+    def test_stdout_file(self):
+        with open(self.out_filename, 'w') as f:
+            status = self.debugger.SetOutputFile(f)
+            self.assertTrue(status.Success())
+            self.handleCmd(r"script sys.stdout.write('foobar\n')")
+        with open(self.out_filename, 'r') as f:
+            # In python2 sys.stdout.write() returns None, which
+            # the REPL will ignore, but in python3 it will
+            # return the number of bytes written, which the REPL
+            # will print out.
+            lines = [x for x in f.read().strip().split() if x != "7"]
+            self.assertEqual(lines, ["foobar"])
+
+
+    @add_test_categories(['pyapi'])
     @skipIf(py_version=['<', (3,)])
     def test_identity(self):
 
         f = io.StringIO()
         sbf = lldb.SBFile(f)
         self.assertTrue(f is sbf.GetFile())
         sbf.Close()
         self.assertTrue(f.closed)
def test_identity(self):
             self.assertTrue(f is sbf.GetFile())
             sbf.Write(b"foobar\n")
             self.assertEqual(f.fileno(), sbf.GetFile().fileno())
             sbf.Close()
             self.assertTrue(f.closed)
 
         with open(self.out_filename, 'r') as f:
             self.assertEqual("foobar", f.read().strip())
+
+
+    @add_test_categories(['pyapi'])
+    def test_back_and_forth(self):
+        with open(self.out_filename, 'w') as f:
+            # at each step here we're borrowing the file, so we have to keep
+            # them all alive until the end.
+            sbf = lldb.SBFile.Create(f, borrow=True)
+            def i(sbf):
+                for i in range(10):
+                    f = sbf.GetFile()
+                    yield f
+                    sbf = lldb.SBFile.Create(f, borrow=True)
+                    yield sbf
+                    sbf.Write(str(i).encode('ascii') + b"\n")
+            files = list(i(sbf))
+        with open(self.out_filename, 'r') as f:
+            self.assertEqual(list(range(10)), list(map(int, f.read().strip().split())))
+

lldb/scripts/Python/python-typemaps.swig

%typecheck(SWIG_TYPECHECK_POINTER) lldb::FileSP {
   if (lldb_private::PythonFile::Check($input)) {
     $1 = 1;
   } else {
     PyErr_Clear();
     $1 = 0;
   }
 }
 
+%typemap(out) lldb::FileSP {
+  using namespace lldb_private;
+  $result = nullptr;
+  lldb::FileSP &sp = $1;
+  if (sp) {
+    PythonFile pyfile = unwrapOrSetPythonException(PythonFile::FromFile(*sp));
+    if (!pyfile.IsValid())
+      return nullptr;
+    $result = pyfile.release();
+  }
+  if (!$result)
+  {
+      $result = Py_None;
+      Py_INCREF(Py_None);
+  }
+}
 
 // FIXME both of these paths wind up calling fdopen() with no provision for ever calling
 // fclose() on the result.  SB interfaces that use FILE* should be deprecated for scripting
 // use and this typemap should eventually be removed.
 %typemap(in) FILE * {
    using namespace lldb_private;
    if ($input == Py_None)
       $1 = nullptr;

lldb/scripts/interface/SBFile.i

public:
 
     void Flush();
 
     bool IsValid() const;
 
     operator bool() const;
 
     SBError Close();
+
+    %feature("docstring", "
+    Convert this SBFile into a python io.IOBase file object.
+
+    If the SBFile is itself a wrapper around a python file object,
+    this will return that original object.
+
+    The file returned from here should be considered borrowed,
+    in the sense that you may read and write to it, and flush it,
+    etc, but you should not close it.   If you want to close the
+    SBFile, call SBFile.Close().
+
+    If there is no underlying python file to unwrap, GetFile will
+    use the file descriptor, if availble to create a new python
+    file object using `open(fd, mode=..., closefd=False)`
+    ");
+    FileSP GetFile();
 };
 
 } // namespace lldb

lldb/source/API/SBFile.cpp

SBFile::operator bool() const {
   return LLDB_RECORD_RESULT(IsValid());
 }
 
 bool SBFile::operator!() const {
   LLDB_RECORD_METHOD_CONST_NO_ARGS(bool, SBFile, operator!);
   return LLDB_RECORD_RESULT(!IsValid());
 }
 
+FileSP SBFile::GetFile() const {
+  LLDB_RECORD_METHOD_CONST_NO_ARGS(FileSP, SBFile, GetFile);
+  return m_opaque_sp;
+}
+
 namespace lldb_private {
 namespace repro {
 
 template <> void RegisterMethods<SBFile>(Registry &R) {
 
   LLDB_REGISTER_METHOD(lldb::SBError, SBFile, Flush, ());
   LLDB_REGISTER_METHOD_CONST(bool, SBFile, IsValid, ());
   LLDB_REGISTER_METHOD_CONST(bool, SBFile, operator bool,());
   LLDB_REGISTER_METHOD_CONST(bool, SBFile, operator!,());
+  LLDB_REGISTER_METHOD_CONST(FileSP, SBFile, GetFile, ());
   LLDB_REGISTER_METHOD(lldb::SBError, SBFile, Close, ());
 }
 } // namespace repro
 } // namespace lldb_private

lldb/source/Host/common/File.cpp

 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using llvm::Expected;
 
-static Expected<const char *> GetStreamOpenModeFromOptions(uint32_t options) {
+Expected<const char *>
+File::GetStreamOpenModeFromOptions(File::OpenOptions options) {
   if (options & File::eOpenOptionAppend) {
     if (options & File::eOpenOptionRead) {
       if (options & File::eOpenOptionCanCreateNewOnly)
         return "a+x";
       else
         return "a+";
     } else if (options & File::eOpenOptionWrite) {
       if (options & File::eOpenOptionCanCreateNewOnly)
if (result > 0) {
       Write(s, s_len);
       result = s_len;
     }
     free(s);
   }
   return result;
 }
 
+Expected<File::OpenOptions> File::GetOptions() const {
+  return llvm::createStringError(
+      llvm::inconvertibleErrorCode(),
+      "GetOptions() not implemented for this File class");
+}
+
 uint32_t File::GetPermissions(Status &error) const {
   int fd = GetDescriptor();
   if (!DescriptorIsValid(fd)) {
     error = std::error_code(ENOTSUP, std::system_category());
     return 0;
   }
   struct stat file_stats;
   if (::fstat(fd, &file_stats) == -1) {
     error.SetErrorToErrno();
     return 0;
   }
   error.Clear();
   return file_stats.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO);
 }
 
+Expected<File::OpenOptions> NativeFile::GetOptions() const { return m_options; }
+
 int NativeFile::GetDescriptor() const {
   if (DescriptorIsValid())
     return m_descriptor;
 
   // Don't open the file descriptor if we don't need to, just get it from the
   // stream if we have one.
   if (StreamIsValid()) {
 #if defined(_WIN32)
mode_t File::ConvertOpenOptionsForPOSIXOpen(OpenOptions open_options) {
 
   if (open_options & eOpenOptionCanCreateNewOnly)
     mode |= O_CREAT | O_EXCL;
   else if (open_options & eOpenOptionCanCreate)
     mode |= O_CREAT;
 
   return mode;
 }
+
+char File::ID = 0;
+char NativeFile::ID = 0;

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

public:
   static bool Check(PyObject *py_obj);
 
   // Bring in the no-argument base class version
   using PythonObject::Reset;
 
   void Reset(PyRefType type, PyObject *py_obj) override;
 
   ArgInfo GetNumArguments() const;
-  
   // If the callable is a Py_Class, then find the number of arguments
   // of the __init__ method.
   ArgInfo GetNumInitArguments() const;
 
   PythonObject operator()();
 
   PythonObject operator()(std::initializer_list<PyObject *> args);
 
   PythonObject operator()(std::initializer_list<PythonObject> args);
 
   template <typename Arg, typename... Args>
   PythonObject operator()(const Arg &arg, Args... args) {
     return operator()({arg, args...});
   }
 };
 
 class PythonFile : public PythonObject {
 public:
   PythonFile();
-  PythonFile(File &file, const char *mode);
   PythonFile(PyRefType type, PyObject *o);
 
   ~PythonFile() override;
 
   static bool Check(PyObject *py_obj);
 
   using PythonObject::Reset;
 
   void Reset(PyRefType type, PyObject *py_obj) override;
-  void Reset(File &file, const char *mode);
+
+  static llvm::Expected<PythonFile> FromFile(File &file,
+                                             const char *mode = nullptr);
+
+  // FIXME delete this after FILE* typemaps are deleted
+  // and ScriptInterpreterPython is fixed
+  PythonFile(File &file, const char *mode = nullptr) {
+    auto f = FromFile(file, mode);
+    if (f)
+      *this = std::move(f.get());
+    else {
+      Reset();
+      llvm::consumeError(f.takeError());
+    }
+  }
 
   lldb::FileUP GetUnderlyingFile() const;
 
   llvm::Expected<lldb::FileSP> ConvertToFile(bool borrowed = false);
   llvm::Expected<lldb::FileSP>
   ConvertToFileForcingUseOfScriptingIOMethods(bool borrowed = false);
 };
 

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

 
 #include "lldb/Host/File.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Interpreter/ScriptInterpreter.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Stream.h"
 
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/Errno.h"
 
 #include <stdio.h>
 
 using namespace lldb_private;
 using namespace lldb;
 using namespace lldb_private::python;

 operator()(std::initializer_list<PythonObject> args) {
   PythonTuple arg_tuple(args);
   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(PyRefType type, PyObject *o) { Reset(type, o); }
 
 PythonFile::~PythonFile() {}
 
 bool PythonFile::Check(PyObject *py_obj) {
   if (!py_obj)
     return false;
 #if PY_MAJOR_VERSION < 3
if (!PythonFile::Check(py_obj)) {
     return;
   }
 
   // Calling PythonObject::Reset(const PythonObject&) will lead to stack
   // overflow since it calls back into the virtual implementation.
   PythonObject::Reset(PyRefType::Borrowed, result.get());
 }
 
-void PythonFile::Reset(File &file, const char *mode) {
-  if (!file.IsValid()) {
-    Reset();
-    return;
-  }
-
-  char *cmode = const_cast<char *>(mode);
-#if PY_MAJOR_VERSION >= 3
-  Reset(PyRefType::Owned, PyFile_FromFd(file.GetDescriptor(), nullptr, cmode,
-                                        -1, nullptr, "ignore", nullptr, 0));
-#else
-  // Read through the Python source, doesn't seem to modify these strings
-  Reset(PyRefType::Owned,
-        PyFile_FromFile(file.GetStream(), const_cast<char *>(""), cmode,
-                        nullptr));
-#endif
-}
-
-
 FileUP PythonFile::GetUnderlyingFile() const {
   if (!IsValid())
     return nullptr;
 
   // 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 = File::GetOptionsFromMode(py_mode.GetString());
if (!m_borrowed) {
         py_error = Status(r.takeError());
     }
     base_error = Base::Close();
     if (py_error.Fail())
       return py_error;
     return base_error;
   };
 
+  PyObject *GetPythonObject() const {
+    assert(m_py_obj.IsValid());
+    return m_py_obj.get();
+  }
+
+  static bool classof(const File *file) = delete;
+
 protected:
   PythonFile m_py_obj;
   bool m_borrowed;
 };
 } // namespace
 
 // A SimplePythonFile is a OwnedPythonFile that just does all I/O as
 // a NativeFile
 namespace {
 class SimplePythonFile : public OwnedPythonFile<NativeFile> {
 public:
   SimplePythonFile(const PythonFile &file, bool borrowed, int fd,
                    File::OpenOptions options)
       : OwnedPythonFile(file, borrowed, fd, options, false) {}
+
+  static char ID;
+  bool isA(const void *classID) const override {
+    return classID == &ID || NativeFile::isA(classID);
+  }
+  static bool classof(const File *file) { return file->isA(&ID); }
 };
+char SimplePythonFile::ID = 0;
 } // namespace
 
 #if PY_MAJOR_VERSION >= 3
 
 namespace {
 class PythonBuffer {
 public:
   PythonBuffer &operator=(const PythonBuffer &) = delete;
public:
   Status Flush() override {
     GIL takeGIL;
     auto r = m_py_obj.CallMethod("flush");
     if (!r)
       return Status(r.takeError());
     return Status();
   }
 
+  Expected<File::OpenOptions> GetOptions() const override {
+    GIL takeGIL;
+    return GetOptionsForPyObject(m_py_obj);
+  }
+
+  static char ID;
+  bool isA(const void *classID) const override {
+    return classID == &ID || File::isA(classID);
+  }
+  static bool classof(const File *file) { return file->isA(&ID); }
 };
+char PythonIOFile::ID = 0;
 } // namespace
 
 namespace {
 class BinaryPythonFile : public PythonIOFile {
 protected:
   int m_descriptor;
 
 public:
if (!file_sp->IsValid())
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "invalid File");
 
   return file_sp;
 
 #endif
 }
 
+Expected<PythonFile> PythonFile::FromFile(File &file, const char *mode) {
+  if (!file.IsValid())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "invalid file");
+
+  if (auto *simple = llvm::dyn_cast<SimplePythonFile>(&file))
+    return Retain<PythonFile>(simple->GetPythonObject());
+#if PY_MAJOR_VERSION >= 3
+  if (auto *pythonio = llvm::dyn_cast<PythonIOFile>(&file))
+    return Retain<PythonFile>(pythonio->GetPythonObject());
+#endif
+
+  if (!mode) {
+    auto m = file.GetOpenMode();
+    if (!m)
+      return m.takeError();
+    mode = m.get();
+  }
+
+  PyObject *file_obj;
+#if PY_MAJOR_VERSION >= 3
+  file_obj = PyFile_FromFd(file.GetDescriptor(), nullptr, mode, -1, nullptr,
+                           "ignore", nullptr, 0);
+#else
+  // Read through the Python source, doesn't seem to modify these strings
+  char *cmode = const_cast<char *>(mode);
+  // We pass ::flush instead of ::fclose here so we borrow the FILE* --
+  // the lldb_private::File still owns it.
+  file_obj =
+      PyFile_FromFile(file.GetStream(), const_cast<char *>(""), cmode, ::fflush);
+#endif
+
+  if (!file_obj)
+    return exception();
+
+  return Take<PythonFile>(file_obj);
+}
+
 #endif