Fix and simplify PrepareCommandsForSourcing

[amccarth]

Summary

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 (success) { } 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 #ifdefs 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 #ifdefs.

[labath]

Nice cleanup, thanks. I have one suggestion inline you can implement if you think it's a good idea.

FTR, I believe using pipes here is wrong altogether because it can lead to deadlock. The size of the pipe buffer is implementation-defined, and since there's noone reading from it while we write it, we can block indefinitely if we encounter a particularly long sequence of -o commands. I guess the fact that we haven't hit this in practice means that the implementation-defined size is sufficiently big on all OSs we care about.

lldb/tools/driver/Driver.cpp

492–496 ↗

(On Diff #193364)

Do you think it would be possible to use llvm::Process::SafelyCloseFileDescriptor here? It looks like that still uses close (without the _) on windows, which we are trying hard to avoid here, but I'm not sure why. I know close is technically deprecated on windows, but AFAIK the only thing deprecated about it is the name, and otherwise it works correctly.

(If that works, I'd even consider removing this function entirely, as the only thing it does extra is clear the fd, but that does not seem necessary since we're now calling it immediately before we return (and the fds go out of scope anyway)).

[amccarth]

Thanks for the review.

In D60152#1452704, @labath wrote:

FTR, I believe using pipes here is wrong altogether because it can lead to deadlock. The size of the pipe buffer is implementation-defined, and since there's noone reading from it while we write it, we can block indefinitely if we encounter a particularly long sequence of -o commands. I guess the fact that we haven't hit this in practice means that the implementation-defined size is sufficiently big on all OSs we care about.

Agreed, pipes probably aren't the right way, but this was just a drive-by cleanup. On Windows, the necessary size is passed to the _pipe call, so if we have an unusually large buffer of commands it should either work or fail gracefully rather than deadlocking.

lldb/tools/driver/Driver.cpp

492–496 ↗

(On Diff #193364)

I'm happy to use llvm::sys::Process::SafelyCloseFileDescriptor and delete my little wrapper. I agree that resetting the df to -1 right before it goes out of scope isn't useful. I'll update the patch momentarily.

If we're not that worried about the deprecated names on Windows, should I also just use fdopen and forget about wrapping that as well?

Either way, I'm going to keep the OpenPipe wrapper since the Windows version takes additional parameters.

[amccarth]

Further simplification per labath's feedback.

Specifically:

1. Use a utility function from llvm to avoid making own wrapper for closing a file descriptor.
2. Don't bother resetting fd numbers right before they go out of scope.
3. Live with the Windows-deprecated spelling of fdopen in favor of more #ifdefs.
4. Updated summary to reflect these changes.

[labath]

lldb/tools/driver/Driver.cpp

492–496 ↗

(On Diff #193364)

I think using the non-underscore fdopen is fine, but I am not a windows guy. (The nice thing about SafelyCloseFileDescriptor is that it already provides an abstraction and so we can add the underscore there centrally, should it ever become necessary).

[zturner]

lldb/tools/driver/Driver.cpp

492–496 ↗

(On Diff #193364)

I dug into this a little bit to find out what the actual difference is, and there isn't a difference at all. close and _close, fdopen and _fdopen, and all other pairs of underscore and non underscore names both resolve to the exact same symbol in the object file (it links in an object file that has a symbol for close that aliases it to _close. So there is literally no difference.

[ted]

llvm/tools/lldb/tools/driver/Driver.cpp:495:12: error: comparison of integers of different signs: 'ssize_t' (aka 'long') and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]

if (nrwr != commands_size) {
    ~~~~ ^  ~~~~~~~~~~~~~

1 error generated.

lldb/trunk/tools/driver/Driver.cpp

 #include "lldb/API/SBStream.h"
 #include "lldb/API/SBStringList.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ConvertUTF.h"
 #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"
 
 #include <algorithm>
 #include <atomic>
 #include <bitset>
 #include <csignal>
if (python_file_spec.IsValid()) {
       llvm::outs() << "<COULD NOT FIND PATH>\n";
     exiting = true;
     return error;
   }
 
   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 {
-    WithColor::error()
-        << "can't create pipe file descriptors for LLDB commands\n";
+}
+
+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;
+  }
+
+  if (nrwr != commands_size) {
+    llvm::sys::Process::SafelyCloseFileDescriptor(fds[READ]);
+    llvm::sys::Process::SafelyCloseFileDescriptor(fds[WRITE]);
+    return nullptr;
+  }
+
+  // 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) "
+    llvm::sys::Process::SafelyCloseFileDescriptor(fds[READ]);
+    return nullptr;
   }
 
+  // 'commands_file' now owns the read descriptor.
   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
-  }
-
-  if (fds[READ] != -1) {
-#ifdef _WIN32
-    _close(fds[READ]);
-    fds[READ] = -1;
-#else
-    close(fds[READ]);
-    fds[READ] = -1;
-#endif
-  }
-}
-
 std::string EscapeString(std::string arg) {
   std::string::size_type pos = 0;
   while ((pos = arg.find_first_of("\"\\", pos)) != std::string::npos) {
     arg.insert(pos, 1, '\\');
     pos += 2;
   }
   return '"' + arg + '"';
 }
#endif
   const char *commands_data = commands_stream.GetData();
   const size_t commands_size = commands_stream.GetSize();
 
   // The command file might have requested that we quit, this variable will
   // track that.
   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);
 
       // Set the debugger into Sync mode when running the command file.
       // Otherwise command files
       // that run the target won't run in a sensible way.
       bool old_async = m_debugger.GetAsync();
       m_debugger.SetAsync(false);
       int num_errors;
 
       SBCommandInterpreterRunOptions options;
       options.SetStopOnError(true);
       if (m_option_data.m_batch)
         options.SetStopOnCrash(true);
 
       m_debugger.RunCommandInterpreter(handle_events, spawn_thread, options,
                                        num_errors, quit_requested,
                                        stopped_for_crash);
 
       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;
           m_debugger.SetInputFileHandle(commands_file, true);
 
           m_debugger.RunCommandInterpreter(
               handle_events, spawn_thread, options, num_errors,
               local_quit_requested, local_stopped_for_crash);
           if (local_quit_requested)
             quit_requested = true;
         }
       }
       m_debugger.SetAsync(old_async);
     } 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);
     }
   }
 
   // Now set the input file handle to STDIN and run the command
   // interpreter again in interactive mode or repl mode and let the debugger