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

[Alexey Samsonov]

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer.h:144
@@ +143,3 @@
+  // __cxa_pure_virtual might be unavailable.
+  virtual char *SendCommand(uptr addr, bool is_data, const char *module_name,
+                            uptr module_offset) {
----------------
Looks like some overrides need one half of the arguments (module_name/module_offset), and some need another (addr). This kind of interface is not really convenient.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer.h:149
@@ +148,3 @@
+
+  virtual void ParseSymbolizedStackFrames(const char *str,
+                                          SymbolizedStack *res) {
----------------
This function has nothing to do with `ExternalSymbolizerInterface`

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:94
@@ +93,3 @@
+  uptr length = internal_snprintf(buffer_, kBufferSize, "0x%zx\n", addr);
+  uptr write_len = internal_write(fd_to_child_, buffer_, length);
+  if (write_len == 0 || write_len == (uptr)-1) {
----------------
Note: you need to use internal_iserror() for write/read syscalls. If we don't have them for existing code - this is most likely a bug.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.h:21
@@ +20,3 @@
+
+#include "sanitizer_allocator_internal.h"
+#include "sanitizer_common.h"
----------------
Why do you need it?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.h:33
@@ +32,3 @@
+ public:
+  explicit AtosSymbolizer(const char *atos_path) : atos_path_(atos_path) {}
+
----------------
don't you need to initialize fd_to_child_ here?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:692
@@ -671,1 +691,3 @@
 
+  void ParseSymbolizedStackFrames(const char *str, SymbolizedStack *res) {
+    mu_.CheckLocked();
----------------
I don't like the way the code is structured: first you call `SendCommand()`, that looks at which symbolizers are available, and dispatches `SendCommand()` to one of them, and then you call `ParseSymbolizedStackFrame()` that does the similar dispatch. I don't believe this is correct. On top of that, there is libbacktrace_symbolizer_, which works in yet different way.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:792
@@ +791,3 @@
+#if SANITIZER_MAC
+        } else if (const char *atos_path = FindPathToBinary("atos")) {
+          external_symbolizer =
----------------
I'd prefer having this under
   if (SANITIZER_MAC)
, and all the AtosSymbolizer code not #ifdef'ed out on Linux - this way it would be easier to check that we don't break one platform while modifying the code for another.

http://reviews.llvm.org/D6588

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