This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][Python] fix another fflush issue on NetBSD
ClosedPublic

Authored by lawrence_danna on Oct 27 2019, 2:26 PM.

Details

Summary

Here's another instance where we were calling fflush on an input
stream, which is illegal on NetBSD.

Event Timeline

lawrence_danna created this revision.Oct 27 2019, 2:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2019, 2:26 PM
Herald added a subscriber: krytarowski. · View Herald Transcript

py2_const_cast shouldn't be static as a free function

found another one

I'm sorry but I won't be able to test it until later today. Visually, looks good though.

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

For the record, this doesn't really guarantee specific order of destruction. Generally, in Python code you shouldn't rely on destructors being called at all and close files manually. That's why with is commonly used with files.

lawrence_danna marked 2 inline comments as done.Oct 27 2019, 11:09 PM
lawrence_danna added inline comments.
lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py
854

The point is not to rely on the file being closed at a certain time. They point is that each file added to the list is a borrowed reference to the previous one, and we should not allow those references to become dangling by letting the older ones go to zero-references before the younger ones.

Now that I say it, I wonder if it was a bad idea to expose this kind of C++ object lifetime semantics to python programs.
I put in GetFile mainly so that in the case that a SBFile was a proxy of a python file, you could get the python file back.

It might be better to restrict it to that case, and return None instead of a borrowed file in other cases.

On the other hand, there's value in having a consistent API. Under the current semantics this just works:

print("foo",   file=debugger.GetOutputFile().GetFile())

Overall I think I'd lean towards keeping GetFile the way it is, but I can see the argument for restricting it.

lawrence_danna marked an inline comment as done.Oct 27 2019, 11:10 PM
lawrence_danna marked an inline comment as done.Oct 27 2019, 11:16 PM
lawrence_danna added inline comments.
lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py
854

On second second thought, there's a better way. We don't need to expose the borrow semantics, and we don't need to restrict GetFile like that. We can do the same thing File.cpp does when you ask it for a FILE* on a borrowed FD -- that is, use dup and create one that python can truly own.

protect python from being exposed to C++ reference borrowing semantics

There, I think that fixes the issue with borrow semantics and the NetBSD issues.

Thanks but I'd personally prefer splitting this into a future commit, in case it introduced unrelated issues.

I am not thrilled by all of that duping going around. Having multiple FILE objects means that you have multiple independent file caches too. That can cause different kinds of strange behavior if multiple things start reading/writing to the different FILE objects simultaneously. I think I'd rather just keep the existing borrow semantics. I don't think that should be a problem in practice -- it's just that this test is weird because is testing the extreme cases of this behavior (which is fine). Normally you'll just use one level of wrapping and so the underlying file will be naturally kept around, since lldb will still be holding onto it.

I am not thrilled by all of that duping going around. Having multiple FILE objects means that you have multiple independent file caches too. That can cause different kinds of strange behavior if multiple things start reading/writing to the different FILE objects simultaneously. I think I'd rather just keep the existing borrow semantics. I don't think that should be a problem in practice -- it's just that this test is weird because is testing the extreme cases of this behavior (which is fine). Normally you'll just use one level of wrapping and so the underlying file will be naturally kept around, since lldb will still be holding onto it.

I'm not exactly thrilled with it but I will still argue it is the right thing to do. If you look at the docstring for GetFile, it doesn't mention this problem. It says you can't use a borrowed file if it becomes dangling, but it doesn't say you have to delete your references to it before it can dangle. That's because this problem is surprising enough that neither of us thought of it.

Consider this situation: The python programmer makes an object that keeps a reference to debugger and also to debugger.GetOutputFile().GetFile(). When they're done with the object they destroy the debugger and let their object go out of scope.

In python3, they may get an IOError at this point, as the saved output file tries to flush its buffer to a closed file. In python 2 they are exposed to to a use-after free violation and the program could crash!

It is very difficult to write python programs that deal with these kind of semantics correctly, and nothing about this API suggests that it is as dangerous to use as that. I think the correctness and ergonomics concerns outweigh the performance concerns here. Particularly because this issue only affects python 2. Is it really a bigger deal that we generate some extra dups on an end-of-life scripting language than that we potentially crash on that language?

I have confirmed that your previous revision fixed the problem in question. The newer one would probably require full test suite run which I can't do right now. As I said, I would prefer those two changes to be committed separately, preferably with a few hours delay so that it would be clear if it causes any breakage on buildbot.

I have confirmed that your previous revision fixed the problem in question. The newer one would probably require full test suite run which I can't do right now. As I said, I would prefer those two changes to be committed separately, preferably with a few hours delay so that it would be clear if it causes any breakage on buildbot.

which commit id did you test?

revert to previous version

Diff 4, I think. Lemme do a quick test with 5. Hopefully it won't request me to rebuild everything again.

mgorny accepted this revision.Oct 28 2019, 9:45 PM

WFM. Thanks!

This revision is now accepted and ready to land.Oct 28 2019, 9:45 PM
labath accepted this revision.Oct 29 2019, 12:50 AM

Ok, let's get this thing fixed. We can continue the discussion on the other patch.

This revision was automatically updated to reflect the committed changes.