[PATCH] D36032: [sanitizer_common] Fuchsia-specific symbolizer

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_stacktrace_printer.cc:20
 
+// sanitizer_symbolizer_fuchsia.cc implements these differently for Fuchsia.
+#if !SANITIZER_FUCHSIA
----------------
mcgrathr wrote:
> vitalybuka wrote:
> > I don't think this comment is necessary
> You really prefer few comments???
Unless comment explains something obvious.


================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_fuchsia.cc:32
+// This is used by UBSan for type names, and by ASan for global variable names.
+constexpr const char *kFormatDemangle = "{{{symbol:%s}}}";
+constexpr uptr kFormatDemangleMax = 1024;  // Arbitrary.
----------------
mcgrathr wrote:
> vitalybuka wrote:
> > Regular array is shorter
> > ```
> > const char kFormatDemangle[] = 
> > ```
> constexpr is correct for this use.  The compiler should know that it's just a vanilla string constant and doesn't need to be an independent link-time object.
lgtm


================
Comment at: lib/sanitizer_common/sanitizer_symbolizer_fuchsia.cc:47
+// Symbolizer class supported linker initialization.
+Symbolizer *Symbolizer::GetOrInit() {
+  SpinMutexLock l(&init_mu_);
----------------
mcgrathr wrote:
> vitalybuka wrote:
> > Why not just override PlatformInit?
> Because then GetOrInit would be the only thing from sanitizer_symbolizer_libcdep.cc actually used and it's cleaner to #if out the entire file.
> I'd be happy to move the generic GetOrInit from sanitizer_symbolizer_libcdep.cc to sanitizer_symbolizer.cc if you prefer that.
I am OK to keep it as is


https://reviews.llvm.org/D36032