802240 - Figure out how to get Android library data from the APK to Breakpad for SPS profiling

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[(not currently active) Ted Mielczarek]

In Julian's patch to make SPS use Breakpad for unwinding we're relying on Breakpad being able to load libraries so it can read symbols out of them. This only works if you have the libraries on disk. For the loading-from-the-APK case we'll need to figure out how to make the library loader and Breakpad meet in the middle to get the symbol data. If we used my "dump symbols from ARM unwind tables" patch and we fixed this then we could profile release Android builds, which would be really cool.

I think the answer here is going to be to expose an API to get the load addresses of the .ARM.exidx and .ARM.extab sections, and to also make sure that we can call directly into the Breakpad methods that convert that data into debug info.

Comment 1

[Mike Hommey [:glandium]]

dl_iterate_phdr can give your the Phdrs, where you can find the EXIDX program headers yourself.

Comment 2

[Julian Seward [:jseward]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #0)
> I think the answer here is going to be to expose an API to get the load
> addresses of the .ARM.exidx and .ARM.extab sections

I'd prefer something a bit more general, which is that Mike's linker
gives us a pair of functions, to mmap and munmap files in the mapped
APK.  Then we can hand those to breakpad and it can use them in
preference to the standard mmap/munmap, to read whatever it wants.
This makes it agnostic to whether we go with EXIDX or CFI or some
combination of both.  It would be used to replace the mmap call in
LoadELF in
toolkit/crashreporter/google-breakpad/src/common/linux/dump_symbols.cc

The functionality can be very restricted compared to the standard
mmap:

* MAP_ANON, MAP_PRIVATE only
* mmap gets to choose where to put it
* zero offset in the file

Also, presumably it would need to identify the file by name rather
than by fd.

Comment 3

[Julian Seward [:jseward]]

Oh, and no partial unmaps necessary.  munmap would only have to handle
unmapping precisely the range that just got mapped (I assume -- unless
breakpad is doing wierd stuff w/ mappings.)

Comment 4

[(not currently active) Ted Mielczarek]

Breakpad simply maps the entire file, pokes at it, and then unmaps it.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Expose an API to mmap the underlying file for a library loaded by faulty.lib

Julian, can you test if this works for you?

You need to define the following functions where you'll be using them:
extern "C" {
  MFBT_API size_t __dl_get_mappable_length(void *handle);
  MFBT_API void *__dl_mmap(void *handle, void *addr, size_t length, off_t offset);
  MFBT_API void __dl_munmap(void *handle, void *addr, size_t length);
}

handle is what you get from dlopen();

Comment 6

[Julian Seward [:jseward]]

Random comment, of no great importance, while reading through the patch:

+void
+CustomElf::MappableMUnmap(void *addr, size_t length) const
+{
+  return mappable->munmap(addr, length);
+}

Inconsistency in return type (void vs implied-not-void?)  Can this
return an error code instead of void, so it's possible to check it
didn't fail?

Comment 7

[Mike Hommey [:glandium]]

As found through irc debugging, this fails because of MappableDeflate::finalize freeing memory.

Comment 8

[Julian Seward [:jseward]]

Attachment: Rollup WIP patch, that does actually appear to work on ARM/Android

Contents of this patch:

* Mike's linker patch from comment 5

* Fixes to the above that Mike told me over irc

* My changes to the SPS and breakpad that hook up to the linker
  changes.  These center around the use of the type AltMapFns to
  abstractify out how breakpad maps objects so as to read from them.
  Basically, local_debug_info_symbolizer.cc creates one of these
  AltMapFns objects that hook up to faulty.lib.  It then hands it
  off to ReadSymbolData, which uses that to do the mapping.

Comment 9

[Julian Seward [:jseward]]

Attachment: Rollup WIP patch, somewhat cleaned up

No functional changes.  Is a minorly cleaned up version of
the previous rollup patch -- some debug printing removed,
and a couple of unrelated hunks deleted.

Comment 10

[Julian Seward [:jseward]]

Comment on attachment 734841 [details] [diff] [review]
Rollup WIP patch, somewhat cleaned up

Ted, what's your view on the breakpad side changes here? 
Specifically, the changes the following 3 files:

toolkit/crashreporter/google-breakpad/src/common/linux/dump_symbols.cc
toolkit/crashreporter/google-breakpad/src/common/linux/dump_symbols.h
tools/profiler/local_debug_info_symbolizer.cc

as summarised in comment 8.  Would this approach be considered acceptable?

Comment 11

[Julian Seward [:jseward]]

(In reply to Mike Hommey [:glandium] from comment #5)
> Created attachment 729470 [details] [diff] [review]
> Expose an API to mmap the underlying file for a library loaded by faulty.lib
> Julian, can you test if this works for you?

This does indeed seem to work, when using the extra three fixes you
told me on irc.  These are marked using //MH-IRC in the rollup patch.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 734841 [details] [diff] [review]
Rollup WIP patch, somewhat cleaned up

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

I think this would be much simpler if you just exposed ReadSymbolDataInternal as a public API, and did the mapping management in local_debug_info_symbolizer.

In fact, you could already just use that API like the unit tests do, since it's not static. (You just have to define a prototype for it because it's not in the header.)

Comment 13

[Julian Seward [:jseward]]

Attachment: Rollup WIP patch, revised as per comment 12

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #12)

Good suggestion.  That makes the patch about 25% shorter and almost
completely non-intrusive for Breakpad.  Could you pls look over:

toolkit/crashreporter/google-breakpad/src/common/linux/dump_symbols.h
tools/profiler/local_debug_info_symbolizer.cc

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 735400 [details] [diff] [review]
Rollup WIP patch, revised as per comment 12

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

This looks a lot better, thanks. I'll land the dump_symbols.h change upstream for you.

Comment 15

[Mike Hommey [:glandium]]

Attachment: Expose an API to mmap the underlying file for a library loaded by faulty.lib

Julian, can you test this? (totally untested locally, besides the fact that it builds)

Comment 16

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #15)
> Created attachment 736304 [details] [diff] [review]
> Expose an API to mmap the underlying file for a library loaded by faulty.lib
> 
> Julian, can you test this? (totally untested locally, besides the fact that
> it builds)

Note, you shouldn't need the __attribute__((weak)) anymore (in fact, you can probably include ElfLoader.h instead of making your own declarations), and the api should now take care of system libraries too (no need for your fallback)

Comment 17

[Mike Hommey [:glandium]]

Attachment: Expose an API to mmap the underlying file for a library loaded by faulty.lib

Comment 18

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #17)
> Created attachment 736433 [details] [diff] [review]
> Expose an API to mmap the underlying file for a library loaded by faulty.lib

Let's already get a review on this. It's kind of awful in that it sucks memory and the restrictions on GetMappableLength and friends are horrible, but it's only a quick hack to have something working. A proper patch will come in a followup.

Comment 19

[Julian Seward [:jseward]]

(In reply to Mike Hommey [:glandium] from comment #15)
> Julian, can you test this? (totally untested locally, besides the fact that
> it builds)

This appears to work OK in the case where it is only used to process
in-APK files.

When passed a non-fully-qualified, not-in-APK file name (eg
"libc.so"), it appears to fail at the __dl_get_mappable_length call --
even though the preceding dlopen call looks like it succeeds:

E/Profiler( 4282): 2013-04-11 20:46:18:
local_debug_info_symbolizer.cc:75: INFO: ReadSymbolData_APK: Unable to
get size for ELF file 'libc.so'

Comment 20

[Julian Seward [:jseward]]

(In reply to Mike Hommey [:glandium] from comment #15)
> Julian, can you test this? (totally untested locally, besides the fact that
> it builds)

If I install the Quit Now extension, and start the profiler, then it
segfaults at exit.  It segfault if I don't start the profiler:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 5781]
0xb00055f2 in ?? () from /home/sewardj/MOZ/JimDB/jimdb/lib/0288504044c06217/system/bin/linker
(gdb) where
#0  0xb00055f2 in ?? () from /home/sewardj/MOZ/JimDB/jimdb/lib/0288504044c06217/system/bin/linker
#1  0xb000593e in ?? () from /home/sewardj/MOZ/JimDB/jimdb/lib/0288504044c06217/system/bin/linker
#2  0x5b77158e in ElfLoader::DebuggerHelper::Remove (this=0x5b785258 <ElfLoader::Singleton+64>, map=0x1c3634c)
    at /home/sewardj/MOZ/MC-11-04-2013/mozglue/linker/ElfLoader.cpp:770
#3  0x5b77186a in ElfLoader::Forget (this=0x5b785218 <ElfLoader::Singleton>, handle=<error reading variable: Cannot access memory at address 0xfffffffe>)
    at /home/sewardj/MOZ/MC-11-04-2013/mozglue/linker/ElfLoader.cpp:416
#4  0x5b7728c2 in CustomElf::~CustomElf (this=0x1c36338, __in_chrg=<optimized out>) at /home/sewardj/MOZ/MC-11-04-2013/mozglue/linker/CustomElf.cpp:237
#5  0x5b772946 in CustomElf::~CustomElf (this=0x1c36338, __in_chrg=<optimized out>) at /home/sewardj/MOZ/MC-11-04-2013/mozglue/linker/CustomElf.cpp:238
#6  0x5b771ed0 in Release (this=<optimized out>) at /home/sewardj/MOZ/MC-11-04-2013/mozglue/linker/ElfLoader.h:230
#7  ReleaseDirectRef (this=<optimized out>) at /home/sewardj/MOZ/MC-11-04-2013/mozglue/linker/ElfLoader.h:153
#8  ElfLoader::~ElfLoader (this=0x5b785218 <ElfLoader::Singleton>, __in_chrg=<optimized out>) at /home/sewardj/MOZ/MC-11-04-2013/mozglue/linker/ElfLoader.cpp:444
#9  0x400b7f84 in __cxa_finalize () from /home/sewardj/MOZ/JimDB/jimdb/lib/0288504044c06217/system/lib/libc.so
#10 0x400b8320 in exit () from /home/sewardj/MOZ/JimDB/jimdb/lib/0288504044c06217/system/lib/libc.so
#11 0x400b8320 in exit () from /home/sewardj/MOZ/JimDB/jimdb/lib/0288504044c06217/system/lib/libc.so
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

Comment 21

[Julian Seward [:jseward]]

> If I install the Quit Now extension, and start the profiler, then it
> segfaults at exit.  It segfault if I don't start the profiler:
                        ^
Duh.  That should be "doesn't segfault".

Comment 22

[Mike Hommey [:glandium]]

Add DEFINES += -DMOZ_DEBUG_LINKER to mozglue/linker/Makefile.in and attach a logcat of GeckoLinker near the crash.

Comment 23

[Julian Seward [:jseward]]

(In reply to Julian Seward from comment #19)
> E/Profiler( 4282): 2013-04-11 20:46:18:
> local_debug_info_symbolizer.cc:75: INFO: ReadSymbolData_APK: Unable to
> get size for ELF file 'libc.so'

> [23:31] <glandium> sewardj: edit mozglue/linker/ElfLoader.cpp, 
  find /system/lib and add a / (/system/lib/)

-    systemPath = "/system/lib";
+    systemPath = "/system/lib/";

This doesn't fix the problem.  If an incorrect systemPath was the
problem, would the initial dlopen("libc.so") call have succeeded?

Comment 24

[Julian Seward [:jseward]]

Attachment: Complete logcat (.txt.bz2) as per comment 22

Note, this is also with   systemPath = "/system/lib/";  as per
comment 23, and you can see the calls dlopen("libc.so") etc in
the log.

Comment 25

[Mike Hommey [:glandium]]

(In reply to Julian Seward from comment #23)
> (In reply to Julian Seward from comment #19)
> > E/Profiler( 4282): 2013-04-11 20:46:18:
> > local_debug_info_symbolizer.cc:75: INFO: ReadSymbolData_APK: Unable to
> > get size for ELF file 'libc.so'
> 
> > [23:31] <glandium> sewardj: edit mozglue/linker/ElfLoader.cpp, 
>   find /system/lib and add a / (/system/lib/)
> 
> -    systemPath = "/system/lib";
> +    systemPath = "/system/lib/";
> 
> This doesn't fix the problem.

It does for me:
void * foo = dlopen("libc.so", RTLD_GLOBAL | RTLD_LAZY);
__android_log_print(ANDROID_LOG_ERROR, "GeckoLinker", "__dl_get_mappable_length returns %d", __dl_get_mappable_length(foo));
dlclose(foo);

--> E/GeckoLinker( 5644): __dl_get_mappable_length returns 286500

Comment 26

[Mike Hommey [:glandium]]

(In reply to Julian Seward from comment #24)
> Created attachment 736529 [details]
> Complete logcat (.txt.bz2) as per comment 22
> 
> Note, this is also with   systemPath = "/system/lib/";  as per
> comment 23, and you can see the calls dlopen("libc.so") etc in
> the log.

Ah, but the error is different this time: "Failed to mmap ELF file 'libc.so'" instead of "Unable to
get size for ELF file 'libc.so'"

About the crash, the log shows this:
E/GeckoLinker( 7034): Caught segmentation fault @0x7c3de0e8
That's within the libxul range, after libxul is unloaded. Sounds like the good old "run code after libxul is unloaded", probably triggered by the profiler being on. This doesn't sound related to this patch at all.

Comment 27

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4badbe3db2

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d4badbe3db2