This is an archive of the discontinued LLVM Phabricator instance.

Summary: SetOutputFileHandle doesn't work with IOBase
ClosedPublic

Authored by lawrence_danna on Oct 20 2017, 10:41 AM.

Event Timeline

Mentioned earlier, but if at all possible I think it would be helpful to instead of trying to force this into the existing Host/File class, we should instead try to keep all of this in the PythonFile class. This seems like trying to drive a square nail into a round hole. Could PythonFile just inherit from IOObjectBase so that we can use it everywhere we would currently use a File?

source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
964–966

How about just

if (PyFile_Check(py_obj))
  return true;
1041

This all seems horribly complicated. Would it be possible to just make PythonFile inherit directly from IOObjectBase and then just provide an implementation of read, write, and close?

1109

:-/

1166–1184

Can we replace this entire block with:

if (!PythonFile::Check(m_py_obj))
  return false;

#if PY_MAJOR_VERSION < 3
  int fd = PyObject_AsFileDescriptor(m_py_obj);
  assert(fd >= 0);  // assert, since PythonFile::Check() returned true.
    // We don't own the file descriptor returned by this function, make sure the
    // File object knows about that.
    file.SetDescriptor(PyObject_AsFileDescriptor(m_py_obj), false);
    PythonString py_mode = GetAttributeValue("mode").AsType<PythonString>();
    file.SetOptions(PythonFile::GetOptionsFromMode(py_mode.GetString()));
    return file.IsValid();
#else
  PyObject *r = PyEval_CallMethod(m_py_obj, "readable", "()");
  bool readable = PyObject_IsTrue(r);

  r = PyEval_CallMethod(m_py_obj, "writable", "()");
  bool writable = PyObject_IsTrue(r);

  // Note that if this function returned a std::unique_ptr<IOObjectBase>, then we
  // wouldn't need all these weird read, write, and close functions, because it could
  // just implement them virtually and return an instance of PyNativeFile or whatever.
  Py_XINCREF(m_py_obj);
  file = File(m_py_obj, readable ? readfn : NULL, writable ? writefn : NULL, closefn);
  if (!file.IsValid())
    closefn(m_py_obj);
  return file.isValid();
#endif
1189

Isn't the PyEval_CallMethod function undocumented? Can we use the documented version?

lawrence_danna added inline comments.Oct 20 2017, 2:30 PM
source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
1041

I don't know, what is IOObjectBase? I don't see a class called that in either LLDB or python.

1166–1184

I suppose we could skip the subclass check. What's your reasoning for why we should though? I don't understand why that would be better. Seems like we should be checking something to see if it's a valid stream rather than just start calling functions on it right away.

We shouldn't skip the PyObject_AsFileDescriptor() part on python 3 though. If it's an ordinary file. handle then we should just write to it directly rather than going though funopen/fopencookie.

especially because there's no funopen/fopencookie equivalent on windows so only the file descriptor path can work on windows.

1189

It's marked with PyAPI_FUNC, so it's a public function. I think it's OK.

What's the name of the documented version that you're referring to?

zturner added inline comments.Oct 20 2017, 2:42 PM
source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
1041

Sorry, I meant IOObject. It's the base class of File, So instead of passing the File class pointers to functions that read, write and close, you just inherit PythonFile from IOObject and implement Read, Write, and Close as overridden virtual methods.

1166–1184

It's actually not skipping the sanity check though. That's the reason for calling PythonFile::Check() first, which contains all of that logic. This function contained code that appeared to have been copied out of that function, so i was hoping for a way to avoid code duplication.

1189

I can't find documentation for it anywhere on the python.org website. In other parts of LLDB where we want to call functions through the C API, we have been using PyObject_CallFunction or PyObject_CallMethod. Is there a functional difference that you know of?

lawrence_danna planned changes to this revision.Oct 20 2017, 3:09 PM

I'll update this.

source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
1041

I'll try it and see if it works.

1166–1184

ah, yea good idea.

1189

ah, ok

lawrence_danna added inline comments.Oct 20 2017, 6:07 PM
source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
1041

OK, I investigated this a bit further, and it looks to me like this would require changing the APIs in SBDebugger.h to take IOObject instead of FILE*

Isn't SBDebugger.h the public API that isn't supposed to be changed like that?

lawrence_danna marked 4 inline comments as done.
  • cleanup according to code review suggestions
jasonmolenda accepted this revision.Nov 1 2017, 7:43 PM

I'll check this in for Larry. I'm going to change the Flush methods to FlushDebuggerOutputHandles to make it clearer what they are doing.

This revision is now accepted and ready to land.Nov 1 2017, 7:43 PM
This revision was automatically updated to reflect the committed changes.