amccarth created this revision.
amccarth added reviewers: zturner, labath, rnk.
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. It used `fdopen` on Windows, which is a deprecated name. In other
places the function used `#ifdef _WIN32` to use the proper names
for Windows and Posix. For readability and consistency, I factored
these out into small static functions.
3. 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.
https://reviews.llvm.org/D60152
Files:
lldb/tools/driver/Driver.cpp
Index: lldb/tools/driver/Driver.cpp
===================================================================
--- lldb/tools/driver/Driver.cpp
+++ lldb/tools/driver/Driver.cpp
@@ -469,85 +469,79 @@
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
+static inline int OpenPipe(int fds[2], std::size_t size) {
+#ifdef _WIN32
+ return _pipe(fds, size, O_BINARY);
+#else
+ (void) size;
+ return pipe(fds);
+#endif
+}
+
- ::FILE *commands_file = nullptr;
- fds[0] = -1;
- fds[1] = -1;
- int err = 0;
+static inline ::FILE *OpenFileDescriptor(int fd, const char *mode) {
#ifdef _WIN32
- err = _pipe(fds, commands_size, O_BINARY);
+ return _fdopen(fd, mode);
#else
- err = pipe(fds);
+ return fdopen(fd, mode);
#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
+}
+
+static inline int CloseFileDescriptor(int &fd) {
+ int result =
#ifdef _WIN32
- _close(fds[WRITE]);
- fds[WRITE] = -1;
+ _close(fd);
#else
- close(fds[WRITE]);
- fds[WRITE] = -1;
+ close(fd);
#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 {
+ fd = -1;
+ return result;
+}
+
+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;
-}
+ 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';
+ CloseFileDescriptor(fds[READ]);
+ CloseFileDescriptor(fds[WRITE]);
+ return nullptr;
+ }
-void CleanupAfterCommandSourcing(int fds[2]) {
- enum PIPES { READ, WRITE }; // Constants 0 and 1 for READ and WRITE
+ // Close the write end of the pipe, so that the command interpreter will exit
+ // when it consumes all the data.
+ CloseFileDescriptor(fds[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
+ // Open the read file descriptor as a FILE * that we can return as an input
+ // handle.
+ ::FILE *commands_file = OpenFileDescriptor(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';
+ CloseFileDescriptor(fds[READ]);
+ return nullptr;
}
- if (fds[READ] != -1) {
-#ifdef _WIN32
- _close(fds[READ]);
- fds[READ] = -1;
-#else
- close(fds[READ]);
- fds[READ] = -1;
-#endif
- }
+ // 'commands_file' now owns the read descriptor.
+ fds[READ] = -1;
+ return commands_file;
}
std::string EscapeString(std::string arg) {
@@ -678,10 +672,9 @@
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 +696,13 @@
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 +719,6 @@
} 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);
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
