This is an archive of the discontinued LLVM Phabricator instance.

Python: SetOutputFileHandle doesn't work with IOBase
AbandonedPublic

Authored by lawrence_danna on Oct 11 2017, 4:13 PM.

Details

Summary

SetOutputFileHandle only works with legit file objects with file descriptors. It ought to work with subclasses of IOBase as well.

This patch adds support for passing an arbitrary python stream (anything inheriting from IOBase) to SetOutputFileHandle or SetErrorFileHandle.

It also fixes some crashes associated with PythonFile.

Supposedly it also works for SetInputFileHandle, but as far as I can tell nothing actually uses the input file handle so I coluldn’t figure out how to test that part at all.

Diff Detail

Repository
rL LLVM

Event Timeline

lawrence_danna created this revision.Oct 11 2017, 4:13 PM
krytarowski added inline comments.Oct 11 2017, 4:21 PM
lldb/source/Host/common/File.cpp
103 ↗(On Diff #118717)

I propose __linux__ for consistency.

code review fix

oops, that last diff I added replaced the whole diff instead of stacking on top.

jasonmolenda accepted this revision.Oct 16 2017, 2:07 PM

Looks good Larry, I'll commit it later tonight.

This revision is now accepted and ready to land.Oct 16 2017, 2:07 PM
This revision was automatically updated to reflect the committed changes.
labath reopened this revision.Nov 2 2017, 3:40 AM
labath added subscribers: lldb-commits, labath.

(Please add lldb-commits to future reviews, so that people are aware of what's going on.)

So, the reason why this failed to compile is that android does not have the fopencookie function (neither does windows as far as I am aware). However, looking at the change, it's not clear to me whether we actually need the fopencookie functionality to implement this. Couldn't we just change the File::Read/Write functions to call these directly. Right now they do if (am_i_backed_by_FILE) fwrite() else write(). We could add a option for them to be backed by a set of callbacks. An even better option would be to just use virtual functions for this (that's the c++ way of doing cookie callbacks). I think that's what the IOObject hierarchy (which File is a part of) was meant to be used for, although I am not sure it will work out of the box for this case.

And it would be great to see some tests for this.

lldb/trunk/include/lldb/Host/File.h
65 ↗(On Diff #119244)

Why are you changing the File to be movable? I don't see the connection between this and the fopencookie part.

lldb/trunk/source/Host/common/File.cpp
124 ↗(On Diff #119244)

cast seems wrong

140 ↗(On Diff #119244)

same here

This revision is now accepted and ready to land.Nov 2 2017, 3:40 AM
zturner added inline comments.
lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
1075–1096 ↗(On Diff #119244)

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;
  1. In File.cpp, define something called class DefaultLowLevelIo : public IOObject that implements the virtual methods against an fd.
  1. In PythonDataObjects, define PythonFileIo : public IOObject and implement the virtual methods against a PyObject.
  1. 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)

zturner requested changes to this revision.Nov 2 2017, 10:42 AM
This revision now requires changes to proceed.Nov 2 2017, 10:42 AM
lawrence_danna added inline comments.Nov 2 2017, 10:50 AM
lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
1075–1096 ↗(On Diff #119244)

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?

labath added inline comments.Nov 2 2017, 10:59 AM
lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
1075–1096 ↗(On Diff #119244)

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).

zturner added inline comments.Nov 2 2017, 11:15 AM
lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
1075–1096 ↗(On Diff #119244)

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?

1075–1096 ↗(On Diff #119244)

Sorry, s/add a new version that takes an fd/add a new version that takes an SBFile/

labath added inline comments.Nov 2 2017, 11:28 AM
lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
1075–1096 ↗(On Diff #119244)

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...

Another option would be to add a URL version of these functions:

class SBDebugger {
  void SetInputURL(const char *url);
  void SetOutputURL(const char *url);
  void SetErrorURL(const char *url);
};

Then we allow this to trickle down to the IOBase in that way. This allows us to innovate by adding new support for new URLs.

"FILE*" is unfortunately what we will need for libedit for now, but we can probably make sure the IOBase call can provide this somehow, or convert a file descriptor into one, and fall back on a simple non-libedit based command interpreter if not. The current IOHandler stack will check the FILE's abilities and do the right thing already.

Ok, I wasn't aware of the libedit problem.

I still don't like this approach, because it causes the design of the File class to be influenced by a limitation of a 3rd party library.

What about adding a method to IOObject called FILE *GetFileStream(). Then the method I proposed earlier would just involve PythonFileIo implementing this in the proper way. Then we can pass SBFile or lldb_private::File through all layers of the codebase, and once we're in libedit we just call file.GetFileStream() and pass it to libedit?

@labath

(Please add lldb-commits to future reviews, so that people are aware of what's going on.)

ok

Couldn't we just change the File::Read/Write functions to call these directly

Like I said to @zturner , this is possible, but it can't work with the existing APIs in SBDebugger.h. We'd need to add new APIs that take SBIOBase instead of FILE *. Do you agree with that plan?

And it would be great to see some tests for this

yup, there's tests in TestFileHandle.py

lldb/trunk/include/lldb/Host/File.h
65 ↗(On Diff #119244)

I use the move constructor here:

file = File(m_py_obj, readable ? readfn : NULL, writable ? writefn : NULL, closefn);

Ok, I wasn't aware of the libedit problem.

I still don't like this approach, because it causes the design of the File class to be influenced by a limitation of a 3rd party library.

What about adding a method to IOObject called FILE *GetFileStream(). Then the method I proposed earlier would just involve PythonFileIo implementing this in the proper way. Then we can pass SBFile or lldb_private::File through all layers of the codebase, and once we're in libedit we just call file.GetFileStream() and pass it to libedit?

That could work. If we don't get a "FILE *" back from IOObject::GetFileStream() we need to fall back onto a simple implementation. The current IOHandler will fall back to fgets() on a FILE*, but we can easily make it just call a method on IOObject. Probably would be best to add a fgets() type call to IOObject so the underlying implementation can do this as efficiently as possible.

I mainly care that the current API continues to work with "FILE *" at least on all Unix based platforms. We can add new APIs, but we must leave the existing ones there.

Ok, I wasn't aware of the libedit problem.

I still don't like this approach, because it causes the design of the File class to be influenced by a limitation of a 3rd party library.

What about adding a method to IOObject called FILE *GetFileStream(). Then the method I proposed earlier would just involve PythonFileIo implementing this in the proper way. Then we can pass SBFile or lldb_private::File through all layers of the codebase, and once we're in libedit we just call file.GetFileStream() and pass it to libedit?

That's how I'd do it.

(*) libedit needs both a FILE* and a raw file descriptor in this case (I think it's calling select, and some terminal functions).

lawrence_danna added inline comments.Nov 2 2017, 12:08 PM
lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
1075–1096 ↗(On Diff #119244)

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

@labath

Couldn't we just change the File::Read/Write functions to call these directly

Like I said to @zturner , this is possible, but it can't work with the existing APIs in SBDebugger.h.

Ok, I am starting to understand what you want to do here.

And it would be great to see some tests for this

yup, there's tests in TestFileHandle.py

These may prove that you don't break existing functionality, but here you are introducing new one, which should be tested as well. (But don't go writing these yet until we figure out how exactly to implement this)

zturner added inline comments.Nov 2 2017, 1:17 PM
lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
1075–1096 ↗(On Diff #119244)

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.

lawrence_danna abandoned this revision.Sep 19 2019, 9:17 PM

I've reworked this -- finally -- as zturner suggested. I'll be posting it as a bunch of individual commits, so i'm abandoning this one.

Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2019, 9:17 PM