James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) Reporter
	Description • 7 years ago

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.

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) Reporter
	Comment 1 • 7 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 2 • 7 years ago

See bug 1163171 comment 22.

	Mike Hommey [:glandium] Assignee
	Comment 3 • 7 years ago

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, mh+mozilla@glandium.org — 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, bugmail@mozilla.staktrace.com — 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

	Mike Hommey [:glandium] Assignee
	Comment 4 • 7 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 7 • 7 years ago

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?

	Mike Hommey [:glandium] Assignee
	Comment 8 • 7 years ago

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

	Mike Hommey [:glandium] Assignee
	Comment 9 • 7 years ago

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

	Nathan Froyd [:froydnj]
	Comment 10 • 7 years ago
mozreview-review

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

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

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) Reporter
	Comment 11 • 7 years ago

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.

	Nathan Froyd [:froydnj]
	Comment 12 • 7 years ago
mozreview-review

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

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

	Pulsebot
	Comment 13 • 7 years ago

Pushed by mh@glandium.org:
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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 14 • 7 years ago
bugherder

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

	Pulsebot
	Comment 16 • 7 years ago

Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f12698197df5
Avoid crashing in elfhack when the input file has no relocations. r=me a=bustage

	Carsten Book [:Tomcat]
	Comment 17 • 7 years ago
bugherder

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

	Mike Hommey [:glandium] Assignee
	Comment 18 • 7 years ago

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.

	Firefox Bug Husbandry Bot
	Comment 19 • 6 years ago

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