[Backtracing][Linux] Tidy a few things up after review.

Mike and Max made various helpful suggestions, so I've added and updated
various comments and amended the code to cope with partial reads and
writes.

rdar://110261430

Author: al45tair

include/swift/Runtime/CrashInfo.h

@@ -27,34 +27,62 @@ namespace backtrace {
 extern "C" {
 #endif
 
+// Note: The "pointers" below are pointers in a different process's address
+//       space, which might not even share our bitness.  That is why they are
+//       `uint64_t`s, rather than pointers or `uintptr_t`s.
+
+// The address of this structure in memory is passed to swift-backtrace.
 struct CrashInfo {
+  // The thread ID for the crashing thread.
   uint64_t crashing_thread;
+
+  // The signal number corresponding to this crash.
   uint64_t signal;
+
+  // The fault address.
   uint64_t fault_address;
 
 #ifdef __APPLE__
+  // Points to the mcontext_t structure for the crashing thread; other
+  // threads' contexts can be recovered using Mach APIs later.
   uint64_t mctx;
 #elif defined(__linux__)
+  // The head of the thread list; points at a "struct thread" (see below).
   uint64_t thread_list;
 #endif
 };
 
 #ifdef __linux__
 
+// A memory server request packet.
 struct memserver_req {
+  // Address to read.
   uint64_t addr;
+
+  // Number of bytes to read.
   uint64_t len;
 };
 
+// A memory server response packet.
 struct memserver_resp {
+  // Address that was read from.
   uint64_t addr;
+
+  // Number of bytes, *or* negative to indicate an error.
   int64_t  len;
-  /* Then len bytes of data */
+
+  // Followed by len bytes of data if len > 0
 };
 
+// Holds details of a running thread.
 struct thread {
+  // Points at the next thread.
   uint64_t next;
+
+  // The thread ID for this thread.
   int64_t  tid;
+
+  // Points to the Linux ucontext_t structure.
   uint64_t uctx;
 };
 

stdlib/public/runtime/Backtrace.cpp

@@ -857,23 +857,26 @@ _swift_isThunkFunction(const char *mangledName) {
 /// @param mangledName is the symbol name to be demangled.
 /// @param mangledNameLength is the length of this name.
 /// @param outputBuffer is a pointer to a buffer in which to place the result.
-/// @param outputBufferSize points to a variable that will be filled in with
-/// the size of the allocated buffer (NOT the length of the result).
+/// @param outputBufferSize points to a variable that contains the size of the
+/// output buffer.
 /// @param status returns the status codes defined in the C++ ABI.
 ///
-/// If outputBuffer and outputBufferSize are both nullptr, this function will
-/// allocate memory for the result using malloc().
-///
-/// If outputBuffer is nullptr but outputBufferSize is not, the function will
-/// fill outputBufferSize with the required buffer size and return nullptr.
+/// If outputBuffer is nullptr, the function will allocate memory for the
+/// result using malloc().  In this case, outputBufferSize may be nullptr;
+/// if it is *not* nullptr, it will be set to the size of buffer that was
+/// allocated.  This is not necessarily the length of the string (it may be
+/// somewhat higher).
 ///
 /// Otherwise, the result will be written into the output buffer, and the
 /// size of the result will be written into outputBufferSize.  If the buffer
 /// is too small, the result will be truncated, but outputBufferSize will
 /// still be set to the number of bytes that would have been required to
 /// copy out the full result (including a trailing NUL).
 ///
-/// @returns `true` if demangling was successful.
+/// The unusual behaviour here is a consequence of the way the C++ ABI's
+/// demangling function works.
+///
+/// @returns a pointer to the output if demangling was successful.
 SWIFT_RUNTIME_STDLIB_SPI char *
 _swift_backtrace_demangle(const char *mangledName,
                           size_t mangledNameLength,
@@ -882,6 +885,11 @@ _swift_backtrace_demangle(const char *mangledName,
                           int *status) {
   llvm::StringRef name = llvm::StringRef(mangledName, mangledNameLength);
 
+  // You must provide buffer size if you're providing your own output buffer
+  if (outputBuffer && !outputBufferSize) {
+    return nullptr;
+  }
+
   if (Demangle::isSwiftSymbol(name)) {
     // This is a Swift mangling
     auto options = DemangleOptions::SimplifiedUIDemangleOptions();
@@ -926,6 +934,8 @@ _swift_backtrace_demangle(const char *mangledName,
       ::memcpy(outputBuffer, result, toCopy);
       outputBuffer[toCopy] = '\0';
 
+      free(result);
+
       *status = 0;
       return outputBuffer;
     }

stdlib/public/runtime/CrashHandlerLinux.cpp

@@ -67,22 +67,44 @@ int  memserver_start();
 int  memserver_entry(void *);
 bool run_backtracer(int fd);
 
-int safe_read(int fd, void *buf, size_t len) {
-  int ret;
-  do {
-    ret = read(fd, buf, len);
-  } while (ret < 0 && errno == EINTR);
+ssize_t safe_read(int fd, void *buf, size_t len) {
+  uint8_t *ptr = (uint8_t *)buf;
+  uint8_t *end = ptr + len;
+  ssize_t total = 0;
+
+  while (ptr < end) {
+    ssize_t ret;
+    do {
+      ret = read(fd, buf, len);
+    } while (ret < 0 && errno == EINTR);
+    if (ret < 0)
+      return ret;
+    total += ret;
+    ptr += ret;
+    len -= ret;
+  }
 
-  return ret;
+  return total;
 }
 
-int safe_write(int fd, const void *buf, size_t len) {
-  int ret;
-  do {
-    ret = write(fd, buf, len);
-  } while (ret < 0 && errno == EINTR);
+ssize_t safe_write(int fd, const void *buf, size_t len) {
+  const uint8_t *ptr = (uint8_t *)buf;
+  const uint8_t *end = ptr + len;
+  ssize_t total = 0;
+
+  while (ptr < end) {
+    ssize_t ret;
+    do {
+      ret = write(fd, buf, len);
+    } while (ret < 0 && errno == EINTR);
+    if (ret < 0)
+      return ret;
+    total += ret;
+    ptr += ret;
+    len -= ret;
+  }
 
-  return ret;
+  return total;
 }
 
 CrashInfo crashInfo;
@@ -195,8 +217,7 @@ handle_fatal_signal(int signum,
   // Start the memory server
   int fd = memserver_start();
 
-  /* Start the backtracer; this will suspend the process, so there's no need
-     to try to suspend other threads from here. */
+  // Actually start the backtracer
   run_backtracer(fd);
 
 #if !MEMSERVER_USE_PROCESS
@@ -276,15 +297,15 @@ getdents(int fd, void *buf, size_t bufsiz)
 }
 
 /* Stop all other threads in this process; we do this by establishing a
-   signal handler for SIGUSR1, then iterating through the threads sending
-   SIGUSR1.
+   signal handler for SIGPROF, then iterating through the threads sending
+   SIGPROF.
 
    Finding the other threads is a pain, because Linux has no actual API
    for that; instead, you have to read /proc.  Unfortunately, opendir()
    and readdir() are not async signal safe, so we get to do this with
    the getdents system call instead.
 
-   The SIGUSR1 signals also serve to build the thread list. */
+   The SIGPROF signals also serve to build the thread list. */
 void
 suspend_other_threads(struct thread *self)
 {