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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/include/lldb/Host/File.h:56
   static bool DescriptorIsValid(int descriptor) { return descriptor >= 0; };
+  static const char *GetStreamOpenModeFromOptions(uint32_t options);
 
----------------
lawrence_danna wrote:
> labath wrote:
> > 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..
> Are you sure that's the right thing to do?   `eOpenOptionRead | eOpenOptionWrite` has type `unsigned int`, not `OpenOptions`.
Ah, right, that was the reason... Regardless, I do thing that's right -- I'll continue the discussion on the other patch.


================
Comment at: lldb/include/lldb/Host/File.h:331-335
+  static char ID;
+
+  virtual bool isA(const void *classID) const { return classID == &ID; }
+
+  static bool classof(const File *file) { return file->isA(&ID); }
----------------
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.


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


================
Comment at: lldb/scripts/interface/SBFile.i:81
+
+    %feature("docstring", "convert this SBFile into a python io.IOBase file object");
+    FileSP GetFile();
----------------
lawrence_danna wrote:
> labath wrote:
> > 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)?
> Oh, yea good point.    `GetFile` returns a borrowed file, it shouldn't be closed.   I'll add a comment explaining.
Cool, thanks. This description is very helpful.


================
Comment at: lldb/scripts/interface/SBFile.i:93
+    If there is no underlying python file to unwrap, GetFile will
+    use the file descirptor, if availble to create a new python
+    file object using `open(fd, mode=..., closefd=False)`
----------------
s/descirptor/descriptor


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1553-1554
+
+  auto *simple = llvm::dyn_cast<SimplePythonFile>(&file);
+  if (simple)
+    return Retain<PythonFile>(simple->GetPythonObject());
----------------
if (auto *simple = ...)


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:674-682
+  PythonFile(File &file, const char *mode = nullptr) {
+    auto f = FromFile(file, mode);
+    if (f)
+      *this = std::move(f.get());
+    else {
+      Reset();
+      llvm::consumeError(f.takeError());
----------------
lawrence_danna wrote:
> labath wrote:
> > 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?
> 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?
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68737