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

[Alexander Potapenko]

================
Comment at: lib/sanitizer_common/CMakeLists.txt:30
@@ -29,2 +29,3 @@
   sanitizer_symbolizer_win.cc
+  sanitizer_symbolizer_mac.cc
   sanitizer_tls_get_addr.cc
----------------
Please swap this line with the previous one.

================
Comment at: lib/sanitizer_common/sanitizer_common.cc:293
@@ +292,3 @@
+// Same as ExtractToken, but converts extracted token to integer.
+const char *ExtractInt(const char *str, const char *delims, int *result) {
+  char *buff;
----------------
As far as I can tell, both ExtractInt and ExtractUptr are used to extract unsigned integers. Why use both (and deal with the potential overflow situations)?

================
Comment at: lib/sanitizer_common/sanitizer_common.cc:315
@@ +314,3 @@
+// be multiple characters long.
+const char *ExtractUpToDelimiter(const char *str, const char *delimiter,
+                                 char **result) {
----------------
How 'bout "ExtractTokenUpToDelimiter" since this function is extracting tokens, not ints or uptrs?

================
Comment at: lib/sanitizer_common/sanitizer_common.cc:325
@@ +324,3 @@
+  if (*prefix_end != '\0')
+    prefix_end += internal_strlen(delimiter);
+  return prefix_end;
----------------
Isn't that just prefix_len?

================
Comment at: lib/sanitizer_common/sanitizer_common.h:71
@@ -70,1 +70,3 @@
 
+const char *ExtractToken(const char *str, const char *delims, char **result);
+const char *ExtractInt(const char *str, const char *delims, int *result);
----------------
This group of helpers needs at least a brief comment.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.cc:29
@@ +28,3 @@
+  //   myfunction (in library.dylib) + 0x1fe
+  //   0xdeadbeef (in library.dylib)
+  //   0xdeadbeef
----------------
In this case you're assigning "0xdeadbeef" to res->info.function, is that intentional?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:170
@@ -300,3 +169,3 @@
 
-  static const uptr kBufferSize = 16 * 1024;
-  char buffer_[kBufferSize];
+static void POSIXParseSymbolizePCOutput(const char *str, SymbolizedStack *res) {
+  uptr orig_addr = res->info.address;
----------------
A comment may give better understanding of what you're parsing here.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:205
@@ +204,3 @@
+    line_info = ExtractInt(line_info, ":", &info->line);
+    line_info = ExtractInt(line_info, "", &info->column);
+    InternalFree(file_line_info);
----------------
Is the column part always present? I think for addr2line it is not. In that case, will info->column be initialized properly?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:221
@@ -303,7 +220,3 @@
 
-  static const uptr kMaxTimesRestarted = 5;
-  static const int kSymbolizerStartupTimeMillis = 10;
-  uptr times_restarted_;
-  bool failed_to_start_;
-  bool reported_invalid_path_;
-};
+static void POSIXParseSymbolizeDataOutput(const char *str, uptr addr,
+                                          DataInfo *info) {
----------------
I think this function (and the other one) has little to do with POSIX, despite it's in a *_posix_*.cc file.
Better remove "POSIX" from the name.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:226
@@ +225,3 @@
+  str = ExtractUptr(str, "\n", &info->size);
+  info->start += (addr - info->module_offset); // Add the module base address.
+}
----------------
Two spaces before the comment, please (here and in the tests below)

================
Comment at: test/asan/TestCases/closed-fds.cc:2
@@ +1,3 @@
+// Check that when the program closed its std(in|out|err), running the external
+// symbolizer still works.
+
----------------
Wonder if we want/should check that exactly the required symbolizer is being used (e.g. print its name under verbosity=2)

http://reviews.llvm.org/D6588

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