Add an unnamed pipe fail-safe to launching lldb-gdbserver.

[chaoren]

Summary

Currently, launching lldb-gdbserver from platform on Android requires root for
mkfifo() and an explicit TMPDIR variable. This should remove both requirements.

[clayborg]

Looks good.

[ovyalov]

lldb/trunk/tools/lldb-server/lldb-gdbserver.cpp

322 ↗

(On Diff #24512)

Could we pass Pipe by reference here? Otherwise ~Pipe is called twice trying to close the same pipe descriptor more than once.

include/lldb/Host/posix/PipePosix.h

 #define liblldb_Host_posix_PipePosix_h_
 #if defined(__cplusplus)
 
 #include "lldb/Host/PipeBase.h"
 
 namespace lldb_private {
 
 //----------------------------------------------------------------------
-/// @class PipePosix PipePosix  .h "lldb/Host/posix/PipePosix.h"
+/// @class PipePosix PipePosix.h "lldb/Host/posix/PipePosix.h"
 /// @brief A posix-based implementation of Pipe, a class that abtracts
 ///        unix style pipes.
 ///
 /// A class that abstracts the LLDB core from host pipe functionality.
 //----------------------------------------------------------------------
 class PipePosix : public PipeBase
 {
 public:
     static int kInvalidDescriptor;
 
     PipePosix();
 
     ~PipePosix() override;
 
     Error
     CreateNew(bool child_process_inherit) override;
     Error
     CreateNew(llvm::StringRef name, bool child_process_inherit) override;
     Error
+    CreateWithFD(int read_fd, int write_fd);
+    Error
     CreateWithUniqueName(llvm::StringRef prefix, bool child_process_inherit, llvm::SmallVectorImpl<char>& name) override;
     Error
     OpenAsReader(llvm::StringRef name, bool child_process_inherit) override;
     Error
     OpenAsWriterWithTimeout(llvm::StringRef name, bool child_process_inherit, const std::chrono::microseconds &timeout) override;
 
     bool
     CanRead() const override;
public:
     Error
     Delete(llvm::StringRef name) override;
 
     Error
     Write(const void *buf, size_t size, size_t &bytes_written) override;
     Error
     ReadWithTimeout(void *buf, size_t size, const std::chrono::microseconds &timeout, size_t &bytes_read) override;
 
-private:
     void
     CloseReadFileDescriptor();
     void
     CloseWriteFileDescriptor();
 
+private:
     int m_fds[2];
 };
 
 } // namespace lldb_private
 
 #endif  // #if defined(__cplusplus)
 #endif  // liblldb_Host_posix_PipePosix_h_

source/Host/posix/PipePosix.cpp

PipePosix::CreateNew(llvm::StringRef name, bool child_process_inherit)
     Error error;
     if (::mkfifo(name.data(), 0660) != 0)
         error.SetErrorToErrno();
 
     return error;
 }
 
 Error
+PipePosix::CreateWithFD(int read_fd, int write_fd) {
+    if (CanRead() || CanWrite())
+        return Error("Pipe is already opened");
+    m_fds[READ] = read_fd;
+    m_fds[WRITE] = write_fd;
+    return Error();
+}
+
+Error
 PipePosix::CreateWithUniqueName(llvm::StringRef prefix, bool child_process_inherit, llvm::SmallVectorImpl<char>& name)
 {
     llvm::SmallString<PATH_MAX> named_pipe_path;
     llvm::SmallString<PATH_MAX> pipe_spec((prefix + ".%%%%%%").str());
     FileSpec tmpdir_file_spec;
     tmpdir_file_spec.Clear();
     if (HostInfo::GetLLDBPath(ePathTypeLLDBTempSystemDir, tmpdir_file_spec))
     {

source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

#endif
         debugserver_args.AppendArgument("--native-regs");
 
         if (launch_info.GetLaunchInSeparateProcessGroup())
         {
             debugserver_args.AppendArgument("--setsid");
         }
 
         llvm::SmallString<PATH_MAX> named_pipe_path;
-        Pipe port_named_pipe;
+        Pipe port_pipe;
 
         bool listen = false;
         if (host_and_port[0])
         {
             // Create a temporary file to get the stdout/stderr and redirect the
             // output of the command into this file. We will later read this file
             // if all goes well and fill the data into "command_output_ptr"
 
             if (in_port == 0)
             {
                 // Binding to port zero, we need to figure out what port it ends up
                 // using using a named pipe...
-                error = port_named_pipe.CreateWithUniqueName("debugserver-named-pipe", false, named_pipe_path);
-                if (error.Fail())
-                    return error;
+                error = port_pipe.CreateWithUniqueName("debugserver-named-pipe", false, named_pipe_path);
+                if (error.Success())
+                {
                     debugserver_args.AppendArgument("--named-pipe");
                     debugserver_args.AppendArgument(named_pipe_path.c_str());
                 }
                 else
                 {
+                    if (log)
+                        log->Printf("GDBRemoteCommunication::%s() "
+                                "named pipe creation failed: %s",
+                                __FUNCTION__, error.AsCString());
+                    // let's try an unnamed pipe
+                    error = port_pipe.CreateNew(true);
+                    if (error.Fail())
+                    {
+                        if (log)
+                            log->Printf("GDBRemoteCommunication::%s() "
+                                    "unnamed pipe creation failed: %s",
+                                    __FUNCTION__, error.AsCString());
+                        return error;
+                    }
+                    int write_fd = port_pipe.GetWriteFileDescriptor();
+                    debugserver_args.AppendArgument("--pipe");
+                    debugserver_args.AppendArgument(std::to_string(write_fd).c_str());
+                    launch_info.AppendCloseFileAction(port_pipe.GetReadFileDescriptor());
+                }
+            }
+            else
+            {
                 listen = true;
             }
         }
         else
         {
             // No host and port given, so lets listen on our end and make the debugserver
             // connect to us..
             error = StartListenThread ("127.0.0.1", 0);
#endif
         launch_info.AppendSuppressFileAction (STDERR_FILENO, false, true);
         
         error = Host::LaunchProcess(launch_info);
         
         if (error.Success() && launch_info.GetProcessID() != LLDB_INVALID_PROCESS_ID)
         {
             if (named_pipe_path.size() > 0)
             {
-                error = port_named_pipe.OpenAsReader(named_pipe_path, false);
-                if (error.Success())
-                {
-                    char port_cstr[256];
-                    port_cstr[0] = '\0';
-                    size_t num_bytes = sizeof(port_cstr);
-                    // Read port from pipe with 10 second timeout.
-                    error = port_named_pipe.ReadWithTimeout(port_cstr, num_bytes, std::chrono::microseconds(10 * 1000000), num_bytes);
-                    if (error.Success())
-                    {
-                        assert (num_bytes > 0 && port_cstr[num_bytes-1] == '\0');
-                        out_port = StringConvert::ToUInt32(port_cstr, 0);
-                        if (log)
-                            log->Printf("GDBRemoteCommunication::%s() debugserver listens %u port", __FUNCTION__, out_port);
-                    }
-                    else
-                    {
-                        if (log)
-                            log->Printf("GDBRemoteCommunication::%s() failed to read a port value from named pipe %s: %s", __FUNCTION__, named_pipe_path.c_str(), error.AsCString());
+                error = port_pipe.OpenAsReader(named_pipe_path, false);
+                if (error.Fail())
+                    if (log)
+                        log->Printf("GDBRemoteCommunication::%s() "
+                                "failed to open named pipe %s for reading: %s",
+                                __FUNCTION__, named_pipe_path.c_str(), error.AsCString());
+            }
+
+            if (port_pipe.CanWrite())
+                port_pipe.CloseWriteFileDescriptor();
+            if (port_pipe.CanRead())
+                error = port_pipe.ReadWithTimeout(port_cstr, num_bytes,
+                        std::chrono::seconds{10}, num_bytes);
+                    assert(num_bytes > 0 && port_cstr[num_bytes-1] == '\0');
+                        log->Printf("GDBRemoteCommunication::%s() "
+                                "debugserver listens %u port",
+                                __FUNCTION__, out_port);
+                        log->Printf("GDBRemoteCommunication::%s() "
+                                "failed to read a port value from pipe %s: %s",
+                                __FUNCTION__, named_pipe_path.c_str(), error.AsCString());
 
-                    }
-                    port_named_pipe.Close();
-                }
-                else
-                {
-                    if (log)
-                        log->Printf("GDBRemoteCommunication::%s() failed to open named pipe %s for reading: %s", __FUNCTION__, named_pipe_path.c_str(), error.AsCString());
-                }
-                const auto err = port_named_pipe.Delete(named_pipe_path);
+                port_pipe.Close();
+
+            if (named_pipe_path.size() > 0)
+                const auto err = port_pipe.Delete(named_pipe_path);
                 if (err.Fail())
                 {
                     if (log)
-                        log->Printf ("GDBRemoteCommunication::%s failed to delete pipe %s: %s", __FUNCTION__, named_pipe_path.c_str(), err.AsCString());
+                        log->Printf ("GDBRemoteCommunication::%s failed to delete pipe %s: %s",
+                                __FUNCTION__, named_pipe_path.c_str(), err.AsCString());
                 }
             }
-            else if (listen)
-            {
-                
-            }
-            else
-            {
-                // Make sure we actually connect with the debugserver...
-                JoinListenThread();
-            }
-        }
-    }
     else
     {
         error.SetErrorStringWithFormat ("unable to locate " DEBUGSERVER_BASENAME );
     }
     return error;
 }
 
 void

tools/lldb-server/lldb-gdbserver.cpp

 {
     { "debug",              no_argument,        &g_debug,           1   },
     { "platform",           required_argument,  NULL,               'p' },
     { "verbose",            no_argument,        &g_verbose,         1   },
     { "lldb-command",       required_argument,  NULL,               'c' },
     { "log-file",           required_argument,  NULL,               'l' },
     { "log-flags",          required_argument,  NULL,               'f' },
     { "attach",             required_argument,  NULL,               'a' },
-    { "named-pipe",         required_argument,  NULL,               'P' },
+    { "named-pipe",         required_argument,  NULL,               'N' },
+    { "pipe",               required_argument,  NULL,               'U' },
     { "native-regs",        no_argument,        NULL,               'r' },  // Specify to use the native registers instead of the gdb defaults for the architecture.  NOTE: this is a do-nothing arg as it's behavior is default now.  FIXME remove call from lldb-platform.
     { "reverse-connect",    no_argument,        NULL,               'R' },  // Specifies that llgs attaches to the client address:port rather than llgs listening for a connection from address on port.
     { "setsid",             no_argument,        NULL,               'S' },  // Call setsid() to make llgs run in its own session.
     { NULL,                 0,                  NULL,               0   }
 };
 
 
 //----------------------------------------------------------------------
case SIGHUP:
         break;
     }
 }
 #endif // #ifndef _WIN32
 
 static void
 display_usage (const char *progname, const char* subcommand)
 {
-    fprintf(stderr, "Usage:\n  %s %s [--log-file log-file-path] [--log-flags flags] [--lldb-command command]* [--platform platform_name] [--setsid] [--named-pipe named-pipe-path] [--native-regs] [--attach pid] [[HOST]:PORT] "
+    fprintf(stderr, "Usage:\n  %s %s "
+            "[--log-file log-file-path] "
+            "[--log-flags flags] "
+            "[--lldb-command command]* "
+            "[--platform platform_name] "
+            "[--setsid] "
+            "[--named-pipe named-pipe-path] "
+            "[--native-regs] "
+            "[--attach pid] "
+            "[[HOST]:PORT] "
             "[-- PROGRAM ARG1 ARG2 ...]\n", progname, subcommand);
     exit(0);
 }
 
 static void
 dump_available_platforms (FILE *output_file)
 {
     fprintf (output_file, "Available platform plugins:\n");

 JoinListenThread ()
 {
     if (s_listen_thread.IsJoinable())
         s_listen_thread.Join(nullptr);
     return true;
 }
 
 Error
-writePortToPipe (const char *const named_pipe_path, const uint16_t port)
+WritePortToPipe(Pipe port_pipe, const uint16_t port)
+{
+    char port_str[64];
+    const auto port_str_len = ::snprintf(port_str, sizeof(port_str), "%u", port);
+
+    size_t bytes_written = 0;
+    // Write the port number as a C string with the NULL terminator.
+    return port_pipe.Write(port_str, port_str_len + 1, bytes_written);
+}
+
+Error
+writePortToPipe(const char *const named_pipe_path, const uint16_t port)
 {
     Pipe port_name_pipe;
     // Wait for 10 seconds for pipe to be opened.
-    auto error = port_name_pipe.OpenAsWriterWithTimeout (named_pipe_path, false, std::chrono::microseconds (10 * 1000000));
-    if (error.Fail ())
+    auto error = port_name_pipe.OpenAsWriterWithTimeout(named_pipe_path, false,
+            std::chrono::seconds{10});
+    if (error.Fail())
         return error;
+    return WritePortToPipe(port_name_pipe, port);
+}
 
-    char port_str[64];
-    const auto port_str_len = ::snprintf (port_str, sizeof (port_str), "%u", port);
-
-    size_t bytes_written = 0;
-    // Write the port number as a C string with the NULL terminator.
-    return port_name_pipe.Write (port_str, port_str_len + 1, bytes_written);
+Error
+writePortToPipe(int unnamed_pipe_fd, const uint16_t port)
+{
+    Pipe port_pipe;
+    // Wait for 10 seconds for pipe to be opened.
+    auto error = port_pipe.CreateWithFD(Pipe::kInvalidDescriptor, unnamed_pipe_fd);
+    if (error.Fail())
+        return error;
+    return WritePortToPipe(port_pipe, port);
 }
 
 void
-ConnectToRemote (GDBRemoteCommunicationServerLLGS &gdb_server, bool reverse_connect, const char *const host_and_port, const char *const progname, const char *const subcommand, const char *const named_pipe_path)
+ConnectToRemote(GDBRemoteCommunicationServerLLGS &gdb_server,
+        bool reverse_connect, const char *const host_and_port,
+        const char *const progname, const char *const subcommand,
+        const char *const named_pipe_path, int unnamed_pipe_fd)
 {
     Error error;
 
     if (host_and_port && host_and_port[0])
     {
         // Parse out host and port.
         std::string final_host_and_port;
         std::string connection_host;
if (host_and_port && host_and_port[0])
                     }
                 }
                 else
                 {
                     fprintf (stderr, "unable to get the bound port for the listening connection\n");
                 }
             }
 
+            // If we have an unnamed pipe to write the port number back to, do that now.
+            if (unnamed_pipe_fd >= 0 && connection_portno == 0)
+            {
+                const uint16_t bound_port = s_listen_connection_up->GetListeningPort(10);
+                if (bound_port > 0)
+                {
+                    error = writePortToPipe(unnamed_pipe_fd, bound_port);
+                    if (error.Fail())
+                    {
+                        fprintf(stderr, "failed to write to the unnamed pipe: %s",
+                                error.AsCString());
+                    }
+                }
+                else
+                {
+                    fprintf(stderr, "unable to get the bound port for the listening connection\n");
+                }
+            }
+
             // Join the listener thread.
             if (!JoinListenThread ())
             {
                 fprintf (stderr, "failed to join the listener thread\n");
                 display_usage (progname, subcommand);
                 exit (1);
             }
 
#endif
     int long_option_index = 0;
     StreamSP log_stream_sp;
     Args log_args;
     Error error;
     int ch;
     std::string platform_name;
     std::string attach_target;
     std::string named_pipe_path;
+    int unnamed_pipe_fd = -1;
     bool reverse_connect = false;
 
     lldb::DebuggerSP debugger_sp = Debugger::CreateInstance ();
 
     debugger_sp->SetInputFileHandle(stdin, false);
     debugger_sp->SetOutputFileHandle(stdout, false);
     debugger_sp->SetErrorFileHandle(stderr, false);
 
while ((ch = getopt_long_only(argc, argv, short_options.c_str(), g_long_options, &long_option_index)) != -1)
                 lldb_commands.push_back(optarg);
             break;
 
         case 'p': // platform name
             if (optarg && optarg[0])
                 platform_name = optarg;
             break;
 
-        case 'P': // named pipe
+        case 'N': // named pipe
             if (optarg && optarg[0])
                 named_pipe_path = optarg;
             break;
 
+        case 'U': // unnamed pipe
+            if (optarg && optarg[0])
+                unnamed_pipe_fd = StringConvert::ToUInt32(optarg, -1);
+
         case 'r':
             // Do nothing, native regs is the default these days
             break;
 
         case 'R':
             reverse_connect = true;
             break;
 
#endif
     if (!attach_target.empty ())
         handle_attach (gdb_server, attach_target);
     else if (argc > 0)
         handle_launch (gdb_server, argc, argv);
 
     // Print version info.
     printf("%s-%s", LLGS_PROGRAM_NAME, LLGS_VERSION_STR);
 
-    ConnectToRemote (gdb_server, reverse_connect, host_and_port, progname, subcommand, named_pipe_path.c_str ());
+    ConnectToRemote(gdb_server, reverse_connect,
+                    host_and_port, progname, subcommand,
+                    named_pipe_path.c_str(), unnamed_pipe_fd);
 
     fprintf(stderr, "lldb-server exiting...\n");
 
     return 0;
 }