[Lldb-commits] [lldb] r357626 - Fix and simplify PrepareCommandsForSourcing
Adrian McCarthy via lldb-commits
lldb-commits at lists.llvm.org
Wed Apr 3 12:49:15 PDT 2019
Author: amccarth
Date: Wed Apr 3 12:49:14 2019
New Revision: 357626
URL: http://llvm.org/viewvc/llvm-project?rev=357626&view=rev
Log:
Fix and simplify PrepareCommandsForSourcing
Spotted some problems in the Driver's PrepareCommandsForSourcing while
helping a colleague track another problem.
1. One error case was not handled because there was no else clause.
Fixed by switching to llvm's early-out style instead of nested
`if (succes) { } else { }` cases. This keeps error handling close
to the actual error.
2. One call-site failed to call the clean-up function. I solved this
by simplifying the API. PrepareCommandsForSourcing no longer requires
the caller to provide a buffer for the pipe's file descriptors and to
call a separate clean-up function later. PrepareCommandsForSourcing
now ensures the file descriptors are handled before returning.
(The read end of the pipe is held open by the returned FILE * as
before.)
I also eliminated an unnecessary local, shorted the lifetime of another,
and tried to improve the comments.
I wrapped the call to open the pipe to get the `#ifdef`s out of the
mainline. I replaced the `close`/`_close` calls with a platform-neutral
helper from `llvm::sys` for the same reason. Per discussion on the
review, I'm leaving the `fdopen` call to use the spelling that Windows
has officially deprecated because it still works it avoids more `#ifdef`s.
Differential Revision: https://reviews.llvm.org/D60152
Modified:
lldb/trunk/tools/driver/Driver.cpp
Modified: lldb/trunk/tools/driver/Driver.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/tools/driver/Driver.cpp?rev=357626&r1=357625&r2=357626&view=diff
==============================================================================
--- lldb/trunk/tools/driver/Driver.cpp (original)
+++ lldb/trunk/tools/driver/Driver.cpp Wed Apr 3 12:49:14 2019
@@ -22,6 +22,7 @@
#include "llvm/Support/Format.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/PrettyStackTrace.h"
+#include "llvm/Support/Process.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/WithColor.h"
#include "llvm/Support/raw_ostream.h"
@@ -469,85 +470,58 @@ SBError Driver::ProcessArgs(const opt::I
return error;
}
-static ::FILE *PrepareCommandsForSourcing(const char *commands_data,
- size_t commands_size, int fds[2]) {
- enum PIPES { READ, WRITE }; // Constants 0 and 1 for READ and WRITE
-
- ::FILE *commands_file = nullptr;
- fds[0] = -1;
- fds[1] = -1;
- int err = 0;
+static inline int OpenPipe(int fds[2], std::size_t size) {
#ifdef _WIN32
- err = _pipe(fds, commands_size, O_BINARY);
+ return _pipe(fds, size, O_BINARY);
#else
- err = pipe(fds);
+ (void) size;
+ return pipe(fds);
#endif
- if (err == 0) {
- ssize_t nrwr = write(fds[WRITE], commands_data, commands_size);
- if (nrwr < 0) {
- WithColor::error()
- << format(
- "write(%i, %p, %" PRIu64
- ") failed (errno = %i) when trying to open LLDB commands pipe",
- fds[WRITE], static_cast<const void *>(commands_data),
- static_cast<uint64_t>(commands_size), errno)
- << '\n';
- } else if (static_cast<size_t>(nrwr) == commands_size) {
-// Close the write end of the pipe so when we give the read end to
-// the debugger/command interpreter it will exit when it consumes all
-// of the data
-#ifdef _WIN32
- _close(fds[WRITE]);
- fds[WRITE] = -1;
-#else
- close(fds[WRITE]);
- fds[WRITE] = -1;
-#endif
- // Now open the read file descriptor in a FILE * that we can give to
- // the debugger as an input handle
- commands_file = fdopen(fds[READ], "r");
- if (commands_file != nullptr) {
- fds[READ] = -1; // The FILE * 'commands_file' now owns the read
- // descriptor Hand ownership if the FILE * over to the
- // debugger for "commands_file".
- } else {
- WithColor::error() << format("fdopen(%i, \"r\") failed (errno = %i) "
- "when trying to open LLDB commands pipe",
- fds[READ], errno)
- << '\n';
- }
- }
- } else {
+}
+
+static ::FILE *PrepareCommandsForSourcing(const char *commands_data,
+ size_t commands_size) {
+ enum PIPES { READ, WRITE }; // Indexes for the read and write fds
+ int fds[2] = {-1, -1};
+
+ if (OpenPipe(fds, commands_size) != 0) {
WithColor::error()
<< "can't create pipe file descriptors for LLDB commands\n";
+ return nullptr;
}
- return commands_file;
-}
-
-void CleanupAfterCommandSourcing(int fds[2]) {
- enum PIPES { READ, WRITE }; // Constants 0 and 1 for READ and WRITE
-
- // Close any pipes that we still have ownership of
- if (fds[WRITE] != -1) {
-#ifdef _WIN32
- _close(fds[WRITE]);
- fds[WRITE] = -1;
-#else
- close(fds[WRITE]);
- fds[WRITE] = -1;
-#endif
+ ssize_t nrwr = write(fds[WRITE], commands_data, commands_size);
+ if (nrwr != commands_size) {
+ WithColor::error()
+ << format(
+ "write(%i, %p, %" PRIu64
+ ") failed (errno = %i) when trying to open LLDB commands pipe",
+ fds[WRITE], static_cast<const void *>(commands_data),
+ static_cast<uint64_t>(commands_size), errno)
+ << '\n';
+ llvm::sys::Process::SafelyCloseFileDescriptor(fds[READ]);
+ llvm::sys::Process::SafelyCloseFileDescriptor(fds[WRITE]);
+ return nullptr;
}
- if (fds[READ] != -1) {
-#ifdef _WIN32
- _close(fds[READ]);
- fds[READ] = -1;
-#else
- close(fds[READ]);
- fds[READ] = -1;
-#endif
+ // Close the write end of the pipe, so that the command interpreter will exit
+ // when it consumes all the data.
+ llvm::sys::Process::SafelyCloseFileDescriptor(fds[WRITE]);
+
+ // Open the read file descriptor as a FILE * that we can return as an input
+ // handle.
+ ::FILE *commands_file = fdopen(fds[READ], "rb");
+ if (commands_file == nullptr) {
+ WithColor::error() << format("fdopen(%i, \"rb\") failed (errno = %i) "
+ "when trying to open LLDB commands pipe",
+ fds[READ], errno)
+ << '\n';
+ llvm::sys::Process::SafelyCloseFileDescriptor(fds[READ]);
+ return nullptr;
}
+
+ // 'commands_file' now owns the read descriptor.
+ return commands_file;
}
std::string EscapeString(std::string arg) {
@@ -678,10 +652,9 @@ int Driver::MainLoop() {
bool quit_requested = false;
bool stopped_for_crash = false;
if ((commands_data != nullptr) && (commands_size != 0u)) {
- int initial_commands_fds[2];
bool success = true;
FILE *commands_file = PrepareCommandsForSourcing(
- commands_data, commands_size, initial_commands_fds);
+ commands_data, commands_size);
if (commands_file != nullptr) {
m_debugger.SetInputFileHandle(commands_file, true);
@@ -703,14 +676,13 @@ int Driver::MainLoop() {
if (m_option_data.m_batch && stopped_for_crash &&
!m_option_data.m_after_crash_commands.empty()) {
- int crash_command_fds[2];
SBStream crash_commands_stream;
WriteCommandsForSourcing(eCommandPlacementAfterCrash,
crash_commands_stream);
const char *crash_commands_data = crash_commands_stream.GetData();
const size_t crash_commands_size = crash_commands_stream.GetSize();
commands_file = PrepareCommandsForSourcing(
- crash_commands_data, crash_commands_size, crash_command_fds);
+ crash_commands_data, crash_commands_size);
if (commands_file != nullptr) {
bool local_quit_requested;
bool local_stopped_for_crash;
@@ -727,9 +699,6 @@ int Driver::MainLoop() {
} else
success = false;
- // Close any pipes that we still have ownership of
- CleanupAfterCommandSourcing(initial_commands_fds);
-
// Something went wrong with command pipe
if (!success) {
exit(1);
More information about the lldb-commits
mailing list