[PATCH] [compiler-rt] atos and dladdr symbolizers for OS X

[Alexey Samsonov]

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:57
@@ +56,3 @@
+  bool StartSymbolizerSubprocess() override {
+    if (!FileExists(path_)) {
+      if (!reported_invalid_path_) {
----------------
I don't like that this function essentially copies so much of the base class code. Do you have a good understanding of why we need to use one code to run llvm-symbolizer on Mac and different code (forkpty etc.) to run atos on Mac? Can we instead make certain bits of StartSymbolizerProcess() platform-specific?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:66
@@ +65,3 @@
+    if (fd_to_child_ != kInvalidFd) return true;
+    if (fork_failed_) return false;
+
----------------
Why do you need this?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:115
@@ +114,3 @@
+
+    char pid_str[16] = {0};
+    internal_snprintf(pid_str, sizeof(pid_str), "%d", parent_pid_);
----------------
no need to fill with zeroes, snprintf should do the right thing.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:125
@@ +124,3 @@
+
+bool ParseCommandOutput(const char *str, SymbolizedStack *res) {
+  // Trim ending newlines.
----------------
static?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:137
@@ +136,3 @@
+
+  if (internal_strstr(trim, "atos cannot examine process") ||
+      internal_strstr(trim, "unable to get permission to examine process") ||
----------------
factor out these constants and check in a helper function.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:169
@@ +168,3 @@
+AtosSymbolizer::AtosSymbolizer(const char *path)
+    : process_(new AtosSymbolizerProcess(path, getpid())) {}
+
----------------
Wait, shouldn't AtosSymbolizerProcess be created via InternalAllocator?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:177
@@ +176,3 @@
+  if (!result) {
+    process_->Shutdown();
+    return false;
----------------
Why do you need this here? If you want to avoid printing multiple error messages, you can just drop the reference to process_

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:187
@@ +186,3 @@
+
+bool DlAddrSymbolizer::SymbolizePC(uptr addr, SymbolizedStack *stack) {
+  return false;
----------------
See above - I don't think we need to add *any* implementation for non-Mac platforms.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.h:1
@@ +1,2 @@
+//===-- sanitizer_symbolizer_mac.h ------------------------------*- C++ -*-===//
+//
----------------
Add this file to CMakeLists.txt

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.h:18
@@ +17,3 @@
+
+#include "sanitizer_symbolizer_internal.h"
+
----------------
Do you really need the declaration of DlAddrSymbolizer and AtosSymbolizer on non-Mac? Maybe, we could instead protect the whole .h and .cc file with 
  #if SANITIZER_MAC
directive? E.g. you only provide AtosSymbolizerProcess under SANITIZER_MAC anyway.

http://reviews.llvm.org/D6588

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