This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer_common] Fuchsia-specific symbolizer
ClosedPublic

Authored by mcgrathr on Jul 28 2017, 3:44 PM.

Details

Summary

Fuchsia doesn't support built-in symbolization per se at all.
Instead, it always emits a Fuchsia-standard "symbolizer markup"
format that makes it possible for a post-processing filter to
massage the logs into symbolized format. Hence, it does not
support user-specified formatting options for backtraces or other
symbolization.

Diff Detail

Repository
rL LLVM

Event Timeline

mcgrathr created this revision.Jul 28 2017, 3:44 PM
vitalybuka added inline comments.Jul 28 2017, 3:59 PM
lib/sanitizer_common/sanitizer_stacktrace_printer.cc
128

How about having implementation of RenderFrame and RenderData in sanitizer_symbolizer_fuchsia.cc?

#if !FUCHSIA
void RenderFrame(InternalScopedString *buffer, const char *format, int frame_no,

const AddressInfo &info, bool vs_style,
const char *strip_path_prefix, const char *strip_func_prefix) {

...

}

void RenderData(InternalScopedString *buffer, const char *format,
....
}
#endif

mcgrathr updated this revision to Diff 108740.Jul 28 2017, 4:14 PM
mcgrathr marked an inline comment as done.

RenderData and RenderFrame in sanitizer_symbolizer_fuchsia.cc

vitalybuka accepted this revision.Jul 28 2017, 4:37 PM
vitalybuka added inline comments.
lib/sanitizer_common/sanitizer_stacktrace_printer.cc
20

I don't think this comment is necessary

lib/sanitizer_common/sanitizer_symbolizer_fuchsia.cc
33

Regular array is shorter

const char kFormatDemangle[] =
48

Why not just override PlatformInit?

This revision is now accepted and ready to land.Jul 28 2017, 4:37 PM
mcgrathr added inline comments.Jul 28 2017, 4:55 PM
lib/sanitizer_common/sanitizer_stacktrace_printer.cc
20

You really prefer few comments???

lib/sanitizer_common/sanitizer_symbolizer_fuchsia.cc
33

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.

48

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.

vitalybuka added inline comments.Jul 28 2017, 5:03 PM
lib/sanitizer_common/sanitizer_stacktrace_printer.cc
20

Unless comment explains something obvious.

lib/sanitizer_common/sanitizer_symbolizer_fuchsia.cc
33

lgtm

48

I am OK to keep it as is

alekseyshl added inline comments.Jul 31 2017, 11:35 AM
lib/sanitizer_common/sanitizer_symbolizer_fuchsia.cc
48

Please do move GetOrInit to sanitizer_symbolizer.cc and implement PlatformInit and exclude sanitizer_symbolizer_libcdep.cc from Fuchsia build, that #if-ing the entire file is ugly.

mcgrathr added inline comments.Aug 1 2017, 12:17 PM
lib/sanitizer_common/sanitizer_symbolizer_fuchsia.cc
48

I've moved GetOrInit and implemented PlatformInit.

I didn't change to cmake conditionalization instead of #if around the whole file because that would be inconsistent with the rest of the source code, which uses #if around the whole file for other cases like this one.

mcgrathr updated this revision to Diff 109174.Aug 1 2017, 12:20 PM

I've moved GetOrInit and implemented PlatformInit.

This is rebased and ready to land after D36031 lands.
Please land it for me!

Thanks,
Roland

vitalybuka requested changes to this revision.Aug 1 2017, 3:33 PM

ninja check-sanitizer

fails Sanitizer-x86_64-Test-Nolibc test
llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer.cc:27: error: undefined reference to '__sanitizer::Symbolizer::PlatformInit()'

This revision now requires changes to proceed.Aug 1 2017, 3:33 PM
mcgrathr updated this revision to Diff 109241.Aug 1 2017, 3:48 PM
mcgrathr edited edge metadata.

Moved GetOrInit back to sanitizer_symbolizer_libcdep.cc, hoisted up out of #if !SANITIZER_FUCHSIA.

This revision was automatically updated to reflect the committed changes.