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

[Pavel Labath via Phabricator via lldb-commits]

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


Repository:
  rL LLVM

https://reviews.llvm.org/D38829