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 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
