allow arbitrary python streams to be converted to SBFile

[lawrence_danna]

Summary

This patch adds SWIG typemaps that can convert arbitrary python
file objects into lldb_private::File.

A SBFile may be initialized from a python file using the
constructor. There are also alternate, tagged constructors
that allow python files to be borrowed, and for the caller
to control whether or not the python I/O methods will be
called even when a file descriptor is available.I

[labath]

I haven't looked at the patch in detail, but I have one high-level question. It seems to be that instead of subclassing a fully-functional File class, it would be better to create an abstract class, that both File and the new python thingy could implement. That would also create a natural place to write down the FILE* conversion semantics and other stuff we talked about at lldb-dev. I don't have any opinion as to whether the name File should denote the new abstract class, or the real thing, but it should most likely be done as a separate patch.

What do you think about that?

lldb/include/lldb/API/SBFile.h

16–21

I don't think we have anything similar to this in any of our SB classes. It might be better to avoid it. Could the same thing be achieved e.g. with a static factory function (static SBFile SBFile::Create(FileSP file_sp, bool borrow, bool force_scripting_io)) ?

[lawrence_danna]

lldb/include/lldb/API/SBFile.h

16–21

I don't think it can be done that way, because we want those bools to control how the python object is translated into a C++ object, so we need a different swig wrapper for each possibility, which means we need a different c++ function to wrap for each possibility.

One way around this would be to have SWIG translate the python object into an intermediate object that just takes a reference, and then perform further translation inside the SB layer. But we can't do that either, because scripting support is not part of the base LLDB library, it's a plugin. And in fact Xcode ships two versions of the plugin, one for python2 and one for python3. The SB needs to be agnostic about different versions of python, so it can't do the translation itself.

[lawrence_danna]

In D68188#1687532, @labath wrote:

It seems to be that instead of subclassing a fully-functional File class, it would be better to create an abstract class, that both File and the new python thingy could implement. That would also create a natural place to write down the FILE* conversion semantics and other stuff we talked about at lldb-dev. I don't have any opinion as to whether the name File should denote the new abstract class, or the real thing, but it should most likely be done as a separate patch.

What do you think about that?

I could do it that way, but I have some reservations about it. In this version the class hierarchy looks like this:

          File
           |
 SimplePythonFile --------+
  |                       |
TextPythonFile     BinaryPythonFile

If we added an abstract base class, I guess it would look like this:

          AbstractFile
           |        |                
          File      PythonFile
           |         |     | |
         SimplePythonFile  | |
                           | |
  + -----------------------+ |  
  |                          |
TextPythonFile     BinaryPythonFile

Does a more complicated class hierarchy like that really solve a problem or make the code easier to understand?

[labath]

In D68188#1689104, @lawrence_danna wrote:

In D68188#1687532, @labath wrote:

It seems to be that instead of subclassing a fully-functional File class, it would be better to create an abstract class, that both File and the new python thingy could implement. That would also create a natural place to write down the FILE* conversion semantics and other stuff we talked about at lldb-dev. I don't have any opinion as to whether the name File should denote the new abstract class, or the real thing, but it should most likely be done as a separate patch.

What do you think about that?

I could do it that way, but I have some reservations about it. In this version the class hierarchy looks like this:

          File
           |
 SimplePythonFile --------+
  |                       |
TextPythonFile     BinaryPythonFile

If we added an abstract base class, I guess it would look like this:

          AbstractFile
           |        |                
          File      PythonFile
           |         |     | |
         SimplePythonFile  | |
                           | |
  + -----------------------+ |  
  |                          |
TextPythonFile     BinaryPythonFile

Does a more complicated class hierarchy like that really solve a problem or make the code easier to understand?

No that wouldn't make things more understandable, but I think it has helped me understand what are you doing here. The usual OO answer to attempts to introduce complex class hierarchies is to try to use object composition instead. Trying to apply that here gives me the following set of classes.

• File (or AbstractFile ?) - defines just the abstract interface, documents what the interface is
• NativeFile (or File ?) - implements the File interface via the usual FILE* and file descriptor routines (one day these may be split into two classes too)
• TextPythonFile - implements the File interface via Python TextIOBase interface. Does not handle ownership/closing.
• BinaryPythonFile - implements the File interface via Python RawIOBase interface. Does not handle ownership/closing.
• OwnedPythonFile - implements the File interface by delegating to another File object. Has special handling for the close method.

This way the decision whether to "own" something is decoupled from the decision what api do you use to actually write to the file, and I _think_ it would make things clearer. What do you make of that? The thing I don't like about the current solution is that it leaves you with unused FILE* members in the Text/BinaryPythonFile classes, and forces the introduction of weird-looking OverridesIO methods...

(BTW, as this is C++, it allows you to "cheat" and instead of creating another File object the delegation can be incorporated into the type system by making OwnedPythonFile a template parameterized on the base class. I am fine with both solutions though I think I'd prefer the regular class variant.)

lldb/include/lldb/API/SBFile.h

16–21

Are you sure about that? Swig allows you to define typemaps for sequences of values. This example is taken from the swig documentation:

%typemap(in) (char *str, int len) {
    $1 = PyString_AsString($input);   /* char *str */
    $2 = PyString_Size($input);       /* int len   */
}

It seems to me that your typemap could switch on the value of the flag at runtime rather than having compile time switching by swig. This way you wouldn't even need a static function, you could just have an additional constructor with two extra bool arguments.

However, if this is not the preferred syntax for some reason, then we could also have a set of static functions, one for each flag combination, do the conversion that way.

(BTW, the main reason I am trying to avoid the tag structs is because they are wading into uncharted territory in terms of our SB coding guidelines https://lldb.llvm.org/resources/sbapi.html)

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

500 ↗

(On Diff #222313)

no else after return.

[lawrence_danna]

updated based on an abstract base clase

[lawrence_danna]

In D68188#1689544, @labath wrote:

This way the decision whether to "own" something is decoupled from the decision what api do you use to actually write to the file, and I _think_ it would make things clearer. What do you make of that?

Yea I think that works. Updated. I decided to go with the template thingy because there's kind of a lot of virtual methods to delegate and it's nice to have a list of them in the header.

[lawrence_danna]

rename LLDBPythonFile -> OwnedPythonFile

[lawrence_danna]

style fix

[lawrence_danna]

use a python helper instead of funny tag structs

[lawrence_danna]

lldb/include/lldb/API/SBFile.h

16–21

I really do think it can't be done with swig. You can group multiple arguments together on the C++ side, but not on the python side.

I think I did find a reasonable way to deal with it though without uglying up the SB headers with those tags. I added some statics with %extend, and then wrapped them with a python-friendly classmethod called Create, with both flags as keyword args.

How's that look?

[labath]

Hmm... I like your solution of the typemap problem, but this CRTP class seems to be way more complicated than needed. There shouldn't be any need for CRTP as we already have regular dynamic dispatch via virtual methods. Also, I don't think the forwarding should be needed if you're going the template route. How about something like this instead?

template<typename Base>
class OwningPythonFile : public Base {
  Status Close() { /* close magic here*/; }
  // no need to forward anything -- inheritance handles that
};

class TextPythonFile: public OwningPythonFile<File> {
  // override methods as usual
};
// same for BinaryPythonFile

Then depending on what you need, you create either a NativeFile, OwningPythonFile<NativeFile>, or a Text/BinaryPythonFile. How does that sound?

[lawrence_danna]

In D68188#1691129, @labath wrote:

Hmm... I like your solution of the typemap problem, but this CRTP class seems to be way more complicated than needed. There shouldn't be any need for CRTP as we already have regular dynamic dispatch via virtual methods. Also, I don't think the forwarding should be needed if you're going the template route. How about something like this instead?

template<typename Base>
class OwningPythonFile : public Base {
  Status Close() { /* close magic here*/; }
  // no need to forward anything -- inheritance handles that
};

class TextPythonFile: public OwningPythonFile<File> {
  // override methods as usual
};
// same for BinaryPythonFile

Then depending on what you need, you create either a NativeFile, OwningPythonFile<NativeFile>, or a Text/BinaryPythonFile. How does that sound?

Next step is add type maps to convert these things back to python. Without the CRTP it can’t just check for a single OwnedPythonfile base class — it’ll have to check for all three. Is that worse than using CRTP?

[labath]

In D68188#1691337, @lawrence_danna wrote:

In D68188#1691129, @labath wrote:

Then depending on what you need, you create either a NativeFile, OwningPythonFile<NativeFile>, or a Text/BinaryPythonFile. How does that sound?

Next step is add type maps to convert these things back to python. Without the CRTP it can’t just check for a single OwnedPythonfile base class — it’ll have to check for all three. Is that worse than using CRTP?

Hmm... hard to say without knowing more details, but I would say that being worse than CRTP is quite hard. :) Also, Text and Binary python file will have OwningPythonFile<File> as their base, so you can check for those together. I'd say we go with this first, and then revisit if needed.

[lawrence_danna]

rebased

[lawrence_danna]

get rid of CRTP gobbledygook

[lawrence_danna]

rebased

[labath]

Ok, I think that the major issues are out of the way now. I've done a more careful pass over the file, and I think this is looking pretty good. I am particularly happy with how you've implemented (and tested) the interaction with python exceptions. A bunch of additional comments inline, but none really that important.

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

342–343

Actually, I'm pretty sure that this category annotation is unnecessary, as the python_api contains a .categories file specifying this category. Also, the @no_debug_info_test thingy might be better achieved by setting NO_DEBUG_INFO_TESTCASE = True class property.

472–473

self.assertTrue(status.Succes()). If you're doing this because you want a better error message or something, feel free to implement something like assertSuccess or the like. I'd be happy to use that, as I want better error messages too.

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

1100

All of these file-local classes (unless you choose to move them to a separate file) should go into an anonymous namespace. See http://llvm.org/docs/CodingStandards.html#anonymous-namespaces on the recommended way to do that.

1130–1132

We're using LLDB_LOG or LLDB_LOGF these days. The first one is prefereed if you have complex values that can't be displayed by printf natively (e.g. StringRef).

1237

s/Simply/Simple

1241–1243

This reordering of arguments is confusing. Can we make the two orders match?

1299–1306

How about if OwnedPythonFile defines IsValid() as IsPythonObjectValid() || Base::IsValid(). Then PythonIOFile could redefine it to be simply IsPythonObjectValid() without the need for the extra class?

1312

no semicolon

1316–1325

Hmm... how exactly does this differ from the implementation in OwnedPythonFile? Is it only that it does not chain into the base class? Could we use the same trick as for IsValid to reduce the duplication?

1339

What is this needed for? I don't see the PythonFile class doing anything funny (and it probably shouldn't).

1458

I was struggling to find this method, as I was looking it next to the other PythonFile methods. I get why it's here, but I think it's confusing to have PythonFile be split this way. Could you move the other classes so that they are declared before all of PythonFile methods. Or, given that this file is starting to become largish, maybe move some of this stuff into a separate file?

[lawrence_danna]

We need to obsessively check PyErr_Occurred() before calling
into the next python API, even when we'd just catch it a
little later. Python APIs will generally work if you don't,
but it's undefined behavior to do it and debug builds of python
will assert.

[lawrence_danna]

no_debug_info_test -> NO_DEBUG_INFO_TESTCASE

[lawrence_danna]

anonymous namespace, and a windows fix

[lawrence_danna]

assert instead of raise

[lawrence_danna]

use LLDB_LOGF and add a logging test

[lawrence_danna]

cleanup

[lawrence_danna]

cleanup

[lawrence_danna]

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

1299–1306

If I did that then a SimplePythonFile would still be valid if the file was closed on the python side... seems like the wrong behavior.

1312

huh?

1339

python API functions expect you to check for an exception with PyErr_Occured() and clear it before calling into them again. If you don't things usually work out OK, except on a debug build in which an assert will trip.

[lawrence_danna]

group all the PythonFile support together in the source file

[labath]

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

1299–1306

Sorry, I meant &&: IsPythonObjectValid() && Base::IsValid(). Basically, I'm trying to see if there's a reasonable way to reduce the number of classes floating around, and this PresumptivelyValidFile seems like it could be avoided...

1312

There's no need to put a semicolon after the body of the constructor.

1339

Are you sure you're looking at the right place (the comments tend to move around as you reorganize code)? I was asking why is PythonFile a friend of the BinaryPythonFile class. That doesn't seem relevant to the PyErr_Occured checks...

[lawrence_danna]

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

1316–1325

they actually have different behavior. The OwnedPythonFile version chains into the base class, but the PythonIOFile version also flushes the stream on the python side if the file is borrowed. The idea is that closing any file should flush the buffer you've been writing too even if the file is borrowed. That way you don't get any weird surprises where you "close" a file and nothing gets written.

[lawrence_danna]

remove unnecessary friends

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

1299–1306

If i did it that way, with && and no PresumptivelyValidFile then the python IO files would chain into the base File class and say they're invalid.

That's how I coded it up at first, I didn't notice I needed PresumptivelyValidFile until I saw the python IO files coming up as invalid.

1339

oh, yea it got moved around. fixed.

[lawrence_danna]

semicolons

[lawrence_danna]

fixed

[labath]

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

1299–1306

Yeah, but that's where the second part of this idea comes in. The python IO files can re-override IsValid() so that it does *not* chain into the base class, and just calls IsPythonObjectValid() from OwnedPythonFile. That arrangement seems to make sense to me, though I am willing to be convinced otherwise.

[lawrence_danna]

rm class PresumptivelyValidFile

[lawrence_danna]

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

1299–1306

oh, yea that works.

[labath]

Thanks. I was just about to hit approve, but then I noticed one other thing... :/ It seems that somebody (I guess it was @zturner) spent a lot of time in creating the whole PythonObject hierarchy, and it's (worthwhile) goal seems to be to enable the rest of the to code to avoid dealing with the python APIs directly. However, here you're ignoring all of that, and jumping to the C python interface directly.

I'd like to know what's your take on that? Have you considered basing these file classes on the PythonDataObject stuff?

The main reason I mention that is your comment about PyErr_Occurred, and the resulting boiler plate it produced. This seems like something that would be nicely solved by some c++-like wrapper, which is exactly what the PythonObject stuff is AFAICT.

Most of your interactions seem to be about calling methods. Would it be possible to add a PythonDataObject wrapper for this (and any other frequently used python API)? I'm hoping that we could have something like:

if (Expected<PythonInteger> i = m_py_obj.CallMethod<PythonInteger>(pybuffer))
  l_bytes_written = i->GetInteger();
else
  return Status(i.takeError());

instead of stuff like

auto bytes_written = Take<PythonObject>(
    PyObject_CallMethod(m_py_obj, "write", "(O)", pybuffer.get()));
if (PyErr_Occurred())
  return Status(llvm::make_error<PythonException>("Write"));
long l_bytes_written = PyLong_AsLong(bytes_written.get());
if (PyErr_Occurred())
  return Status(llvm::make_error<PythonException>("Write"));

It doesn't look like adding this should be too hard (including the template-based computation of the python signature), but it could go a long way towards improving the readability, and it might actually fix other issues too -- it seems that the PythonDataObjects currently completely ignore the PyErr_Occurred stuff, which seems to be wrong, and it could actually be the reason why @amccarth was unable to run the test suite with a debug python on windows. Improving that, even if it's just for the stuff that's needed for your work would be really helpful, as it would provide a pattern on how to fix the remaining issues.

I'm sorry that I'm bringing this up so late in the review, but I am also learning this stuff as we go along -- unfortunately, I don't think we have anyone really knowledgeable about the python stuff still active in the lldb community..

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

1223

Maybe this GIL-talking can be done inside IsPythonSideValid (instead of two IsValid methods)?

1255

extra semicolon here too

1427

This is an example where _I think_ you may be using PyErr_Occurred incorrectly. If python really asserts that PyErr_Occurred is always called, then this is not right, as you will not call it if the result is null. There are a couple of other examples like that in the code too.

[lawrence_danna]

In D68188#1694276, @labath wrote:

Most of your interactions seem to be about calling methods. Would it be possible to add a PythonDataObject wrapper for this (and any other frequently used python API)? I'm hoping that we could have something like...

I think this is a good idea, but there's a lot of places in PythonDataObjects that need to be fixed to check for exceptions and return Expected<>. I really think it ought to be done in a separate patch.

[lawrence_danna]

minor fixes

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

1427

I think it is correct.

Python doesn't assert that PyErr_Occured is called, it asserts that the error is cleared before you call back in to another thing that can cause an error. PythonException will clear the error if there is one, or say "unknown error" if there isn't one (which should never happen if utf8==NULL, but i'm checking out of abundance of caution)

[labath]

I agree about the separate patch stuff, but it seems to be that this should be done before this one. After all, all (most?) of the existing code has already been DataObject-ized and this patch is the thing that's deviating from that practice. I don't think you should rewrite all of the existing code -- hopefully we can find a way to fit this in there. For instance, the CallMethod thingy would be a new api, so we can have that return expected independently of the other APIs. I guess the main part you will need is (and that already existing in the non-Expected form) is the type conversions, but maybe there we can introduce a new kind of conversion, which is more strict in checking for errors. Or possibly have the current form return Expected<T>, but then have a helper function to suppress those errors and return a default-constructed T() for legacy uses (so legacy code would be sth like suppressError(object.AsType<PythonInteger>()))

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

1427

Ok, thanks for the explanation.

[lawrence_danna]

better integration with PythonObject

[lawrence_danna]

In D68188#1695069, @labath wrote:

I agree about the separate patch stuff, but it seems to be that this should be done before this one. After all, all (most?) of the existing code has already been DataObject-ized and this patch is the thing that's deviating from that practice. I don't think you should rewrite all of the existing code -- hopefully we can find a way to fit this in there.

Yea... I tried updating all the PythonDataObjects code to use Errors and Expected<> and it wound up being just way too big of a patch.

I've updated this patch to add a giant warning at the top of PythonDataObjects.h about the dangers, as well adding just enough Expected<> support to do what I need for this patch.

How's it look now?

[lawrence_danna]

spelling

[lawrence_danna]

comment

[lawrence_danna]

I changed my mind about splitting this into another patch. When I first said that I had in mind a much more extensive rewrite of the PythonDataObjects. I think it's fine now as one patch -- though we should still go back later add Expected<> everywhere in PythonDataObjects.

[lawrence_danna]

python2 const fix

[lawrence_danna]

fix typemaps to deal with None correctly

[lawrence_danna]

rebased

[lawrence_danna]

add safe PythonModule functions and use them

[lawrence_danna]

split off generic exception handling stuff into a separate patch

[lawrence_danna]

rebased

[lawrence_danna]

@labath anything else for this one?

[lawrence_danna]

rebased

[lawrence_danna]

rebased

[labath]

I think this is looking pretty good. I like how the read/write methods look now. Just one more question about the PythonBuffer class.

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

1258–1265

Storing the Error as a member is quite unusual. It would be better to have a factory function which returns Expected<PythonBuffer>.

[lawrence_danna]

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

1258–1265

Oh yea I’ll fix that

[lawrence_danna]

fixed pybuffer error handling weirdness

[lawrence_danna]

fixed

[labath]

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

1284

I would slightly prefer if the PyObject_GetBuffer happened in the Create function (and the constructor took an already-constructed Py_buffer). That way, the call is next to the exception check and you avoid creating an "invalid" PythonBuffer object, even as an implementation detail.

[lawrence_danna]

Pavel would slightly prefer if the PyObject_GetBuffer happened in the Create function

[jankratochvil]

Fixed now by rL374322, there was a regression - Linux Fedora 30 x86_64:

======================================================================
ERROR: test_exceptions (TestFileHandle.FileHandleTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jkratoch/redhat/llvm-monorepo2/lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py", line 483, in test_exceptions
    self.assertRaises(TypeError, lldb.SBFile, None)
  File "/home/jkratoch/redhat/llvm-monorepo2/lldb/third_party/Python/module/unittest2/unittest2/case.py", line 534, in assertRaises
    callableObj(*args, **kwargs)
  File "/home/jkratoch/redhat/llvm-monorepo2-clangassert/lib/python2.7/site-packages/lldb/__init__.py", line 5334, in __init__
    this = _lldb.new_SBFile(*args)
NotImplementedError: Wrong number or type of arguments for overloaded function 'new_SBFile'.
  Possible C/C++ prototypes are:
    lldb::SBFile::SBFile()
    lldb::SBFile::SBFile(int,char const *,bool)
    lldb::SBFile::SBFile(lldb::FileSP)

[labath]

Yeah, as Jan mentions, I've had to make some tweaks (r374331, r374322, r374299) in order to get this passing on all python and swig versions. Let me know if you have questions/issues/etc about any of those.

lldb/include/lldb/API/SBFile.h

 //===-- SBFile.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 LLDB_SBFile_h_
 #define LLDB_SBFile_h_
 
 #include "lldb/API/SBDefines.h"
 
 namespace lldb {
 
 class LLDB_API SBFile {
   friend class SBDebugger;
 
 public:
   SBFile();
+  SBFile(FileSP file_sp);
   SBFile(FILE *file, bool transfer_ownership);
   SBFile(int fd, const char *mode, bool transfer_ownership);
   ~SBFile();
 
   SBError Read(uint8_t *buf, size_t num_bytes, size_t *bytes_read);
   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;
 
 private:
   FileSP m_opaque_sp;
-  SBFile(FileSP file_sp);
 };
 
 } // namespace lldb
 
 #endif // LLDB_SBFile_h_

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

 """
 Test lldb Python API for file handles.
 """
 
 from __future__ import print_function
 
 import os
 import io
 import re
 import sys
 from contextlib import contextmanager
 
 import lldb
 from lldbsuite.test import  lldbtest
 from lldbsuite.test.decorators import (
-    add_test_categories, no_debug_info_test, skipIf)
+    add_test_categories, skipIf, skipIfWindows)
 
+class OhNoe(Exception):
+    pass
+
+class BadIO(io.TextIOBase):
+    @property
+    def closed(self):
+        return False
+    def writable(self):
+        return True
+    def readable(self):
+        return True
+    def write(self, s):
+        raise OhNoe('OH NOE')
+    def read(self, n):
+        raise OhNoe("OH NOE")
+    def flush(self):
+        raise OhNoe('OH NOE')
+
+# This class will raise an exception while it's being
+# converted into a C++ object by swig
+class ReallyBadIO(io.TextIOBase):
+    def fileno(self):
+        return 999
+    def writable(self):
+        raise OhNoe("OH NOE!!!")
+
+class MutableBool():
+    def __init__(self, value):
+        self.value = value
+    def set(self, value):
+        self.value = bool(value)
+    def __bool__(self):
+        return self.value
+
+class FlushTestIO(io.StringIO):
+    def __init__(self, mutable_flushed, mutable_closed):
+        super(FlushTestIO, self).__init__()
+        self.mut_flushed = mutable_flushed
+        self.mut_closed = mutable_closed
+    def close(self):
+        self.mut_closed.set(True)
+        return super(FlushTestIO, self).close()
+    def flush(self):
+        self.mut_flushed.set(True)
+        return super(FlushTestIO, self).flush()
 
 @contextmanager
 def replace_stdout(new):
     old = sys.stdout
     sys.stdout = new
     try:
         yield
     finally:
         sys.stdout = old
 
 def readStrippedLines(f):
     def i():
         for line in f:
             line = line.strip()
             if line:
                 yield line
     return list(i())
 
 
 class FileHandleTestCase(lldbtest.TestBase):
 
+    NO_DEBUG_INFO_TESTCASE = True
     mydir = lldbtest.Base.compute_mydir(__file__)
 
     # The way this class interacts with the debugger is different
     # than normal.   Most of these test cases will mess with the
     # debugger I/O streams, so we want a fresh debugger for each
     # test so those mutations don't interfere with each other.
     #
     # Also, the way normal tests evaluate debugger commands is
def handleCmd(self, cmd, check=True, collect_result=True):
         self.debugger.GetOutputFile().Flush()
         self.debugger.GetErrorFile().Flush()
         if collect_result and check:
             self.assertTrue(ret.Succeeded())
         return ret.GetOutput()
 
 
     @add_test_categories(['pyapi'])
-    @no_debug_info_test
+    @skipIfWindows # FIXME pre-existing bug, should be fixed
+                   # when we delete the FILE* typemaps.
     def test_legacy_file_out_script(self):
         with open(self.out_filename, 'w') as f:
             self.debugger.SetOutputFileHandle(f, False)
             # scripts print to output even if you capture the results
             # I'm not sure I love that behavior, but that's the way
             # it's been for a long time.  That's why this test works
             # even with collect_result=True.
             self.handleCmd('script 1+1')
             self.debugger.GetOutputFileHandle().write('FOO\n')
         lldb.SBDebugger.Destroy(self.debugger)
         with open(self.out_filename, 'r') as f:
             self.assertEqual(readStrippedLines(f), ['2', 'FOO'])
 
 
     @add_test_categories(['pyapi'])
-    @no_debug_info_test
     def test_legacy_file_out(self):
         with open(self.out_filename, 'w') as f:
             self.debugger.SetOutputFileHandle(f, False)
             self.handleCmd('p/x 3735928559', collect_result=False, check=False)
         lldb.SBDebugger.Destroy(self.debugger)
         with open(self.out_filename, 'r') as f:
             self.assertIn('deadbeef', f.read())
 
     @add_test_categories(['pyapi'])
-    @no_debug_info_test
+    @skipIfWindows # FIXME pre-existing bug, should be fixed
+                   # when we delete the FILE* typemaps.
     def test_legacy_file_err_with_get(self):
         with open(self.out_filename, 'w') as f:
             self.debugger.SetErrorFileHandle(f, False)
             self.handleCmd('lolwut', check=False, collect_result=False)
             self.debugger.GetErrorFileHandle().write('FOOBAR\n')
         lldb.SBDebugger.Destroy(self.debugger)
         with open(self.out_filename, 'r') as f:
             errors = f.read()
             self.assertTrue(re.search(r'error:.*lolwut', errors))
             self.assertTrue(re.search(r'FOOBAR', errors))
 
 
     @add_test_categories(['pyapi'])
-    @no_debug_info_test
     def test_legacy_file_err(self):
         with open(self.out_filename, 'w') as f:
             self.debugger.SetErrorFileHandle(f, False)
             self.handleCmd('lol', check=False, collect_result=False)
         lldb.SBDebugger.Destroy(self.debugger)
         with open(self.out_filename, 'r') as f:
             self.assertIn("is not a valid command", f.read())
 
 
     @add_test_categories(['pyapi'])
-    @no_debug_info_test
     def test_sbfile_type_errors(self):
         sbf = lldb.SBFile()
         self.assertRaises(TypeError, sbf.Write, None)
         self.assertRaises(TypeError, sbf.Read, None)
         self.assertRaises(TypeError, sbf.Read, b'this bytes is not mutable')
         self.assertRaises(TypeError, sbf.Write, u"ham sandwich")
         self.assertRaises(TypeError, sbf.Read, u"ham sandwich")
 
 
     @add_test_categories(['pyapi'])
-    @no_debug_info_test
-    def test_sbfile_write(self):
+    def test_sbfile_write_fileno(self):
         with open(self.out_filename, 'w') as f:
             sbf = lldb.SBFile(f.fileno(), "w", False)
             self.assertTrue(sbf.IsValid())
             e, n = sbf.Write(b'FOO\nBAR')
             self.assertTrue(e.Success())
             self.assertEqual(n, 7)
             sbf.Close()
             self.assertFalse(sbf.IsValid())
         with open(self.out_filename, 'r') as f:
             self.assertEqual(readStrippedLines(f), ['FOO', 'BAR'])
 
 
     @add_test_categories(['pyapi'])
-    @no_debug_info_test
-    def test_sbfile_read(self):
+    def test_sbfile_write(self):
+        with open(self.out_filename, 'w') as f:
+            sbf = lldb.SBFile(f)
+            e, n = sbf.Write(b'FOO\n')
+            self.assertTrue(e.Success())
+            self.assertEqual(n, 4)
+            sbf.Close()
+            self.assertTrue(f.closed)
+        with open(self.out_filename, 'r') as f:
+            self.assertEqual(f.read().strip(), 'FOO')
+
+
+    @add_test_categories(['pyapi'])
+    def test_sbfile_read_fileno(self):
         with open(self.out_filename, 'w') as f:
             f.write('FOO')
         with open(self.out_filename, 'r') as f:
             sbf = lldb.SBFile(f.fileno(), "r", False)
             self.assertTrue(sbf.IsValid())
             buffer = bytearray(100)
             e, n = sbf.Read(buffer)
             self.assertTrue(e.Success())
             self.assertEqual(buffer[:n], b'FOO')
 
 
     @add_test_categories(['pyapi'])
-    @no_debug_info_test
+    def test_sbfile_read(self):
+        with open(self.out_filename, 'w') as f:
+            f.write('foo')
+        with open(self.out_filename, 'r') as f:
+            sbf = lldb.SBFile(f)
+            buf = bytearray(100)
+            e, n = sbf.Read(buf)
+            self.assertTrue(e.Success())
+            self.assertEqual(n, 3)
+            self.assertEqual(buf[:n], b'foo')
+            sbf.Close()
+            self.assertTrue(f.closed)
+
+
+    @add_test_categories(['pyapi'])
     def test_fileno_out(self):
         with open(self.out_filename, 'w') as f:
             sbf = lldb.SBFile(f.fileno(), "w", False)
             status = self.debugger.SetOutputFile(sbf)
             self.assertTrue(status.Success())
             self.handleCmd('script 1+2')
             self.debugger.GetOutputFile().Write(b'quux')
 
         with open(self.out_filename, 'r') as f:
             self.assertEqual(readStrippedLines(f), ['3', 'quux'])
 
 
     @add_test_categories(['pyapi'])
-    @no_debug_info_test
     def test_fileno_help(self):
         with open(self.out_filename, 'w') as f:
             sbf = lldb.SBFile(f.fileno(), "w", False)
             status = self.debugger.SetOutputFile(sbf)
             self.assertTrue(status.Success())
             self.handleCmd("help help", collect_result=False, check=False)
         with open(self.out_filename, 'r') as f:
             self.assertTrue(re.search(r'Show a list of all debugger commands', f.read()))
 
 
     @add_test_categories(['pyapi'])
-    @no_debug_info_test
     def test_immediate(self):
         with open(self.out_filename, 'w') as f:
             ret = lldb.SBCommandReturnObject()
             ret.SetImmediateOutputFile(f)
             interpreter = self.debugger.GetCommandInterpreter()
             interpreter.HandleCommand("help help", ret)
             # make sure the file wasn't closed early.
             f.write("\nQUUX\n")
 
         ret = None # call destructor and flush streams
 
         with open(self.out_filename, 'r') as f:
             output = f.read()
             self.assertTrue(re.search(r'Show a list of all debugger commands', output))
             self.assertTrue(re.search(r'QUUX', output))
 
 
     @add_test_categories(['pyapi'])
-    @no_debug_info_test
     def test_fileno_inout(self):
         with open(self.in_filename, 'w') as f:
             f.write("help help\n")
 
         with open(self.out_filename, 'w') as outf, open(self.in_filename, 'r') as inf:
 
             outsbf = lldb.SBFile(outf.fileno(), "w", False)
             status = self.debugger.SetOutputFile(outsbf)
             self.assertTrue(status.Success())
 
             insbf = lldb.SBFile(inf.fileno(), "r", False)
             status = self.debugger.SetInputFile(insbf)
             self.assertTrue(status.Success())
 
             opts = lldb.SBCommandInterpreterRunOptions()
             self.debugger.RunCommandInterpreter(True, False, opts, 0, False, False)
             self.debugger.GetOutputFile().Flush()
 
         with open(self.out_filename, 'r') as f:
             self.assertTrue(re.search(r'Show a list of all debugger commands', f.read()))
 
 
     @add_test_categories(['pyapi'])
-    @no_debug_info_test
     def test_fileno_error(self):
         with open(self.out_filename, 'w') as f:
 
             sbf = lldb.SBFile(f.fileno(), 'w', False)
             status = self.debugger.SetErrorFile(sbf)
             self.assertTrue(status.Success())
 
             self.handleCmd('lolwut', check=False, collect_result=False)
 
             self.debugger.GetErrorFile().Write(b'\nzork\n')
 
         with open(self.out_filename, 'r') as f:
             errors = f.read()
             self.assertTrue(re.search(r'error:.*lolwut', errors))
             self.assertTrue(re.search(r'zork', errors))
 
     #FIXME This shouldn't fail for python2 either.
     @add_test_categories(['pyapi'])
-    @no_debug_info_test
     @skipIf(py_version=['<', (3,)])
     def test_replace_stdout(self):
         f = io.StringIO()
         with replace_stdout(f):
             self.assertEqual(sys.stdout, f)
             self.handleCmd('script sys.stdout.write("lol")',
                 collect_result=False, check=False)
             self.assertEqual(sys.stdout, f)
+
+
+    @add_test_categories(['pyapi'])
+    def test_sbfile_write_borrowed(self):
+        with open(self.out_filename, 'w') as f:
+            sbf = lldb.SBFile.Create(f, borrow=True)
+            e, n = sbf.Write(b'FOO')
+            self.assertTrue(e.Success())
+            self.assertEqual(n, 3)
+            sbf.Close()
+            self.assertFalse(f.closed)
+            f.write('BAR\n')
+        with open(self.out_filename, 'r') as f:
+            self.assertEqual(f.read().strip(), 'FOOBAR')
+
+
+
+    @add_test_categories(['pyapi'])
+    @skipIf(py_version=['<', (3,)])
+    def test_sbfile_write_forced(self):
+        with open(self.out_filename, 'w') as f:
+            written = MutableBool(False)
+            orig_write = f.write
+            def mywrite(x):
+                written.set(True)
+                return orig_write(x)
+            f.write = mywrite
+            sbf = lldb.SBFile.Create(f, force_io_methods=True)
+            e, n = sbf.Write(b'FOO')
+            self.assertTrue(written)
+            self.assertTrue(e.Success())
+            self.assertEqual(n, 3)
+            sbf.Close()
+            self.assertTrue(f.closed)
+        with open(self.out_filename, 'r') as f:
+            self.assertEqual(f.read().strip(), 'FOO')
+
+
+    @add_test_categories(['pyapi'])
+    @skipIf(py_version=['<', (3,)])
+    def test_sbfile_write_forced_borrowed(self):
+        with open(self.out_filename, 'w') as f:
+            written = MutableBool(False)
+            orig_write = f.write
+            def mywrite(x):
+                written.set(True)
+                return orig_write(x)
+            f.write = mywrite
+            sbf = lldb.SBFile.Create(f, borrow=True, force_io_methods=True)
+            e, n = sbf.Write(b'FOO')
+            self.assertTrue(written)
+            self.assertTrue(e.Success())
+            self.assertEqual(n, 3)
+            sbf.Close()
+            self.assertFalse(f.closed)
+        with open(self.out_filename, 'r') as f:
+            self.assertEqual(f.read().strip(), 'FOO')
+
+
+    @add_test_categories(['pyapi'])
+    @skipIf(py_version=['<', (3,)])
+    def test_sbfile_write_string(self):
+        f = io.StringIO()
+        sbf = lldb.SBFile(f)
+        e, n = sbf.Write(b'FOO')
+        self.assertEqual(f.getvalue().strip(), "FOO")
+        self.assertTrue(e.Success())
+        self.assertEqual(n, 3)
+        sbf.Close()
+        self.assertTrue(f.closed)
+
+    @add_test_categories(['pyapi'])
+    @skipIf(py_version=['<', (3,)])
+    def test_sbfile_write_bytes(self):
+        f = io.BytesIO()
+        sbf = lldb.SBFile(f)
+        e, n = sbf.Write(b'FOO')
+        self.assertEqual(f.getvalue().strip(), b"FOO")
+        self.assertTrue(e.Success())
+        self.assertEqual(n, 3)
+        sbf.Close()
+        self.assertTrue(f.closed)
+
+    @add_test_categories(['pyapi'])
+    @skipIf(py_version=['<', (3,)])
+    def test_sbfile_read_string(self):
+        f = io.StringIO('zork')
+        sbf = lldb.SBFile(f)
+        buf = bytearray(100)
+        e, n = sbf.Read(buf)
+        self.assertTrue(e.Success())
+        self.assertEqual(buf[:n], b'zork')
+
+
+    @add_test_categories(['pyapi'])
+    @skipIf(py_version=['<', (3,)])
+    def test_sbfile_read_string_one_byte(self):
+        f = io.StringIO('z')
+        sbf = lldb.SBFile(f)
+        buf = bytearray(1)
+        e, n = sbf.Read(buf)
+        self.assertTrue(e.Fail())
+        self.assertEqual(n, 0)
+        self.assertEqual(e.GetCString(), "can't read less than 6 bytes from a utf8 text stream")
+
+
+    @add_test_categories(['pyapi'])
+    @skipIf(py_version=['<', (3,)])
+    def test_sbfile_read_bytes(self):
+        f = io.BytesIO(b'zork')
+        sbf = lldb.SBFile(f)
+        buf = bytearray(100)
+        e, n = sbf.Read(buf)
+        self.assertTrue(e.Success())
+        self.assertEqual(buf[:n], b'zork')
+
+
+    @add_test_categories(['pyapi'])
+    @skipIf(py_version=['<', (3,)])
+    def test_sbfile_out(self):
+        with open(self.out_filename, 'w') as f:
+            sbf = lldb.SBFile(f)
+            status = self.debugger.SetOutputFile(sbf)
+            self.assertTrue(status.Success())
+            self.handleCmd('script 2+2')
+        with open(self.out_filename, 'r') as f:
+            self.assertEqual(f.read().strip(), '4')
+
+
+    @add_test_categories(['pyapi'])
+    def test_sbfile_error(self):
+        with open(self.out_filename, 'w') as f:
+            sbf = lldb.SBFile(f)
+            status = self.debugger.SetErrorFile(sbf)
+            self.assertTrue(status.Success())
+            self.handleCmd('lolwut', check=False, collect_result=False)
+        with open(self.out_filename, 'r') as f:
+            errors = f.read()
+            self.assertTrue(re.search(r'error:.*lolwut', errors))
+
+
+    @add_test_categories(['pyapi'])
+    def test_exceptions(self):
+        self.assertRaises(TypeError, lldb.SBFile, None)
+        self.assertRaises(TypeError, lldb.SBFile, "ham sandwich")
+        if sys.version_info[0] < 3:
+            self.assertRaises(TypeError, lldb.SBFile, ReallyBadIO())
+        else:
+            self.assertRaises(OhNoe, lldb.SBFile, ReallyBadIO())
+            error, n = lldb.SBFile(BadIO()).Write(b"FOO")
+            self.assertEqual(n, 0)
+            self.assertTrue(error.Fail())
+            self.assertEqual(error.GetCString(), "OhNoe('OH NOE')")
+            error, n = lldb.SBFile(BadIO()).Read(bytearray(100))
+            self.assertEqual(n, 0)
+            self.assertTrue(error.Fail())
+            self.assertEqual(error.GetCString(), "OhNoe('OH NOE')")
+
+
+    @add_test_categories(['pyapi'])
+    @skipIf(py_version=['<', (3,)])
+    def test_exceptions_logged(self):
+        messages = list()
+        self.debugger.SetLoggingCallback(messages.append)
+        self.handleCmd('log enable lldb script')
+        self.debugger.SetOutputFile(lldb.SBFile(BadIO()))
+        self.handleCmd('script 1+1')
+        self.assertTrue(any('OH NOE' in msg for msg in messages))
+
+
+    @add_test_categories(['pyapi'])
+    @skipIf(py_version=['<', (3,)])
+    def test_flush(self):
+        flushed = MutableBool(False)
+        closed = MutableBool(False)
+        f = FlushTestIO(flushed, closed)
+        self.assertFalse(flushed)
+        self.assertFalse(closed)
+        sbf = lldb.SBFile(f)
+        self.assertFalse(flushed)
+        self.assertFalse(closed)
+        sbf = None
+        self.assertFalse(flushed)
+        self.assertTrue(closed)
+        self.assertTrue(f.closed)
+
+        flushed = MutableBool(False)
+        closed = MutableBool(False)
+        f = FlushTestIO(flushed, closed)
+        self.assertFalse(flushed)
+        self.assertFalse(closed)
+        sbf = lldb.SBFile.Create(f, borrow=True)
+        self.assertFalse(flushed)
+        self.assertFalse(closed)
+        sbf = None
+        self.assertTrue(flushed)
+        self.assertFalse(closed)
+        self.assertFalse(f.closed)
+
+
+    @add_test_categories(['pyapi'])
+    def test_fileno_flush(self):
+        with open(self.out_filename, 'w') as f:
+            f.write("foo")
+            sbf = lldb.SBFile(f)
+            sbf.Write(b'bar')
+            sbf = None
+            self.assertTrue(f.closed)
+        with open(self.out_filename, 'r') as f:
+            self.assertEqual(f.read(), 'foobar')
+
+        with open(self.out_filename, 'w+') as f:
+            f.write("foo")
+            sbf = lldb.SBFile.Create(f, borrow=True)
+            sbf.Write(b'bar')
+            sbf = None
+            self.assertFalse(f.closed)
+            f.seek(0)
+            self.assertEqual(f.read(), 'foobar')

lldb/scripts/Python/python-typemaps.swig

%typemap(in) (lldb::LogOutputCallback log_callback, void *baton) {
   $2 = $input;
 }
 
 %typemap(typecheck) (lldb::LogOutputCallback log_callback, void *baton) {
   $1 = $input == Py_None;
   $1 = $1 || PyCallable_Check(reinterpret_cast<PyObject*>($input));
 }
 
+
+%typemap(in) lldb::FileSP {
+  using namespace lldb_private;
+  PythonFile py_file(PyRefType::Borrowed, $input);
+  if (!py_file) {
+    PyErr_SetString(PyExc_TypeError, "not a file");
+    return nullptr;
+  }
+  auto sp = unwrapOrSetPythonException(py_file.ConvertToFile());
+  if (!sp)
+    return nullptr;
+  $1 = sp;
+}
+
+%typemap(in) lldb::FileSP FORCE_IO_METHODS {
+  using namespace lldb_private;
+  PythonFile py_file(PyRefType::Borrowed, $input);
+  if (!py_file) {
+    PyErr_SetString(PyExc_TypeError, "not a file");
+    return nullptr;
+  }
+  auto sp = unwrapOrSetPythonException(py_file.ConvertToFileForcingUseOfScriptingIOMethods());
+  if (!sp)
+    return nullptr;
+  $1 = sp;
+}
+
+%typemap(in) lldb::FileSP BORROWED {
+ using namespace lldb_private;
+  PythonFile py_file(PyRefType::Borrowed, $input);
+  if (!py_file) {
+    PyErr_SetString(PyExc_TypeError, "not a file");
+    return nullptr;
+  }
+  auto sp = unwrapOrSetPythonException(py_file.ConvertToFile(/*borrowed=*/true));
+  if (!sp)
+    return nullptr;
+  $1 = sp;
+}
+
+%typemap(in) lldb::FileSP BORROWED_FORCE_IO_METHODS {
+  using namespace lldb_private;
+  PythonFile py_file(PyRefType::Borrowed, $input);
+  if (!py_file) {
+    PyErr_SetString(PyExc_TypeError, "not a file");
+    return nullptr;
+  }
+  auto sp = unwrapOrSetPythonException(py_file.ConvertToFileForcingUseOfScriptingIOMethods(/*borrowed=*/true));
+  if (!sp)
+    return nullptr;
+  $1 = sp;
+}
+
+%typecheck(SWIG_TYPECHECK_POINTER) lldb::FileSP {
+  if (lldb_private::PythonFile::Check($input)) {
+    $1 = 1;
+  } else {
+    PyErr_Clear();
+    $1 = 0;
+  }
+}
+
+
 // 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;
    else if (!lldb_private::PythonFile::Check($input)) {

lldb/scripts/interface/SBFile.i

 
 %feature("docstring",
 "Represents a file."
 ) SBFile;
 
 class SBFile
 {
 public:
+
     SBFile();
+
+    %feature("docstring", "
+    Initialize a SBFile from a file descriptor.  mode is
+    'r', 'r+', or 'w', like fdopen.");
     SBFile(int fd, const char *mode, bool transfer_ownership);
 
+    %feature("docstring", "initialize a SBFile from a python file object");
+    SBFile(FileSP file);
+
+    %extend {
+        static lldb::SBFile MakeBorrowed(lldb::FileSP BORROWED) {
+            return lldb::SBFile(BORROWED);
+        }
+        static lldb::SBFile MakeForcingIOMethods(lldb::FileSP FORCE_IO_METHODS) {
+            return lldb::SBFile(FORCE_IO_METHODS);
+        }
+        static lldb::SBFile MakeBorrowedForcingIOMethods(lldb::FileSP BORROWED_FORCE_IO_METHODS) {
+            return lldb::SBFile(BORROWED_FORCE_IO_METHODS);
+        }
+    }
+
+    %pythoncode {
+        @classmethod
+        def Create(cls, file, borrow=False, force_io_methods=False):
+            """
+            Create a SBFile from a python file object, with options.
+
+            If borrow is set then the underlying file will
+            not be closed when the SBFile is closed or destroyed.
+
+            If force_scripting_io is set then the python read/write
+            methods will be called even if a file descriptor is available.
+            """
+            if borrow:
+                if force_io_methods:
+                    return cls.MakeBorrowedForcingIOMethods(file)
+                else:
+                    return cls.MakeBorrowed(file)
+            else:
+                if force_io_methods:
+                    return cls.MakeForcingIOMethods(file)
+                else:
+                    return cls(file)
+    }
+
     ~SBFile ();
 
     %feature("autodoc", "Read(buffer) -> SBError, bytes_read") Read;
     SBError Read(uint8_t *buf, size_t num_bytes, size_t *OUTPUT);
 
     %feature("autodoc", "Write(buffer) -> SBError, written_read") Write;
     SBError Write(const uint8_t *buf, size_t num_bytes, size_t *OUTPUT);
 

lldb/source/API/SBFile.cpp

 #include "lldb/API/SBError.h"
 #include "lldb/Host/File.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 SBFile::~SBFile() {}
 
-SBFile::SBFile(FileSP file_sp) : m_opaque_sp(file_sp) {}
+SBFile::SBFile(FileSP file_sp) : m_opaque_sp(file_sp) {
+  LLDB_RECORD_DUMMY(void, SBfile, SBFile, (FileSP), file_sp);
+}
 
 SBFile::SBFile() { LLDB_RECORD_CONSTRUCTOR_NO_ARGS(SBFile); }
 
 SBFile::SBFile(FILE *file, bool transfer_ownership) {
   m_opaque_sp = std::make_shared<NativeFile>(file, transfer_ownership);
 }
 
 SBFile::SBFile(int fd, const char *mode, bool transfer_owndership) {

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

if (!py_obj)
     return false;
 #if PY_MAJOR_VERSION < 3
   return PyFile_Check(py_obj);
 #else
   // In Python 3, there is no `PyFile_Check`, and in fact PyFile is not even a
   // first-class object type anymore.  `PyFile_FromFd` is just a thin wrapper
   // over `io.open()`, which returns some object derived from `io.IOBase`. As a
   // result, the only way to detect a file in Python 3 is to check whether it
-  // inherits from `io.IOBase`.  Since it is possible for non-files to also
-  // inherit from `io.IOBase`, we additionally verify that it has the `fileno`
-  // attribute, which should guarantee that it is backed by the file system.
-  PythonObject io_module(PyRefType::Owned, PyImport_ImportModule("io"));
-  PythonDictionary io_dict(PyRefType::Borrowed,
-                           PyModule_GetDict(io_module.get()));
-  PythonObject io_base_class = io_dict.GetItemForKey(PythonString("IOBase"));
-
-  PythonObject object_type(PyRefType::Owned, PyObject_Type(py_obj));
-
-  if (1 != PyObject_IsSubclass(object_type.get(), io_base_class.get()))
+  // inherits from `io.IOBase`.
+  auto io_module = PythonModule::Import("io");
+  if (!io_module) {
+    llvm::consumeError(io_module.takeError());
     return false;
-  if (!object_type.HasAttribute("fileno"))
+  }
+  auto iobase = io_module.get().Get("IOBase");
+  if (!iobase) {
+    llvm::consumeError(iobase.takeError());
     return false;
-
-  return true;
+  }
+  int r = PyObject_IsInstance(py_obj, iobase.get().get());
+  if (r < 0) {
+    llvm::consumeError(exception()); // clear the exception and log it.
+    return false;
+  }
+  return !!r;
 #endif
 }
 
 void PythonFile::Reset(PyRefType type, PyObject *py_obj) {
   // Grab the desired reference type so that if we end up rejecting `py_obj` it
   // still gets decremented if necessary.
   PythonObject result(type, py_obj);
 
FileUP PythonFile::GetUnderlyingFile() const {
   auto options = File::GetOptionsFromMode(py_mode.GetString());
   auto file = std::unique_ptr<File>(
       new NativeFile(PyObject_AsFileDescriptor(m_py_obj), options, false));
   if (!file->IsValid())
     return nullptr;
   return file;
 }
 
+namespace {
+class GIL {
+public:
+  GIL() {
+    m_state = PyGILState_Ensure();
+    assert(!PyErr_Occurred());
+  }
+  ~GIL() { PyGILState_Release(m_state); }
+
+protected:
+  PyGILState_STATE m_state;
+};
+} // namespace
+
 const char *PythonException::toCString() const {
   if (!m_repr_bytes)
     return "unknown exception";
   return PyBytes_AS_STRING(m_repr_bytes);
 }
 
 PythonException::PythonException(const char *caller) {
   assert(PyErr_Occurred());
   m_exception_type = m_exception = m_traceback = m_repr_bytes = NULL;
   PyErr_Fetch(&m_exception_type, &m_exception, &m_traceback);
   PyErr_NormalizeException(&m_exception_type, &m_exception, &m_traceback);
   PyErr_Clear();
   if (m_exception) {
     PyObject *repr = PyObject_Repr(m_exception);
     if (repr) {
       m_repr_bytes = PyUnicode_AsEncodedString(repr, "utf-8", nullptr);
       if (!m_repr_bytes) {
         PyErr_Clear();
       }
       Py_XDECREF(repr);
     } else {
       PyErr_Clear();
     }
   }
   Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_SCRIPT);
   if (caller)
     LLDB_LOGF(log, "%s failed with exception: %s", caller, toCString());

 void PythonException::log(llvm::raw_ostream &OS) const { OS << toCString(); }
 
 std::error_code PythonException::convertToErrorCode() const {
   return llvm::inconvertibleErrorCode();
 }
 
 char PythonException::ID = 0;
 
+llvm::Expected<uint32_t> GetOptionsForPyObject(const PythonObject &obj) {
+  uint32_t options = 0;
+#if PY_MAJOR_VERSION >= 3
+  auto readable = As<bool>(obj.CallMethod("readable"));
+  if (!readable)
+    return readable.takeError();
+  auto writable = As<bool>(obj.CallMethod("writable"));
+  if (!writable)
+    return writable.takeError();
+  if (readable.get())
+    options |= File::eOpenOptionRead;
+  if (writable.get())
+    options |= File::eOpenOptionWrite;
+#else
+  PythonString py_mode = obj.GetAttributeValue("mode").AsType<PythonString>();
+  options = File::GetOptionsFromMode(py_mode.GetString());
+#endif
+  return options;
+}
+
+// Base class template for python files.   All it knows how to do
+// is hold a reference to the python object and close or flush it
+// when the File is closed.
+namespace {
+template <typename Base> class OwnedPythonFile : public Base {
+public:
+  template <typename... Args>
+  OwnedPythonFile(const PythonFile &file, bool borrowed, Args... args)
+      : Base(args...), m_py_obj(file), m_borrowed(borrowed) {
+    assert(m_py_obj);
+  }
+
+  ~OwnedPythonFile() override {
+    assert(m_py_obj);
+    GIL takeGIL;
+    Close();
+    m_py_obj.Reset();
+  }
+
+  bool IsPythonSideValid() const {
+    GIL takeGIL;
+    auto closed = As<bool>(m_py_obj.GetAttribute("closed"));
+    if (!closed) {
+      llvm::consumeError(closed.takeError());
+      return false;
+    }
+    return !closed.get();
+  }
+
+  bool IsValid() const override {
+    return IsPythonSideValid() && Base::IsValid();
+  }
+
+  Status Close() override {
+    assert(m_py_obj);
+    Status py_error, base_error;
+    GIL takeGIL;
+    if (!m_borrowed) {
+      auto r = m_py_obj.CallMethod("close");
+      if (!r)
+        py_error = Status(r.takeError());
+    }
+    base_error = Base::Close();
+    if (py_error.Fail())
+      return py_error;
+    return base_error;
+  };
+
+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,
+                   uint32_t options)
+      : OwnedPythonFile(file, borrowed, fd, options, false) {}
+};
+} // namespace
+
+#if PY_MAJOR_VERSION >= 3
+
+namespace {
+class PythonBuffer {
+public:
+  // you must check PyErr_Occurred() after calling this constructor.
+  PythonBuffer(PythonObject &obj, int flags = PyBUF_SIMPLE)
+      : m_buffer({}), m_error(Error::success()) {
+    PyObject_GetBuffer(obj.get(), &m_buffer, flags);
+    if (!m_buffer.obj) {
+      m_error = llvm::make_error<PythonException>();
+    }
+  }
+  operator bool() { return m_buffer.obj != nullptr; }
+  Error takeError() { return std::move(m_error); }
+  ~PythonBuffer() {
+    if (m_buffer.obj) {
+      PyBuffer_Release(&m_buffer);
+    }
+    if (!m_error) {
+      llvm::consumeError(std::move(m_error));
+    }
+  }
+  Py_buffer &get() { return m_buffer; }
+
+protected:
+  Py_buffer m_buffer;
+  Error m_error;
+};
+} // namespace
+
+// Shared methods between TextPythonFile and BinaryPythonFile
+namespace {
+class PythonIOFile : public OwnedPythonFile<File> {
+public:
+  PythonIOFile(const PythonFile &file, bool borrowed)
+      : OwnedPythonFile(file, borrowed) {}
+
+  ~PythonIOFile() override { Close(); }
+
+  bool IsValid() const override { return IsPythonSideValid(); }
+
+  Status Close() override {
+    assert(m_py_obj);
+    GIL takeGIL;
+    if (m_borrowed)
+      return Flush();
+    auto r = m_py_obj.CallMethod("close");
+    if (!r)
+      return Status(r.takeError());
+    return Status();
+  }
+
+  Status Flush() override {
+    GIL takeGIL;
+    auto r = m_py_obj.CallMethod("flush");
+    if (!r)
+      return Status(r.takeError());
+    return Status();
+  }
+
+};
+} // namespace
+
+namespace {
+class BinaryPythonFile : public PythonIOFile {
+protected:
+  int m_descriptor;
+
+public:
+  BinaryPythonFile(int fd, const PythonFile &file, bool borrowed)
+      : PythonIOFile(file, borrowed),
+        m_descriptor(File::DescriptorIsValid(fd) ? fd
+                                                 : File::kInvalidDescriptor) {}
+
+  int GetDescriptor() const override { return m_descriptor; }
+
+  Status Write(const void *buf, size_t &num_bytes) override {
+    GIL takeGIL;
+    PyObject *pybuffer_p = PyMemoryView_FromMemory(
+        const_cast<char *>((const char *)buf), num_bytes, PyBUF_READ);
+    if (!pybuffer_p)
+      return Status(llvm::make_error<PythonException>());
+    auto pybuffer = Take<PythonObject>(pybuffer_p);
+    num_bytes = 0;
+    auto bytes_written = As<long long>(m_py_obj.CallMethod("write", pybuffer));
+    if (!bytes_written)
+      return Status(bytes_written.takeError());
+    if (bytes_written.get() < 0)
+      return Status(".write() method returned a negative number!");
+    static_assert(sizeof(long long) >= sizeof(size_t), "overflow");
+    num_bytes = bytes_written.get();
+    return Status();
+  }
+
+  Status Read(void *buf, size_t &num_bytes) override {
+    GIL takeGIL;
+    static_assert(sizeof(long long) >= sizeof(size_t), "overflow");
+    auto pybuffer_obj =
+        m_py_obj.CallMethod("read", (unsigned long long)num_bytes);
+    if (!pybuffer_obj)
+      return Status(pybuffer_obj.takeError());
+    num_bytes = 0;
+    if (pybuffer_obj.get().IsNone()) {
+      // EOF
+      num_bytes = 0;
+      return Status();
+    }
+    PythonBuffer pybuffer(pybuffer_obj.get());
+    if (!pybuffer)
+      return Status(pybuffer.takeError());
+    memcpy(buf, pybuffer.get().buf, pybuffer.get().len);
+    num_bytes = pybuffer.get().len;
+    return Status();
+  }
+};
+} // namespace
+
+namespace {
+class TextPythonFile : public PythonIOFile {
+protected:
+  int m_descriptor;
+
+public:
+  TextPythonFile(int fd, const PythonFile &file, bool borrowed)
+      : PythonIOFile(file, borrowed),
+        m_descriptor(File::DescriptorIsValid(fd) ? fd
+                                                 : File::kInvalidDescriptor) {}
+
+  int GetDescriptor() const override { return m_descriptor; }
+
+  Status Write(const void *buf, size_t &num_bytes) override {
+    GIL takeGIL;
+    auto pystring =
+        PythonString::FromUTF8(llvm::StringRef((const char *)buf, num_bytes));
+    if (!pystring)
+      return Status(pystring.takeError());
+    num_bytes = 0;
+    auto bytes_written =
+        As<long long>(m_py_obj.CallMethod("write", pystring.get()));
+    if (!bytes_written)
+      return Status(bytes_written.takeError());
+    if (bytes_written.get() < 0)
+      return Status(".write() method returned a negative number!");
+    static_assert(sizeof(long long) >= sizeof(size_t), "overflow");
+    num_bytes = bytes_written.get();
+    return Status();
+  }
+
+  Status Read(void *buf, size_t &num_bytes) override {
+    GIL takeGIL;
+    size_t num_chars = num_bytes / 6;
+    size_t orig_num_bytes = num_bytes;
+    num_bytes = 0;
+    if (orig_num_bytes < 6) {
+      return Status("can't read less than 6 bytes from a utf8 text stream");
+    }
+    auto pystring = As<PythonString>(
+        m_py_obj.CallMethod("read", (unsigned long long)num_chars));
+    if (!pystring)
+      return Status(pystring.takeError());
+    if (pystring.get().IsNone()) {
+      // EOF
+      return Status();
+    }
+    auto stringref = pystring.get().AsUTF8();
+    if (!stringref)
+      return Status(stringref.takeError());
+    num_bytes = stringref.get().size();
+    memcpy(buf, stringref.get().begin(), num_bytes);
+    return Status();
+  }
+};
+} // namespace
+
+#endif
+
+llvm::Expected<FileSP> PythonFile::ConvertToFile(bool borrowed) {
+  if (!IsValid())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "invalid PythonFile");
+
+  int fd = PyObject_AsFileDescriptor(m_py_obj);
+  if (fd < 0) {
+    PyErr_Clear();
+    return ConvertToFileForcingUseOfScriptingIOMethods(borrowed);
+  }
+  auto options = GetOptionsForPyObject(*this);
+  if (!options)
+    return options.takeError();
+
+  // LLDB and python will not share I/O buffers.  We should probably
+  // flush the python buffers now.
+  auto r = CallMethod("flush");
+  if (!r)
+    return r.takeError();
+
+  FileSP file_sp;
+  if (borrowed) {
+    // In this case we we don't need to retain the python
+    // object at all.
+    file_sp = std::make_shared<NativeFile>(fd, options.get(), false);
+  } else {
+    file_sp = std::static_pointer_cast<File>(
+        std::make_shared<SimplePythonFile>(*this, borrowed, fd, options.get()));
+  }
+  if (!file_sp->IsValid())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "invalid File");
+
+  return file_sp;
+}
+
+llvm::Expected<FileSP>
+PythonFile::ConvertToFileForcingUseOfScriptingIOMethods(bool borrowed) {
+
+  assert(!PyErr_Occurred());
+
+  if (!IsValid())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "invalid PythonFile");
+
+#if PY_MAJOR_VERSION < 3
+
+  return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                 "not supported on python 2");
+
+#else
+
+  int fd = PyObject_AsFileDescriptor(m_py_obj);
+  if (fd < 0) {
+    PyErr_Clear();
+    fd = File::kInvalidDescriptor;
+  }
+
+  auto io_module = PythonModule::Import("io");
+  if (!io_module)
+    return io_module.takeError();
+  auto textIOBase = io_module.get().Get("TextIOBase");
+  if (!textIOBase)
+    return textIOBase.takeError();
+  auto rawIOBase = io_module.get().Get("RawIOBase");
+  if (!rawIOBase)
+    return rawIOBase.takeError();
+  auto bufferedIOBase = io_module.get().Get("BufferedIOBase");
+  if (!bufferedIOBase)
+    return bufferedIOBase.takeError();
+
+  FileSP file_sp;
+
+  auto isTextIO = IsInstance(textIOBase.get());
+  if (!isTextIO)
+    return isTextIO.takeError();
+  if (isTextIO.get())
+    file_sp = std::static_pointer_cast<File>(
+        std::make_shared<TextPythonFile>(fd, *this, borrowed));
+
+  auto isRawIO = IsInstance(rawIOBase.get());
+  if (!isRawIO)
+    return isRawIO.takeError();
+  auto isBufferedIO = IsInstance(bufferedIOBase.get());
+  if (!isBufferedIO)
+    return isBufferedIO.takeError();
+
+  if (isRawIO.get() || isBufferedIO.get()) {
+    file_sp = std::static_pointer_cast<File>(
+        std::make_shared<BinaryPythonFile>(fd, *this, borrowed));
+  }
+
+  if (!file_sp)
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "python file is neither text nor binary");
+
+  if (!file_sp->IsValid())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "invalid File");
+
+  return file_sp;
+
+#endif
+}
+
 #endif