[PATCH] [compiler-rt] atos symbolizer for OS X

[Alexander Potapenko]

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:24
@@ +23,3 @@
+bool AtosSymbolizer::StartSubprocessIfNotStarted() {
+  if (fd_to_child_)
+    return true;
----------------
Please consider using fd_t for |fd_to_child_| and kInvalidFd for invalid fd.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:31
@@ +30,3 @@
+  // Use forkpty to disable buffering in the new terminal.
+  int pid = forkpty(&fd, 0, 0, 0);
+  if (pid == -1) {
----------------
This blew my mind out ;)
I'm curious whether using the same fd to write commands to the symbolizer and read the output actually works.
Is it in any sense better than having ye olde pipes or a socketpair like in StartSymbolizerSubprocess() in sanitizer_symbolizer_posix_libcdep.cc?

Anyway, I think we need to use the same code to spawn the subprocesses for both the addr2line and atos symbolizers. Not sure which version is better.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:33
@@ +32,3 @@
+  if (pid == -1) {
+    // Fork() failed.
+    Report("WARNING: failed to fork external symbolizer (errno: %d)\n", errno);
----------------
s/Fork/fork

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:34
@@ +33,3 @@
+    // Fork() failed.
+    Report("WARNING: failed to fork external symbolizer (errno: %d)\n", errno);
+    return false;
----------------
I think you need to ensure fd isn't spoiled by the failed fork().

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:56
@@ +55,3 @@
+
+static const char *ExtractToken(const char *str, const char *delimiter,
+                                char **result) {
----------------
How about moving ExtractToken to a common file?
There's a similar function in sanitizer_symbolizer_posix_libcdep.cc

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:103
@@ +102,3 @@
+    uptr just_read =
+        internal_read(fd_to_child_, buffer_ + length, kBufferSize - length - 1);
+    if (just_read == 0 || just_read == (uptr)-1) {
----------------
I've mixed feelings about internal_read() returning a uptr, but don't think we need to clean it up in this CL.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.h:10
@@ +9,3 @@
+//
+// This file is shared between AddressSanitizer and ThreadSanitizer
+// run-time libraries.
----------------
This is not accurate anymore, because we've more than two tools (although only ASan supports OSX so far)
How about "This file is shared between sanitizer tools run-time libraries"?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.h:27
@@ +26,3 @@
+#include <util.h>
+#include <sys/errno.h>
+
----------------
Please fix the include order (standard headers should be sorted alphabetically)

http://reviews.llvm.org/D6588

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/