This is an archive of the discontinued LLVM Phabricator instance.

SBFile::GetFile: convert SBFile back into python native files.
ClosedPublic

Authored by lawrence_danna on Oct 9 2019, 3:01 PM.

Details

Summary

This makes SBFile::GetFile public and adds a SWIG typemap to convert
the result back into a python native file.

If the underlying File itself came from a python file, it is returned
identically. Otherwise a new python file object is created using
the file descriptor.

Event Timeline

lawrence_danna created this revision.Oct 9 2019, 3:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2019, 3:01 PM

It looks pretty straight-forward. The two main comments I have are:

  • implement rtti to get rid of the GetPythonObject method on the base class
  • it looks like the implementations of new apis you're adding (GetOptions, mainly) have to go out of their way to ignore errors, only for their callers to synthesize new Error objects. It seems like it would be better to just use Expecteds throughout.
lldb/include/lldb/Host/File.h
62

change the argument type to OpenOptions. For some reason, lldb has historically used integer types for passing enumerations around, but we're now slowly changing that..

323

We've spend a lot of time removing python from the lowest layers, and this let's it back in through the back door. It would be way better to add support for llvm-style rtti to this class, so that the python code can dyn_cast to a PythonFile when it needs to (re)construct the python object. You can take a look at CPPLanguageRuntime::classof (and friend) for how to implement this in a plugin-friendly manner.

335

How about returning Expected<OpenOptions> from here?

337

and Expected<const char*> from here?

lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py
775

So, if I understand correctly, these tests check that you can feed a file opened by python into lldb, and then pump it back out to python. Are there any tests which do the opposite (have lldb open a file, move it to python and then reinject it into lldb)?

lldb/scripts/interface/SBFile.i
81

I am somewhat surprised that there's no ownership handling in this patch (whereas ownership transfer was a fairly big chunk of the conversion in the other direction). Could you maybe explain what are the ownership expectations of the file returned through here (and what happens when we close it for instance)?

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
674–682

It looks like there's just one caller of this constructor (the "out" typemap for FILE*). Can we just inline this stuff there and delete this constructor?

lawrence_danna marked 3 inline comments as done.Oct 10 2019, 12:41 PM
lawrence_danna added inline comments.
lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py
775

Yea, foo is bar in python is essentially a pointer comparison between the PyObject* pointers, so this is testing that you get the same identical file object back in the situations where you should.

There's no test going the other way, because going the other way isn't implemented. I didn't write anything that could stash an arbitrary lldb_private::File in a python object . If you convert a non-python File, you will get a new python file based on the descriptor, if it's available, or the conversion will fail if one is not. We do test that here, on line 801, we're testing that a NativeFile is converted to a working python file and the file descriptors match.

We could, in a follow-on patch make the other direction work with identity too, but right now I can't think of what it would be useful for.

lldb/scripts/interface/SBFile.i
81

Oh, yea good point. GetFile returns a borrowed file, it shouldn't be closed. I'll add a comment explaining.

lawrence_danna marked an inline comment as done.Oct 10 2019, 12:43 PM
lawrence_danna marked an inline comment as done.Oct 10 2019, 12:46 PM
lawrence_danna added inline comments.
lldb/include/lldb/Host/File.h
323

Ah, thanks. I orginally wrote this using dyn_cast but I found myself putting classnames of python things in an enum in File.h, so I thought "what's the point?" -- and I converted it to just using the virtual method. The static char ID trick is nice.

lawrence_danna marked an inline comment as done.Oct 10 2019, 3:03 PM
lawrence_danna added inline comments.
lldb/include/lldb/Host/File.h
62

Are you sure that's the right thing to do? eOpenOptionRead | eOpenOptionWrite has type unsigned int, not OpenOptions.

lawrence_danna marked 7 inline comments as done.

review fixes

I split off the OpenOptions enum stuff into a separate patch, because changing it from uint32_t to the enum is kind of viral

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
674–682

It's also called in some parts of ScriptInterpreterPython that are broken for other reasons, and are getting fixed later in my patch queue. I'll just put a fixme here and delete it when all the other code that uses it also gets deleted, ok?

labath added inline comments.Oct 11 2019, 1:38 AM
lldb/include/lldb/Host/File.h
62

Ah, right, that was the reason... Regardless, I do thing that's right -- I'll continue the discussion on the other patch.

331–335

Please move this somewhere else -- somewhere near the top or bottom of the class maybe.. It does not make sense to have it in the middle of the open options stuff.

lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py
775

Right, sorry, that came out a bit wrong. While I think it would be cool to have the other direction be an "identity" too, I don't think that is really necessary. Nevertheless, taking a File out of lldb and then back in will do _something_ right now, and that "something" could probably use a test. I suppose you could construct an SBFile from a path, convert it to a python file and back, and then ensure that reading/writing on those two SBFiles does something reasonable.

lldb/scripts/interface/SBFile.i
81

Cool, thanks. This description is very helpful.

93

s/descirptor/descriptor

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
1553–1554

if (auto *simple = ...)

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
674–682

Sounds good.

lawrence_danna marked 8 inline comments as done.

review fixes

lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py
775

I was gonna say that this test already covers it, but then I thought, "oh whatever I'll just write another test". And the test I wrote uncovered a bug 😐.

@labath, how's this look now?

Looks good, just a couple of quick questions

lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py
775

:)

780

I find this comment confusing. Does that refer to the write call above? If so, I don't see how that is relevant here..

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
1574–1577

What would you say to passing fflush here directly? The signature is compatible with fclose, and fclose is documented to return (a superset of) the errors also reported by fflush. So, it seems like it might be useful to report flushing errors instead of swallowing them. Since accessing the FILE* after a fclose call (even if it fails) is UB, and the python code thinks it's calling fclose, it shouldn't mess up the FILE state in any way even if the flushing fails..

lawrence_danna marked 3 inline comments as done.Oct 15 2019, 12:44 AM
lawrence_danna added inline comments.
lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py
780

When we check the output on the next line we have to strip out the 7, which is the number of bytes written.

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
1574–1577

Oh yea good point

lawrence_danna marked an inline comment as done.

pass ::flush to PyFile_FromFile

lawrence_danna marked 2 inline comments as done.Oct 15 2019, 5:33 AM
labath accepted this revision.Oct 15 2019, 7:35 AM
labath added inline comments.
lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py
780

Ah, right. Thanks for clearing that up.

This revision is now accepted and ready to land.Oct 15 2019, 7:35 AM
This revision was automatically updated to reflect the committed changes.
mgorny added a subscriber: mgorny.Oct 25 2019, 11:29 PM

Well, I found another problem with these series, specifically starting at this commit. Any use of script in LLDB ends up with:

(lldb) script True
True
close failed in file object destructor:
IOError: [Errno 9] Bad file descriptor

I'd suspect the same fd is using in more than one file but that's only a guess.

The curious part is that I've tried ktruss-ing the process and I don't see any failed close() syscalls.

Well, I found another problem with these series, specifically starting at this commit. Any use of script in LLDB ends up with:

(lldb) script True
True
close failed in file object destructor:
IOError: [Errno 9] Bad file descriptor

I'd suspect the same fd is using in more than one file but that's only a guess.

The curious part is that I've tried ktruss-ing the process and I don't see any failed close() syscalls.

I just tested it on commit ef7a154d17f2e38ba3c8bfa33f240b60464e4cc7 (from Thurday), on 8.1_STABLE, And I don't see that happening.

netbsd# ./bin/lldb
(lldb) script True
True
(lldb) exit

Hmm, that's weird. It happened on the buildbot which is running some recent 8.x kernel, and on my testing machine with 9.x.

Which Python version are you using? I've just hacked Python 2.7 a bit and found that the failing close() is actually fclose() on FILE* object with fd=0. I can reproduce EBADF only when the same fd is closed twice, so I suspect that something else is closing stdin first, then some extra Python file created for stdin is being closed.

I've just tried on Linux and it seems that Python closes the same descriptors. So somehow closing stdin works here on Linux, and fails on NetBSD. Curious enough, with a simple test I can verify that both NetBSD and Linux return EBADF when trying to close stdin twice. So unless there's another reason EBADF is returned here, it seems that stdin is closed somewhere earlier on NetBSD, and the same thing doesn't happen on Linux.

However, I think this is only a symptom of a more generic problem. I don't think we should really be creating a second system of file objects outside Python, and injecting it into Python file objects like this. I have a bad feeling for e.g. creating low-level a new file object for stdin, and then having Python destroy it alongside its own stdin object.

However, I think this is only a symptom of a more generic problem. I don't think we should really be creating a second system of file objects outside Python, and injecting it into Python file objects like this. I have a bad feeling for e.g. creating low-level a new file object for stdin, and then having Python destroy it alongside its own stdin object.

What do you mean "a second system of file objects outside Python, and injecting it into Python file objects"? That's not what it does. We have lldb_private::File proxies for python files, not the other way around.

Which Python version are you using?

3.7. I'll try it with 2.7.

However, I think this is only a symptom of a more generic problem. I don't think we should really be creating a second system of file objects outside Python, and injecting it into Python file objects like this. I have a bad feeling for e.g. creating low-level a new file object for stdin, and then having Python destroy it alongside its own stdin object.

What do you mean "a second system of file objects outside Python, and injecting it into Python file objects"? That's not what it does. We have lldb_private::File proxies for python files, not the other way around.

Hmm, I guess I've been reading that code wrong. I guess I'm out of clues then.

However, I think this is only a symptom of a more generic problem. I don't think we should really be creating a second system of file objects outside Python, and injecting it into Python file objects like this. I have a bad feeling for e.g. creating low-level a new file object for stdin, and then having Python destroy it alongside its own stdin object.

What do you mean "a second system of file objects outside Python, and injecting it into Python file objects"? That's not what it does. We have lldb_private::File proxies for python files, not the other way around.

Hmm, I guess I've been reading that code wrong. I guess I'm out of clues then.

I can reproduce it with python 2.7. I'll post a fix.

Great, thanks! So it's not just my computer going crazy after all ;-).