1378986 - Make elfhack work again on Android

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

In bug 1163171, it was mentioned that elfhack does not build with clang, and I wonder if we still needed elfhack at all in 2017.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Try run with elfhack disabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e193f54c7dda0f87ac2a091976f90065bd2a0d7&selectedJob=112276700

Autophone results: http://phonedash.mozilla.org/#/2017-07-05/2017-07-06/binning=repo-phonetype-phoneid-test_name&rejected=norejected&errorbars=noerrorbars&errorbartype=standarderror&valuetype=median&geckoview-e10s-nytimes=on&geckoview-nytimes=on&remote-nytimes=on&throbberstart=on&first=on&second=on&autoland=on&mozilla-beta=on&mozilla-central=on&mozilla-inbound=on&try-jwillcox=on&nexus-6p=on&nexus-6p-10=on&nexus-6p-11=on&nexus-6p-12=on

It looks like the startup time is about the same with elfhack or not, and the APK size did not change signficiantly.

Comment 2

[Mike Hommey [:glandium]]

See bug 1163171 comment 22.

Comment 3

[Mike Hommey [:glandium]]

06:52 <glandium> regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a812ef63de87&tochange=6955309291ee
06:52 <glandium> 4.5 years ago
06:53 <glandium> it's funny that bug 822584 is in that range
06:53 <firebot> https://bugzil.la/822584 — FIXED, [e-mail] — Breakpad can't deal with elfhacked binaries on ARM (B2G crash stacks are broken/corrupted)
06:54 <snorp> glandium: wow
06:55 <glandium> My bet is on bug 825151
06:55 <firebot> https://bugzil.la/825151 — FIXED, [e-mail] — Switch ARMv7 builds over to using NDK r8c
06:56 <glandium> so, I fixed something elfhack related on arm, and 15 changesets later, elfhack went kaboom from an upgrade of gcc
06:56 <glandium> nice timing

Comment 4

[Mike Hommey [:glandium]]

I got past the compilation failures with relro disabled, and got this during stage-package:
[task 2017-07-07T03:07:07.499266Z] 03:07:07     INFO -  ../../../dist/geckoview/assets/armeabi-v7a/liblgpllibs.so: Didn't find relocation for DT_INIT_ARRAY's first entry. Skipping
[task 2017-07-07T03:07:07.499443Z] 03:07:07     INFO -  ../../../dist/geckoview/assets/armeabi-v7a/libomxplugin.so: No gain. Skipping
[task 2017-07-07T03:07:07.499626Z] 03:07:07     INFO -  ../../../dist/geckoview/assets/armeabi-v7a/libomxpluginkk.so: No gain. Skipping
[task 2017-07-07T03:07:07.499806Z] 03:07:07     INFO -  ../../../dist/geckoview/assets/armeabi-v7a/libxul.so: Reduced by 2604904 bytes
[task 2017-07-07T03:07:07.500050Z] 03:07:07     INFO -  ../../../dist/geckoview/assets/armeabi-v7a/libnssckbi.so: Didn't find relocation for DT_INIT_ARRAY's first entry. Skipping
[task 2017-07-07T03:07:07.500238Z] 03:07:07     INFO -  ../../../dist/geckoview/assets/armeabi-v7a/libnss3.so: Didn't find relocation for DT_INIT_ARRAY's first entry. Skipping
[task 2017-07-07T03:07:07.500422Z] 03:07:07     INFO -  ../../../dist/geckoview/assets/armeabi-v7a/libsoftokn3.so: Didn't find relocation for DT_INIT_ARRAY's first entry. Skipping
[task 2017-07-07T03:07:07.500604Z] 03:07:07     INFO -  ../../../dist/geckoview/assets/armeabi-v7a/libfreebl3.so: Didn't find relocation for DT_INIT_ARRAY's first entry. Skipping
[task 2017-07-07T03:07:07.500793Z] 03:07:07     INFO -  ../../../dist/geckoview/lib/armeabi-v7a/libmozglue.so: Reduced by 8044 bytes
[task 2017-07-07T03:07:07.501001Z] 03:07:07     INFO -  elfhack: /home/worker/workspace/build/src/build/unix/elfhack/elf.cpp:264: Elf::Elf(std::ifstream&): Assertion `segment->getFileSize() == phdr.p_filesz - gold_adjustment' failed.

The main thing in there is the 2.6MB drop in libxul.so size, although compressed, it's likely not to be as big, but that's still going to be that much less memory used in the processes.

The DT_INIT_ARRAY thing doesn't harm but it would be worth checking what's up with it, because there might be wins to have if we can actually elfhack the libraries that are being skipped because of it.

And the final one is an actual error, and unfortunately, it doesn't tell on which particular file it's happening, but that's definitely something that will need fixing.

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1378986 - Avoid elfhack failing on weird DT_INIT_ARRAYs.

Somehow, with the Android toolchain, we end up with
non-empty-but-really-empty DT_INIT_ARRAYs.

In practical terms, they are arrays with no relocations, and content
that is meaningless:

  $ objdump -s -j .init_array libnss3.so

  libnss3.so:     file format elf32-little

  Contents of section .init_array:
   1086e0 00000000                             ....

  $ readelf -r libnss3.so | grep 1086e0

  $ objdump -s -j .init_array libplugin-container-pie.so

  libplugin-container-pie.so:     file format elf32-little

  Contents of section .init_array:
   4479c ffffffff 00000000 ffffffff 00000000  ................

  $ readelf -r libplugin-container-pie.so | grep 4479c

Because so far, elfhack expected meaningful DT_INIT_ARRAYs, it bailed out
early in that case.

Review commit: https://reviewboard.mozilla.org/r/155184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155184/

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1378986 - Adjust the fake phdr section properly.

The PT_PHDR segment is optional, but the Android toolchain decides to
create one in some cases, and places it first. When that happens, the
work around for bug 1233963 fails, because the fake phdr section has not
been adjusted yet (it only happens when we see a PT_LOAD).

So we adjust the fake phdr section when we see a PT_PHDR segment (and
avoid re-updating it when we see a subsequent PT_LOAD).

Review commit: https://reviewboard.mozilla.org/r/155186/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155186/

Comment 7

[Mike Hommey [:glandium]]

Here's a push with both patches + norelro.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e4410d7d3974c5248b092de2b33a12c8dc96b6e

How do we see how it performs on autophone?

Comment 8

[Mike Hommey [:glandium]]

(Note I'm considering fixing bug 635961, which would make elfhack work without disabling relro)

Comment 9

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #7)
> Here's a push with both patches + norelro.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4e4410d7d3974c5248b092de2b33a12c8dc96b6e

Note this is on top of your previous push to try, so it's on top of bug 1374647 and bug 1377580.

Anyways, a first point of comparison, on the Android 4.0 API15+ opt builds:

on m-c (283debe8155a, which is the base of the try push):
   Size of libxul.so: 22217892 bytes
   Size of target.apk: 36659110 bytes 

on try:
   Size of libxul.so: 22145516 bytes
   Size of target.apk: 36582170 bytes 

So it doesn't make much difference on the apk because libxul.so is xz'ed.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8884233 [details]
Bug 1378986 - Avoid elfhack failing on weird DT_INIT_ARRAYs.

https://reviewboard.mozilla.org/r/155184/#review160230

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Here's phonedash for the latest build with elfhack enabled: http://phonedash.mozilla.org/#/2017-07-06/2017-07-07/binning=repo-phonetype-phoneid-test_name&rejected=norejected&errorbars=noerrorbars&errorbartype=standarderror&valuetype=median&geckoview-e10s-nytimes=on&geckoview-nytimes=on&remote-blank=on&throbberstart=on&second=on&mozilla-central=on&try-mh=on&nexus-5=on&nexus-5-1=on&nexus-5-3=on&nexus-5-5=on&nexus-5-6=on

It looks like there could be a slight improvement on startup time (throbberstart) on the Nexus 5, but it's also well within the noise range so who knows.

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8884234 [details]
Bug 1378986 - Adjust the fake phdr section properly.

https://reviewboard.mozilla.org/r/155186/#review160380

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16ff9fdd6dba
Avoid elfhack failing on weird DT_INIT_ARRAYs. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/9d9b82a56332
Adjust the fake phdr section properly. r=froydnj

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/16ff9fdd6dba
https://hg.mozilla.org/mozilla-central/rev/9d9b82a56332

Comment 15

[Mike Hommey [:glandium]]

Attachment: Avoid crashing in elfhack when the input file has no relocations

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f12698197df5
Avoid crashing in elfhack when the input file has no relocations. r=me a=bustage

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f12698197df5

Comment 18

[Mike Hommey [:glandium]]

Bug 635961 is confirmed to be the last missing piece:
[task 2017-07-26T21:32:29.232371Z] 21:32:29     INFO -  ../../../dist/geckoview/assets/armeabi-v7a/libxul.so: Reduced by 2568040 bytes

This can now be closed.

Comment 19

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.