853851 - Transition some breakpad logging to BPLOG

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Julian Seward [:jseward]]

Transition some breakpad profiling from fprintf(stderr, ...) to BPLOG
so it works sanely for SPS running on Android.  Also add a bit more
logging to src/common/linux/dump_symbols.cc.

Comment 1

[Julian Seward [:jseward]]

Attachment: Proposed patch

Comment 2

[Julian Seward [:jseward]]

Attachment: SPS minor cleanups, assuming the breakpad patch is acceptable

This was a stopgap fix whilst breakpad didn't have adequate logging in
dump_symbols.cc.  The other patch on this bug adds that, though, and
does it better.  In which case the stuff in local_debug_info_symbolizer.cc
is redundant.

Comment 3

[Julian Seward [:jseward]]

Attachment: Proposed patch, take 2

Also moves pathname_stripper.{cc,h} and pathname_stripper_unittest.cc
from processor/ to common/, so as to work around link failures when
--disable-crashreporter is not given.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 729021 [details] [diff] [review]
Proposed patch, take 2

Review of attachment 729021 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me. I'd like to get this up for upstream review just to see what Mark thinks of the idea of using the existing logging code in the symbol dumping code.

::: toolkit/crashreporter/google-breakpad/src/common/dwarf_cfi_to_module.cc
@@ +233,5 @@
>  }
>  
>  void DwarfCFIToModule::Reporter::UnnamedRegister(size_t offset, int reg) {
> +  BPLOG(INFO) << file_ << ", section '" << section_ 
> +    << "': the call frame entry at offset 0x" 

nit: you've got some trailing spaces in this file.

::: toolkit/crashreporter/google-breakpad/src/common/linux/dump_symbols.cc
@@ +528,5 @@
>    typedef typename ElfClass::Addr Addr;
>    typedef typename ElfClass::Phdr Phdr;
>    typedef typename ElfClass::Shdr Shdr;
>  
> +  BPLOG(INFO) << "";

Any reason for this blank line?

::: toolkit/crashreporter/google-breakpad/src/processor/logging.cc
@@ +48,5 @@
>  #ifdef _WIN32
>  #define snprintf _snprintf
>  #endif
>  
> +#ifdef ANDROID

You want __ANDROID__ here and below.

@@ +86,5 @@
>  LogStream::~LogStream() {
> +#ifdef ANDROID
> +  __android_log_print(ANDROID_LOG_ERROR,
> +                      "Profiler", "%s", str_.str().c_str());
> +#else

I'm sure there's a smarter way of handling this, like implementing our own stringstream or ostream subclass and handling << endl to do the log print, and then changing the #defines in logging.h to use our new class, but that seems like a lot more work.

::: toolkit/crashreporter/google-breakpad/src/processor/logging.h
@@ +100,2 @@
>    template<typename T> std::ostream& operator<<(const T &t) {
> +    return str_ << t;

I'm not sure it matters much in practice, but would it be better to ifdef this so the behavior doesn't change on non-Android?

Comment 5

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/636cfcab9682

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/636cfcab9682