lawrence_danna 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:
> 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
Repository:
rL LLVM
https://reviews.llvm.org/D38829
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
