[Lldb-commits] [lldb] [lldb] Fix EditlineTest closing files multiple times. (PR #169100)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: John Harrison (ashgti)

<details>
<summary>Changes</summary>

This updates the EditlineTest to use `lldb::FileSP` to ensure the associated FDs are only closed a single time.

Currently, there is some confusion between the `FilePointer`, `PseudoTerminal` and `LockableStreamFile` about when the files are closed resulting in a crash in some due to a `fflush` on a closed file.

---
Full diff: https://github.com/llvm/llvm-project/pull/169100.diff


1 Files Affected:

- (modified) lldb/unittests/Editline/EditlineTest.cpp (+28-58) 


``````````diff
diff --git a/lldb/unittests/Editline/EditlineTest.cpp b/lldb/unittests/Editline/EditlineTest.cpp
index 2875f4ee7e6b4..2afc33680f348 100644
--- a/lldb/unittests/Editline/EditlineTest.cpp
+++ b/lldb/unittests/Editline/EditlineTest.cpp
@@ -9,6 +9,8 @@
 #include "lldb/Host/Config.h"
 #include "lldb/Host/File.h"
 #include "lldb/Host/HostInfo.h"
+#include "lldb/lldb-forward.h"
+#include "llvm/Testing/Support/Error.h"
 
 #if LLDB_ENABLE_LIBEDIT
 
@@ -25,7 +27,6 @@
 #include "TestingSupport/SubsystemRAII.h"
 #include "lldb/Host/Editline.h"
 #include "lldb/Host/FileSystem.h"
-#include "lldb/Host/Pipe.h"
 #include "lldb/Host/PseudoTerminal.h"
 #include "lldb/Host/StreamFile.h"
 #include "lldb/Utility/Status.h"
@@ -37,27 +38,6 @@ namespace {
 const size_t TIMEOUT_MILLIS = 5000;
 }
 
-class FilePointer {
-public:
-  FilePointer() = delete;
-
-  FilePointer(const FilePointer &) = delete;
-
-  FilePointer(FILE *file_p) : _file_p(file_p) {}
-
-  ~FilePointer() {
-    if (_file_p != nullptr) {
-      const int close_result = fclose(_file_p);
-      EXPECT_EQ(0, close_result);
-    }
-  }
-
-  operator FILE *() { return _file_p; }
-
-private:
-  FILE *_file_p;
-};
-
 /**
  Wraps an Editline class, providing a simple way to feed
  input (as if from the keyboard) and receive output from Editline.
@@ -90,44 +70,39 @@ class EditlineAdapter {
   std::recursive_mutex output_mutex;
   std::unique_ptr<lldb_private::Editline> _editline_sp;
 
-  PseudoTerminal _pty;
-  int _pty_primary_fd = -1;
-  int _pty_secondary_fd = -1;
-
-  std::unique_ptr<FilePointer> _el_secondary_file;
+  lldb::FileSP _el_primary_file;
+  lldb::FileSP _el_secondary_file;
 };
 
-EditlineAdapter::EditlineAdapter()
-    : _editline_sp(), _pty(), _el_secondary_file() {
+EditlineAdapter::EditlineAdapter() : _editline_sp(), _el_secondary_file() {
   lldb_private::Status error;
+  PseudoTerminal pty;
 
   // Open the first primary pty available.
-  EXPECT_THAT_ERROR(_pty.OpenFirstAvailablePrimary(O_RDWR), llvm::Succeeded());
+  EXPECT_THAT_ERROR(pty.OpenFirstAvailablePrimary(O_RDWR), llvm::Succeeded());
+  // Open the corresponding secondary pty.
+  EXPECT_THAT_ERROR(pty.OpenSecondary(O_RDWR), llvm::Succeeded());
 
   // Grab the primary fd.  This is a file descriptor we will:
   // (1) write to when we want to send input to editline.
   // (2) read from when we want to see what editline sends back.
-  _pty_primary_fd = _pty.GetPrimaryFileDescriptor();
+  _el_primary_file.reset(
+      new NativeFile(pty.ReleasePrimaryFileDescriptor(),
+                     lldb_private::NativeFile::eOpenOptionReadWrite, true));
 
-  // Open the corresponding secondary pty.
-  EXPECT_THAT_ERROR(_pty.OpenSecondary(O_RDWR), llvm::Succeeded());
-  _pty_secondary_fd = _pty.GetSecondaryFileDescriptor();
-
-  _el_secondary_file.reset(new FilePointer(fdopen(_pty_secondary_fd, "rw")));
-  EXPECT_FALSE(nullptr == *_el_secondary_file);
-  if (*_el_secondary_file == nullptr)
-    return;
+  _el_secondary_file.reset(
+      new NativeFile(pty.ReleaseSecondaryFileDescriptor(),
+                     lldb_private::NativeFile::eOpenOptionReadWrite, true));
 
   lldb::LockableStreamFileSP output_stream_sp =
-      std::make_shared<LockableStreamFile>(*_el_secondary_file,
-                                           NativeFile::Unowned, output_mutex);
+      std::make_shared<LockableStreamFile>(_el_secondary_file, output_mutex);
   lldb::LockableStreamFileSP error_stream_sp =
-      std::make_shared<LockableStreamFile>(*_el_secondary_file,
-                                           NativeFile::Unowned, output_mutex);
+      std::make_shared<LockableStreamFile>(_el_secondary_file, output_mutex);
 
   // Create an Editline instance.
   _editline_sp.reset(new lldb_private::Editline(
-      "gtest editor", *_el_secondary_file, output_stream_sp, error_stream_sp,
+      "gtest editor", _el_secondary_file->GetStream(), output_stream_sp,
+      error_stream_sp,
       /*color=*/false));
   _editline_sp->SetPrompt("> ");
 
@@ -140,7 +115,7 @@ EditlineAdapter::EditlineAdapter()
 
 void EditlineAdapter::CloseInput() {
   if (_el_secondary_file != nullptr)
-    _el_secondary_file.reset(nullptr);
+    _el_secondary_file->Close();
 }
 
 bool EditlineAdapter::SendLine(const std::string &line) {
@@ -148,19 +123,14 @@ bool EditlineAdapter::SendLine(const std::string &line) {
   if (!IsValid())
     return false;
 
+  std::string out = line + "\n";
+
   // Write the line out to the pipe connected to editline's input.
-  ssize_t input_bytes_written =
-      ::write(_pty_primary_fd, line.c_str(),
-              line.length() * sizeof(std::string::value_type));
-
-  const char *eoln = "\n";
-  const size_t eoln_length = strlen(eoln);
-  input_bytes_written =
-      ::write(_pty_primary_fd, eoln, eoln_length * sizeof(char));
-
-  EXPECT_NE(-1, input_bytes_written) << strerror(errno);
-  EXPECT_EQ(eoln_length * sizeof(char), size_t(input_bytes_written));
-  return eoln_length * sizeof(char) == size_t(input_bytes_written);
+  size_t num_bytes = out.length() * sizeof(std::string::value_type);
+  EXPECT_THAT_ERROR(_el_primary_file->Write(out.c_str(), num_bytes).takeError(),
+                    llvm::Succeeded());
+  EXPECT_EQ(num_bytes, out.length() * sizeof(std::string::value_type));
+  return true;
 }
 
 bool EditlineAdapter::SendLines(const std::vector<std::string> &lines) {
@@ -215,7 +185,7 @@ bool EditlineAdapter::IsInputComplete(lldb_private::Editline *editline,
 }
 
 void EditlineAdapter::ConsumeAllOutput() {
-  FilePointer output_file(fdopen(_pty_primary_fd, "r"));
+  FILE *output_file = _el_primary_file->GetStream();
 
   int ch;
   while ((ch = fgetc(output_file)) != EOF) {

``````````

</details>


https://github.com/llvm/llvm-project/pull/169100