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

[lawrence_danna]

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.

[labath]

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

65

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

326

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.

338

How about returning Expected<OpenOptions> from here?

340

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]

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]

lldb/include/lldb/Host/File.h

326

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]

lldb/include/lldb/Host/File.h

65

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

[lawrence_danna]

review fixes

[lawrence_danna]

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]

lldb/include/lldb/Host/File.h

65

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

334–338

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

1555–1556

if (auto *simple = ...)

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h

674–682

Sounds good.

[lawrence_danna]

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

[lawrence_danna]

rebased

[lawrence_danna]

@labath, how's this look now?

[labath]

Looks good, just a couple of quick questions

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

775

:)

781

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

1576–1579

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]

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

781

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

1576–1579

Oh yea good point

[lawrence_danna]

pass ::flush to PyFile_FromFile

[labath]

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

781

Ah, right. Thanks for clearing that up.

[mgorny]

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.

[lawrence_danna]

In D68737#1722181, @mgorny wrote:

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

[mgorny]

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.

[mgorny]

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.

[mgorny]

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.

[lawrence_danna]

In D68737#1722255, @mgorny wrote:

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.

[lawrence_danna]

In D68737#1722556, @mgorny wrote:

Which Python version are you using?

3.7. I'll try it with 2.7.

[mgorny]

In D68737#1722575, @lawrence_danna wrote:

In D68737#1722255, @mgorny wrote:

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.

[lawrence_danna]

In D68737#1722620, @mgorny wrote:

In D68737#1722575, @lawrence_danna wrote:

In D68737#1722255, @mgorny wrote:

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.

[mgorny]

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