[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