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

Alexey Samsonov vonosmas at gmail.com
Tue Dec 30 17:25:57 PST 2014


If you need to refactor some interfaces or move code around in order to implement Mac-specific symbolizers, let's start with small patches doing that, which introduce no behavior changes.


================
Comment at: lib/sanitizer_common/sanitizer_common.h:75
@@ +74,3 @@
+// to the next characted after the found delimiter.
+const char *ExtractToken(const char *str, const char *delims, char **result);
+const char *ExtractUptr(const char *str, const char *delims, uptr *result);
----------------
I'd prefer to have these function in sanitizer_symbolizer.(h|cc), as for now they are not used anywhere else.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer.h:145
@@ +144,3 @@
+  virtual bool SymbolizePC(uptr addr, SymbolizedStack *stack) {
+    UNIMPLEMENTED();
+  }
----------------
I don't like that in some concrete classes (e.g. LibbacktraceSymbolizer) require that certain fields of "stack" structure are filled by the caller.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer.h:154
@@ +153,3 @@
+  static const uptr kBufferSize = 16 * 1024;
+  char buffer_[kBufferSize];
+};
----------------
This buffer has nothing to do with the interface, it is an implementation detail of specific concrete classes.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer.h:168
@@ +167,3 @@
+        reported_invalid_path_(false) {
+    CHECK(path_);
+    CHECK_NE(path_[0], '\0');
----------------
Please move implementation to .cc files.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_libbacktrace.cc:159
@@ -158,4 +158,3 @@
 
-SymbolizedStack *LibbacktraceSymbolizer::SymbolizeCode(uptr addr,
-                                                       const char *module_name,
-                                                       uptr module_offset) {
+bool LibbacktraceSymbolizer::SymbolizePC(uptr addr, SymbolizedStack *stack) {
+  const char *module_name = stack->info.module;
----------------
This function is completely wrong. First of all, it should return bool now, not SymbolizedStack*. Then, "SymbolizedStack*" argument you pass to this function is leaked, so is its module_name. It seems that the "stack" argument is only required to pass in module_name/module_offset, and serves no actual purpose by itself. It means that the interface is broken.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_libbacktrace.h:35
@@ -34,4 +34,3 @@
 
-  SymbolizedStack *SymbolizeCode(uptr addr, const char *module_name,
-                                 uptr module_offset);
+  bool SymbolizePC(uptr addr, SymbolizedStack *stack);
 
----------------
add override keywords for method overloads please.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_mac.h:46
@@ +45,3 @@
+  explicit AtosSymbolizer(const char *path)
+      : process_(new AtosSymbolizerProcess(path, getpid())) {}
+
----------------
"new"?!

No, please don't call system allocator in the symbolizer code.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:28
@@ -26,2 +27,3 @@
 
+#include <dlfcn.h>
 #include <errno.h>
----------------
Does this header exist on all POSIX systems?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:33
@@ -30,2 +32,3 @@
 #include <unistd.h>
+#include <util.h>
 
----------------
Does this header exist on all POSIX systems?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:509
@@ +508,3 @@
+    Dl_info info;
+    int result = dladdr((const void *)addr, &info);
+    if (!result)
----------------
dladdr may not be available on some platforms. I'd suggest to use it on Mac only for now.

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:620
@@ -656,14 +619,3 @@
     mu_.CheckLocked();
-    // First, try to use internal symbolizer.
-    if (internal_symbolizer_) {
-      SymbolizerScope sym_scope(this);
-      return internal_symbolizer_->SendCommand(is_data, module_name,
-                                               module_offset);
-    }
-    // Otherwise, fall back to external symbolizer.
-    if (external_symbolizer_) {
-      SymbolizerScope sym_scope(this);
-      return external_symbolizer_->SendCommand(is_data, module_name,
-                                               module_offset);
-    }
-    return 0;
+    if (internal_symbolizer_)
+      return internal_symbolizer_;
----------------
Wait, LibbacktraceSymbolizer and DladdrSymbolizer also implement SymbolizerInterface, why don't we return them here?

================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:699
@@ +698,3 @@
+    if (common_flags()->verbosity >= 2)
+      Report("Symbolizing is disabled.\n");
+    return new(symbolizer_allocator_) POSIXSymbolizer(0, 0, 0, 0);
----------------
We have VReport for these purposes.

http://reviews.llvm.org/D6588

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






More information about the llvm-commits mailing list