[Lldb-commits] [PATCH] D38829: Python: SetOutputFileHandle doesn't work with IOBase

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1075-1096
+static int readfn(void *ctx, char *buffer, int n)
+{
+  auto state = PyGILState_Ensure();
+  auto *file = (PyObject *) ctx;
+  int result = -1;
+  auto pybuffer = PyBuffer_FromMemory(buffer, n);
+  PyObject *pyresult = NULL;
----------------
lawrence_danna wrote:
> labath wrote:
> > zturner wrote:
> > > zturner wrote:
> > > > labath wrote:
> > > > > lawrence_danna wrote:
> > > > > > zturner wrote:
> > > > > > > I am still pretty unhappy about these functions, and passing function pointers into the `File` class.
> > > > > > > 
> > > > > > > I think another approach would be this:
> > > > > > > 
> > > > > > > 1) Make the `File` class contain a member `std::unique_ptr<IOObject> LowLevelIo;`
> > > > > > > 
> > > > > > > 2) In `File.cpp`, define something called `class DefaultLowLevelIo : public IOObject` that implements the virtual methods against an fd.
> > > > > > > 
> > > > > > > 3) In `PythonDataObjects`, define `PythonFileIo : public IOObject` and implement the virtual methods against a `PyObject`.
> > > > > > > 
> > > > > > > 4) Add an additional constructor to `File` which takes a `std::unique_ptr<IOObject> LowLevelIo`, which we can use when creating one of these from a python file.
> > > > > > > 
> > > > > > > One advantage of this method is that it allows the `PythonFileIo` class to be easily tested.
> > > > > > > 
> > > > > > > (Also, sorry for not getting back to reviewing this several weeks ago)
> > > > > > I don't see how this approach gets around the problem that the interfaces in SBDebugger use FILE *, not IOObject 
> > > > > > 
> > > > > > The only way I can see to do it the way you are saying is to also add a SBIOObject, with swig wrappers to that, and variants of the SBDebugger  
> > > > > > interfaces that take IOObject instead of FILE *
> > > > > > 
> > > > > > Do you want to do it that way?
> > > > > What's the final use case here. In the patch itself I don't see anything that would necessitate a FILE * conversion, but I don't know what do you actually intend to use this for. We can always return a null FILE * if the File object is backed by a a python file (we do the same for file descriptors, as there is no way to convert those into FILE*, not without going the fopencookie way).
> > > > Alright, I re-read this more closely.  This is actually something I wanted to fix for a long time.    Specifically, I don't believe LLDB should be using `FILE*` anywhere.  I would prefer to mark this method dangerous in big letters in the SB API, and add new versions that take an fd.  A `FILE*` doesn't even mean anything in Python.  It's specifically a native construct.  
> > > > 
> > > > I see it being used in two places currently.   1) Setting to `None`, and 2) setting to the result of `open("/dev/null")`.  The open method documentation says "Open a file, returning an object of the file type described in section File Objects".
> > > > 
> > > > So when this method is being called from Python, it is not even a real `FILE*`, it's a pointer to some python object.  I think this is just a bug in the design of the SB API, and we should fix it there.
> > > > 
> > > > I don't often propose adding new SB APIs, but I think we need an entirely different API here.  There should be methods:
> > > > 
> > > > ```
> > > > SetOutputFileHandle(SBFile F);
> > > > SetInputFileHandle(SBFile F);
> > > > SetErrorFileHandle(SBFile F);
> > > > ```
> > > > 
> > > > And we should be passing those.  This will in turn necessitate a lot of trickle down changes in the native side too.  We can mark the older functions deprecated to indicate that people should no longer be using them.
> > > > 
> > > > Thoughts?
> > > Sorry, s/add a new version that takes an fd/add a new version that takes an SBFile/
> > >I don't often propose adding new SB APIs, but I think we need an entirely different API here. There should be methods:
> > 
> > >SetOutputFileHandle(SBFile F);
> > >SetInputFileHandle(SBFile F);
> > >SetErrorFileHandle(SBFile F);
> > >And we should be passing those. This will in turn necessitate a lot of trickle down changes in the native side too. We can mark the older functions deprecated to indicate that people should no longer be using them.
> > 
> > Note that these file handles eventually trickle down into libedit, which expects to see a FILE *. These could probably be synthesized(*) for libedit's usage alone, and leave the rest of the world oblivious to FILE*, but it will need to be done very carefully.
> > 
> > (*) Windows does not have a fopencookie/funopen equivalent afaik, but then again, it also does not have libedit...
> > What's the final use case here
> 
> LLDB integration with iPython.   I want to be able to redirect LLDB's output to a python callback so I can interact with LLDB from inside an iPython notebook.
> 
> > but I think we need an entirely different API here
> 
> OK I will prepare a new version of the patch that introduces a SBFile API
Thanks, and apologies that the scope is kind of expanding here.

If possible, can you try to do this incrementally in several patches?

For example, a first pass you might just try having `SBFile` that wraps a `lldb_private::File` and provides a couple of simple methods.  This will allow us to focus on the API bridge without having to worry about implementation details.

Next add `SetOutputFile` etc, and try to update existing uses of `SetOutputFileHandle` etc to use this new api.  It should continue to work as today, with the same bugs.

Then, implement the dynamic dispatch inside of `lldb_private::File` so that python files and native files go to a different underlying implementation.


Repository:
  rL LLVM

https://reviews.llvm.org/D38829