[Lldb-commits] [PATCH] D68188: allow arbitrary python streams to be converted to SBFile

[Pavel Labath via Phabricator via lldb-commits]

labath added subscribers: amccarth, zturner.
labath added a comment.

Thanks. I was just about to hit approve, but then I noticed one other thing... :/ It seems that somebody (I guess it was @zturner) spent a lot of time in creating the whole PythonObject hierarchy, and it's (worthwhile) goal seems to be to enable the rest of the to code to avoid dealing with the python APIs directly. However, here you're ignoring all of that, and jumping to the C python interface directly.

I'd like to know what's your take on that? Have you considered basing these file classes on the PythonDataObject stuff?

The main reason I mention that is your comment about PyErr_Occurred, and the resulting boiler plate it produced. This seems like something that would be nicely solved by some c++-like wrapper, which is exactly what the PythonObject stuff is AFAICT.

Most of your interactions seem to be about calling methods. Would it be possible to add a PythonDataObject wrapper for this (and any other frequently used python API)? I'm hoping that we could have something like:

  if (Expected<PythonInteger> i = m_py_obj.CallMethod<PythonInteger>(pybuffer))
    l_bytes_written = i->GetInteger();
  else
    return Status(i.takeError());

instead of stuff like

  auto bytes_written = Take<PythonObject>(
      PyObject_CallMethod(m_py_obj, "write", "(O)", pybuffer.get()));
  if (PyErr_Occurred())
    return Status(llvm::make_error<PythonException>("Write"));
  long l_bytes_written = PyLong_AsLong(bytes_written.get());
  if (PyErr_Occurred())
    return Status(llvm::make_error<PythonException>("Write"));

It doesn't look like adding this should be too hard (including the template-based computation of the python signature), but it could go a long way towards improving the readability, and it might actually fix other issues too -- it seems that the PythonDataObjects currently completely ignore the `PyErr_Occurred` stuff, which seems to be wrong, and it could actually be the reason why @amccarth was unable to run the test suite with a debug python on windows. Improving that, even if it's just for the stuff that's needed for your work would be really helpful, as it would provide a pattern on how to fix the remaining issues.

I'm sorry that I'm bringing this up so late in the review, but I am also learning this stuff as we go along -- unfortunately, I don't think we have anyone really knowledgeable about the python stuff still active in the lldb community..



================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1165
+  bool IsValid() const override {
+    GIL takeGIL;
+    return IsPythonSideValid() && Base::IsValid();
----------------
Maybe this GIL-talking can be done inside `IsPythonSideValid` (instead of two IsValid methods)?


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1197
+                   uint32_t options)
+      : OwnedPythonFile(file, borrowed, fd, options, false){};
+};
----------------
extra semicolon here too


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1369
+    const char *utf8 = PyUnicode_AsUTF8AndSize(pystring.get(), &size);
+    if (!utf8 || PyErr_Occurred())
+      return Status(llvm::make_error<PythonException>("Read"));
----------------
This is an example where _I think_ you may be using `PyErr_Occurred` incorrectly. If python really asserts that PyErr_Occurred is always called, then this is not right, as you will not call it if the result is null. There are a couple of other examples like that in the code too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68188