delete SWIG typemaps for FILE*

[lawrence_danna]

Summary

The SWIG typemaps for FILE* are no longer used, so
this patch deletes them.

[labath]

🍾

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

655

You'll need to handle the error here. (And it would be better to replace auto with Expected<PythonFile> as that would make it obvious that the error needs to be handled.)

[lawrence_danna]

put in missing consumeError()

[lawrence_danna]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

655

thanks, I don't know why I can't remember to do that.

[labath]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

655

Yeah, I don't blame you. The Expected<T> behavior is unlike anything else I've seen, and I myself am still not convinced that such a draconian way of enforcing error checks is in order. However, it is here, and I definitely think it's better than passing around "Status" and T objects separately, like old lldb APIs do.

What you could do is help yourself and reviewers notice that and tune down the "auto" dial (that's also an llvm policy) and spell out the Expected type. Otherwise the type can be confused with Optional<T> (I wish we could say Expected<auto>), which does *not* require checks...

lldb/include/lldb/Host/File.h

public:
   /// Get the file specification for this file, if possible.
   ///
   /// \param[out] file_spec
   ///     the file specification.
   /// \return
   ///     ENOTSUP, success, or another error.
   virtual Status GetFileSpec(FileSpec &file_spec) const;
 
-  /// DEPRECATED! Extract the underlying FILE* and reset this File without closing it.
-  ///
-  /// This is only here to support legacy SB interfaces that need to convert scripting
-  /// language objects into FILE* streams.   That conversion is inherently sketchy and
-  /// doing so may cause the stream to be leaked.
-  ///
-  /// After calling this the File will be reset to its original state.  It will be
-  /// invalid and it will not hold on to any resources.
-  ///
-  /// \return
-  ///     The underlying FILE* stream from this File, if one exists and can be extracted,
-  ///     nullptr otherwise.
-  virtual FILE *TakeStreamAndClear();
-
   /// Get underlying OS file descriptor for this file, or kInvalidDescriptor.
   /// If the descriptor is valid, then it may be used directly for I/O
   /// However, the File may also perform it's own buffering, so avoid using
   /// this if it is not necessary, or use Flush() appropriately.
   ///
   /// \return
   ///    a valid file descriptor for this file or kInvalidDescriptor
   virtual int GetDescriptor() const;
bool IsValid() const override {
     return DescriptorIsValid() || StreamIsValid();
   }
 
   Status Read(void *buf, size_t &num_bytes) override;
   Status Write(const void *buf, size_t &num_bytes) override;
   Status Close() override;
   WaitableHandle GetWaitableHandle() override;
   Status GetFileSpec(FileSpec &file_spec) const override;
-  FILE *TakeStreamAndClear() override;
   int GetDescriptor() const override;
   FILE *GetStream() override;
   off_t SeekFromStart(off_t offset, Status *error_ptr = nullptr) override;
   off_t SeekFromCurrent(off_t offset, Status *error_ptr = nullptr) override;
   off_t SeekFromEnd(off_t offset, Status *error_ptr = nullptr) override;
   Status Read(void *dst, size_t &num_bytes, off_t &offset) override;
   Status Write(const void *src, size_t &num_bytes, off_t &offset) override;
   Status Flush() override;

lldb/scripts/Python/python-typemaps.swig

%typemap(out) lldb::FileSP {
   }
   if (!$result)
   {
       $result = Py_None;
       Py_INCREF(Py_None);
   }
 }
 
-// FIXME both of these paths wind up calling fdopen() with no provision for ever calling
-// fclose() on the result.  SB interfaces that use FILE* should be deprecated for scripting
-// use and this typemap should eventually be removed.
-%typemap(in) FILE * {
-   using namespace lldb_private;
-   if ($input == Py_None)
-      $1 = nullptr;
-   else if (!lldb_private::PythonFile::Check($input)) {
-      int fd = PyObject_AsFileDescriptor($input);
-      if (fd < 0 || PyErr_Occurred())
-        return nullptr;
-      PythonObject py_input(PyRefType::Borrowed, $input);
-      PythonString py_mode = py_input.GetAttributeValue("mode").AsType<PythonString>();
-      if (!py_mode.IsValid() || PyErr_Occurred())
-        return nullptr;
-        FILE *f;
-        if ((f = fdopen(fd, py_mode.GetString().str().c_str())))
-          $1 = f;
-        else {
-          PyErr_SetString(PyExc_TypeError, strerror(errno));
-          return nullptr;
-        }
-   }
-   else
-   {
-      PythonFile py_file(PyRefType::Borrowed, $input);
-      lldb::FileUP file = py_file.GetUnderlyingFile();
-      if (!file)
-         return nullptr;
-      $1 = file->TakeStreamAndClear();
-    }
-}
-
-%typemap(out) FILE * {
-   // TODO: This is gross. We should find a way to fetch the mode flags from the
-   // lldb_private::File object.
-   char mode[4] = {0};
-%#ifdef __APPLE__
-   int i = 0;
-   if ($1)
-   {
-       short flags = $1->_flags;
-
-       if (flags & __SRD)
-          mode[i++] = 'r';
-       else if (flags & __SWR)
-          mode[i++] = 'w';
-       else // if (flags & __SRW)
-          mode[i++] = 'a';
-    }
-%#else
-   // There's no portable way to get the mode here. We just return a mode which
-   // permits both reads and writes and count on the operating system to return
-   // an error when an invalid operation is attempted.
-   mode[0] = 'r';
-   mode[1] = '+';
-%#endif
-   using namespace lldb_private;
-   NativeFile file($1, false);
-   PythonFile py_file(file, mode);
-   $result = py_file.release();
-   if (!$result)
-   {
-       $result = Py_None;
-       Py_INCREF(Py_None);
-   }
-}
-
 %typemap(in) (const char* string, int len) {
     using namespace lldb_private;
     if ($input == Py_None)
     {
         $1 = NULL;
         $2 = 0;
     }
     else if (PythonString::Check($input))

lldb/source/Host/common/File.cpp

IOObject::WaitableHandle File::GetWaitableHandle() {
   return IOObject::kInvalidHandleValue;
 }
 
 Status File::GetFileSpec(FileSpec &file_spec) const {
   file_spec.Clear();
   return std::error_code(ENOTSUP, std::system_category());
 }
 
-FILE *File::TakeStreamAndClear() { return nullptr; }
-
 int File::GetDescriptor() const { return kInvalidDescriptor; }
 
 FILE *File::GetStream() { return nullptr; }
 
 off_t File::SeekFromStart(off_t offset, Status *error_ptr) {
   if (error_ptr)
     *error_ptr = std::error_code(ENOTSUP, std::system_category());
   return -1;
Status NativeFile::Close() {
   m_options = OpenOptions(0);
   m_own_stream = false;
   m_own_descriptor = false;
   m_is_interactive = eLazyBoolCalculate;
   m_is_real_terminal = eLazyBoolCalculate;
   return error;
 }
 
-FILE *NativeFile::TakeStreamAndClear() {
-  FILE *stream = GetStream();
-  m_stream = NULL;
-  m_descriptor = kInvalidDescriptor;
-  m_options = OpenOptions();
-  m_own_stream = false;
-  m_own_descriptor = false;
-  m_is_interactive = m_supports_colors = m_is_real_terminal =
-      eLazyBoolCalculate;
-  return stream;
-}
-
 Status NativeFile::GetFileSpec(FileSpec &file_spec) const {
   Status error;
 #ifdef F_GETPATH
   if (IsValid()) {
     char path[PATH_MAX];
     if (::fcntl(GetDescriptor(), F_GETPATH, path) == -1)
       error.SetErrorToErrno();
     else

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h

public:
 
   PythonFile() : TypedPythonObject() {} // MSVC requires this for some reason
 
   static bool Check(PyObject *py_obj);
 
   static llvm::Expected<PythonFile> FromFile(File &file,
                                              const char *mode = nullptr);
 
-  // FIXME delete this after FILE* typemaps are deleted
-  // and ScriptInterpreterPython is fixed
-  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());
-    }
-  }
-
-  lldb::FileUP GetUnderlyingFile() const;
-
   llvm::Expected<lldb::FileSP> ConvertToFile(bool borrowed = false);
   llvm::Expected<lldb::FileSP>
   ConvertToFileForcingUseOfScriptingIOMethods(bool borrowed = false);
 };
 
 class PythonException : public llvm::ErrorInfo<PythonException> {
 private:
   PyObject *m_exception_type, *m_exception, *m_traceback;

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

#else
   if (r < 0) {
     llvm::consumeError(exception()); // clear the exception and log it.
     return false;
   }
   return !!r;
 #endif
 }
 
-FileUP PythonFile::GetUnderlyingFile() const {
-  if (!IsValid())
-    return nullptr;
-
-  // We don't own the file descriptor returned by this function, make sure the
-  // File object knows about that.
-  PythonString py_mode = GetAttributeValue("mode").AsType<PythonString>();
-  auto options = File::GetOptionsFromMode(py_mode.GetString());
-  if (!options) {
-    llvm::consumeError(options.takeError());
-    return nullptr;
-  }
-  auto file = std::unique_ptr<File>(new NativeFile(
-      PyObject_AsFileDescriptor(m_py_obj), options.get(), false));
-  if (!file->IsValid())
-    return nullptr;
-  return file;
-}
-
 namespace {
 class GIL {
 public:
   GIL() {
     m_state = PyGILState_Ensure();
     assert(!PyErr_Occurred());
   }
   ~GIL() { PyGILState_Release(m_state); }

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

bool ScriptInterpreterPythonImpl::SetStdHandle(FileSP file_sp,
   }
   File &file = *file_sp;
 
   // Flush the file before giving it to python to avoid interleaved output.
   file.Flush();
 
   PythonDictionary &sys_module_dict = GetSysModuleDictionary();
 
+  auto new_file = PythonFile::FromFile(file, mode);
+  if (!new_file) {
+    llvm::consumeError(new_file.takeError());
+    return false;
+  }
+
   save_file = sys_module_dict.GetItemForKey(PythonString(py_name));
 
-  PythonFile new_file(file, mode);
-  sys_module_dict.SetItemForKey(PythonString(py_name), new_file);
+  sys_module_dict.SetItemForKey(PythonString(py_name), new_file.get());
   return true;
 }
 
 bool ScriptInterpreterPythonImpl::EnterSession(uint16_t on_entry_flags,
                                                FileSP in_sp, FileSP out_sp,
                                                FileSP err_sp) {
   // If we have already entered the session, without having officially 'left'
   // it, then there is no need to 'enter' it again.

lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

TEST_F(PythonDataObjectsTest, TestPythonCallableInvoke) {
   EXPECT_EQ(two.get(), list_result.GetItemAtIndex(1).get());
   EXPECT_EQ(three.get(), list_result.GetItemAtIndex(2).get());
 }
 
 TEST_F(PythonDataObjectsTest, TestPythonFile) {
   auto file = FileSystem::Instance().Open(FileSpec(FileSystem::DEV_NULL),
                                           File::eOpenOptionRead);
   ASSERT_THAT_EXPECTED(file, llvm::Succeeded());
-  PythonFile py_file(*file.get(), "r");
-  EXPECT_TRUE(PythonFile::Check(py_file.get()));
+  auto py_file = PythonFile::FromFile(*file.get(), "r");
+  ASSERT_THAT_EXPECTED(py_file, llvm::Succeeded());
+  EXPECT_TRUE(PythonFile::Check(py_file.get().get()));
 }
 
 TEST_F(PythonDataObjectsTest, TestObjectAttributes) {
   PythonInteger py_int(42);
   EXPECT_TRUE(py_int.HasAttribute("numerator"));
   EXPECT_FALSE(py_int.HasAttribute("this_should_not_exist"));
 
   PythonInteger numerator_attr =