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;
----------------
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?
Repository:
rL LLVM
https://reviews.llvm.org/D38829
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
