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
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to