[Lldb-commits] [PATCH] D68737: SBFile::GetFile: convert SBFile back into python native files.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Looks good, just a couple of quick questions



================
Comment at: lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py:776
     @add_test_categories(['pyapi'])
-    @expectedFailure # FIXME implement SBFile::GetFile
     @skipIf(py_version=['<', (3,)])
----------------
lawrence_danna wrote:
> labath wrote:
> > lawrence_danna wrote:
> > > labath wrote:
> > > > 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)?
> > > 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.
> > 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.
> 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 😐.
:)


================
Comment at: lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py:781
+        with open(self.out_filename, 'r') as f:
+            # python2 returns None from write, python3 returns 7
+            lines = [x for x in f.read().strip().split() if x != "7"]
----------------
I find this comment confusing. Does that refer to the write call above? If so, I don't see how that is relevant here..


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1576-1579
+  // Borrow the FILE*, the lldb_private::File still owns it
+  auto close = [](FILE *f) -> int { fflush(f); return 0; };
+  file_obj =
+      PyFile_FromFile(file.GetStream(), const_cast<char *>(""), cmode, close);
----------------
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..


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68737/new/

https://reviews.llvm.org/D68737