STR: 1. Take a profile according to https://developer.mozilla.org/en-US/docs/Performance/Profiling_with_the_Built-in_Profiler#Profiling_Boot_to_Gecko_%28with_a_real_device%29 2. If needed, run ./profile.sh symbolicate to get a .sym file 3. Open in Cleopatra Expected results: accurate symbols (not just pseudosymbols) Actual results: wrong symbols (only pseudosymbols are accurate) as in http://people.mozilla.com/~bgirard/cleopatra/#report=3b565949e20bf7d965adaaf95e8e8fc5c91efde3 Clobbering and re-building with --disable-elf-hack fixes the problem, as in http://people.mozilla.com/~bgirard/cleopatra/#report=75848aa8ceae25cbd7e49ba1518793cdcc2156bb I double checked that: re-building without --disable-elf-hack brought the problem back.

The reason we don't support elfhack on b2g is because we need to get the library information from dl_iterate_phdr. On android we use a custom linker and for b2g this code lives in 'bionic/linker/linker.c' but it's ifdef-ed out. Our best option is to enable it but our fall back is to warn and remove leaf address on b2g+elfhack.

Comment on attachment 699315 [details] [diff] [review] Compile dl_iterate_phdr You could even remove the #if entirely.

Tested it. Everything works fine

Since we can't actually check the bionic change into the tree, wouldn't it make sense to copy the dl_iterate_phdr function into shared_libraries_linux.cc so that we don't need to modify the bionic source?

(In reply to Dave Hylands [:dhylands] from comment #5) > Since we can't actually check the bionic change into the tree, wouldn't it > make sense to copy the dl_iterate_phdr function into > shared_libraries_linux.cc so that we don't need to modify the bionic source? That's a recipe for disaster, as that would rely on bionic internals.

Can we do like we do in gfx and keep an external patch file that we apply on check-in? If the version of bionic is fix then there wont be any conflicts when applying.

apply on bionic check-out*

Actually, I believe that a patching mechanism is going to be introduced which will allow patches to be applied to trees which we don't otherwise own. cc'ing jhford, who I believe is working on this.

There is a small patch (https://github.com/mozilla-b2g/B2G/pull/190) that'll make use of https://github.com/mozilla-b2g/gonk-patches when it's added to the B2G manifests

(In reply to John Ford [:jhford] from comment #10) > There is a small patch (https://github.com/mozilla-b2g/B2G/pull/190) that'll > make use of https://github.com/mozilla-b2g/gonk-patches when it's added to > the B2G manifests How does that work with manufacturer builds?

Comment on attachment 699337 [details] [diff] [review] use dl_iterate_phdr on b2g Review of attachment 699337 [details] [diff] [review]: ----------------------------------------------------------------- I think we should add a configure.in check for dl_iterate_phdr, and #if defined(HAVE_DL_ITERATE_PHDR) || defined(MOZ_LINKER)

Comment on attachment 699315 [details] [diff] [review] Compile dl_iterate_phdr Review of attachment 699315 [details] [diff] [review]: ----------------------------------------------------------------- Sorry- I forgot to r+ this sooner

So ting-yuan figured out how to make profile-symbolicate work with elf-hacked libs over in bug 833267 Will this and his method conflict? If they do conflict, which way should we go?

(In reply to Dave Hylands [:dhylands] from comment #14) > So ting-yuan figured out how to make profile-symbolicate work with > elf-hacked libs over in bug 833267 > > Will this and his method conflict? If they do conflict, which way should we > go? I do believe they will conflict unless we add something to disambiguate when reporting the libraries. In the short term it's not a huge problem but will be a burden long term. Can we just land this patch locally or upstream it to bionic?

I think that we need the patching mechanism in order to land this.

(In reply to Benoit Girard (:BenWa) from comment #15) > (In reply to Dave Hylands [:dhylands] from comment #14) > > So ting-yuan figured out how to make profile-symbolicate work with > > elf-hacked libs over in bug 833267 > > > > Will this and his method conflict? If they do conflict, which way should we > > go? > > I do believe they will conflict unless we add something to disambiguate when > reporting the libraries. In the short term it's not a huge problem but will > be a burden long term. Can we just land this patch locally or upstream it to > bionic? BTW, on platforms that utilize dl_iterate_phdr() + dl_iterate_callback(), the solution in bug 833267 also won't work, since it expects libraries been reported with file offsets. It would result in incompatibility to those platforms. To adopt bug 833267, the profiler on target must be made to report file offsets. Either way (to report vaddrs or offsets) should be fine, as long as the info are addressed in a consistent way. Since SharedLibraryInfo::GetInfoForSelf() on some platform reports vaddr and on the others reports offsets, it should be fixed.

Personally I prefer reporting vaddrs, like BenWa's solution, since otherwise it requires nontrivial changes on macos and win32.

Is the proper system for patching the bionic linker in place now? Is anyone familiar with the system that could integrate my patch above?

I've seen mentions of it in a few places. jhford would be the right person to ask.

All of the build system is in place and the repository is created. We don't have this repository in the manifest quite yet because we're waiting on a patch that needs it. Can you tell me exactly: 1) which patches are needed 2) what branches need the patches 3) which devices need the patches

(In reply to John Ford [:jhford] from comment #21) > All of the build system is in place and the repository is created. We don't > have this repository in the manifest quite yet because we're waiting on a > patch that needs it. > > Can you tell me exactly: > 1) which patches are needed Attachment #699315 [details] [diff] > 2) what branches need the patches bionic. I'm not sure what branches exactly. > 3) which devices need the patches all

(In reply to Benoit Girard (:BenWa) from comment #22) > (In reply to John Ford [:jhford] from comment #21) > > All of the build system is in place and the repository is created. We don't > > have this repository in the manifest quite yet because we're waiting on a > > patch that needs it. > > > > Can you tell me exactly: > > 1) which patches are needed > Attachment #699315 [details] [diff] [diff] > > 2) what branches need the patches > bionic. I'm not sure what branches exactly. > > 3) which devices need the patches > all Hi Benoit, I have landed the changes for this into the gonk-patches repository. To test that everything works with your patch, please do: cd B2G # Get into your B2G root git clone git://github.com/mozilla-b2g/gonk-patches.git patches ./build.sh build.sh will know how to trigger the patching when the patches repository is present.

Once we've verified that these patches are good, I'll open a PR to have this repository included in the manifests

Is dl_iterate_phdr really the problem here? It seems to me that the immediate cause of broken b2g profiles is that scripts/profile-symbolicate.py assumes offset == virtaddr for all executable mappings, which happens to be true of the non-elfhacked versions of our .so files, but isn't true in general (e.g., it can't resolve symbols in the b2g executable itself). See also: we can symbolicate perf_event profiles correctly, even though the Linux kernel neither knows nor cares about the userspace runtime linker's internal state. The profile records executable mmaps (the same info as /proc/*/maps, but with live updates); this is sufficient to translate addresses to file offsets, and then the phdrs from the files in the build tree are used to translate them back to non-ASLR-ed virtual addresses, which are then looked up in the symbol table (or debug info).

Also: currently the "libs" blob in the profile looks like this: {"start":1075273728, "end":1078349824, "offset":0, "name":"/system/b2g/libxul.so"}, {"start":1078349824, "end":1101762560, "offset":552960, "name":"/system/b2g/libxul.so"}, This correctly indicates which parts of the file are mapped, and where. Is that not something I can depend on?

Yes, that would work. It's more work on the symbolication end than using dl_iterate_phdr, but it would work.

I looked into this again. Needing time more and more as the obvious graphics problem are fixed and this will help make sure the leaf is in the right fast paths. The bionic linker.c doesn't apply to the following branches: sprdroid4.0.3_vlx_3.0_b2g (code is modified) refs/tags/android-4.3_r2.1 (linker.c renamed to linker.cpp)

I don't think anyone is hitting this limitation.