827846 - Symbolication on B2G doesn't work unless --disable-elf-hack

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 1

[Benoit Girard (:BenWa)]

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 2

[Benoit Girard (:BenWa)]

Attachment: Compile dl_iterate_phdr

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 699315 [details] [diff] [review]
Compile dl_iterate_phdr

You could even remove the #if entirely.

Comment 4

[Benoit Girard (:BenWa)]

Attachment: use dl_iterate_phdr on b2g

Tested it. Everything works fine

Comment 5

[Dave Hylands [:dhylands]]

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?

Comment 6

[Mike Hommey [:glandium]]

(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.

Comment 7

[Benoit Girard (:BenWa)]

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.

Comment 8

[Benoit Girard (:BenWa)]

apply on bionic check-out*

Comment 9

[Dave Hylands [:dhylands]]

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.

Comment 10

[John Ford [:jhford] CET/CEST Berlin Time]

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

Comment 11

[Mike Hommey [:glandium]]

(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 12

[Mike Hommey [:glandium]]

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 13

[Dave Hylands [:dhylands]]

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

Comment 14

[Dave Hylands [:dhylands]]

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?

Comment 15

[Benoit Girard (:BenWa)]

(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?

Comment 16

[Dave Hylands [:dhylands]]

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

Comment 17

[Ting-Yuan Huang]

(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.

Comment 18

[Ting-Yuan Huang]

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

Comment 19

[Benoit Girard (:BenWa)]

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

Comment 20

[Dave Hylands [:dhylands]]

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

Comment 21

[John Ford [:jhford] CET/CEST Berlin Time]

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

Comment 22

[Benoit Girard (:BenWa)]

(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

Comment 23

[John Ford [:jhford] CET/CEST Berlin Time]

(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.

Comment 24

[John Ford [:jhford] CET/CEST Berlin Time]

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

Comment 25

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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).

Comment 26

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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?

Comment 27

[Mike Hommey [:glandium]]

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

Comment 28

[Benoit Girard (:BenWa)]

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)

Comment 29

[Benoit Girard (:BenWa)]

I don't think anyone is hitting this limitation.