606145 - Compress/pack relocations in ELF binaries (elfhack)

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

The idea, that I'm going to experiment in the coming days, is the following:
- Insert a constructor function (.ctor) that would handle a compressed or packed relocations section, and put it at the end of the .ctors section with something like what is in bug 606137.
- Get rid of the relocations except for the ones necessary to our constructor function.
- Modify the ELF header such that libc won't remove write permissions on the mapped memory, which we would remove ourselves.

The 2 first steps might be replaced by modifying the ELF entry pointer, if that works out.

This could benefit all linux builds including android.

This could also be extended to do fancy stuff such as madvise()ing.

Comment 1

[Mike Hommey [:glandium]]

See http://glandium.org/blog/?p=1177#relocations for some details, and http://hg.mozilla.org/users/mh_glandium.org/elfhack/ for the current implementation.

I'm going to attach two patches for the build system integration.

Comment 2

[Mike Hommey [:glandium]]

Attachment: part 1 - Properly link host programs written in C++

Comment 3

[Mike Hommey [:glandium]]

Attachment: part 3 - Integrate elfhack with the build system

I'm not attaching the source code itself (which will be part 2), as I want to fix a bunch of glitches I encountered, first.

Comment 4

[Mike Hommey [:glandium]]

Attachment: part 2 - Import elfhack code

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 495526 [details] [diff] [review]
part 2 - Import elfhack code

This doesn't build on buildbots because of problems with kernel headers:

/usr/include/linux/byteorder/swab.h: In function '__u16 __fswab16(__u16)':
/usr/include/linux/byteorder/swab.h:134: error: ISO C++ forbids braced-groups within expressions
/usr/include/linux/byteorder/swab.h:134: error: ISO C++ forbids braced-groups within expressions
/usr/include/linux/byteorder/swab.h: In function '__u16 __swab16p(const __u16*)':
/usr/include/linux/byteorder/swab.h:138: error: ISO C++ forbids braced-groups within expressions
/usr/include/linux/byteorder/swab.h:138: error: ISO C++ forbids braced-groups within expressions
/usr/include/linux/byteorder/swab.h: In function 'void __swab16s(__u16*)':
/usr/include/linux/byteorder/swab.h:142: error: ISO C++ forbids braced-groups within expressions
/usr/include/linux/byteorder/swab.h:142: error: ISO C++ forbids braced-groups within expressions

Comment 6

[Mike Hommey [:glandium]]

Attachment: part 3 - Integrate elfhack with the build system

Added missing defines.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 495533 [details] [diff] [review]
part 3 - Integrate elfhack with the build system

There are a few more issues to fix. I think I'll first go with a configure option defaulting to disabled, so that we can land this asap and enhance/fix it before defaulting to enabled.

Comment 8

[Mike Hommey [:glandium]]

Attachment: part 2 - Import elfhack code

This has been successfully tested on maemo, android, linux and linux64 (after some small fixes)

Comment 9

[Mike Hommey [:glandium]]

Attachment: part 3 - Integrate elfhack with the build system

This is now disabled by default, and requires --enable-elf-hack to be enabled. This could thus land ASAP as NPOTB, and we can further improve before enabling by default.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 495791 [details] [diff] [review]
part 2 - Import elfhack code

Taras agreed to review this part.

Comment 11

[(dormant account)]

This needs a README(or a comment) to describe what it does. At least one testcase to verify that it is working.

Comment 12

[(dormant account)]

Comment on attachment 495791 [details] [diff] [review]
part 2 - Import elfhack code

Use .h instead of .hxx

void isNextOf makes no sense, rename it. 

I think this code needs a basic sanity check testcase within elfhack/ directory since it will be useful outside of Mozilla too.

Rest of the code looks like reasonable elf-fiddling.

Comment 13

[Jim Blandy :jimb]

(In reply to comment #11)
> This needs a README(or a comment) to describe what it does. At least one
> testcase to verify that it is working.

Yeah, I agree.

Comment 14

[Mike Hommey [:glandium]]

Attachment: Addition to part 2 to address review comments

This hopefully addresses all your comments. Unfortunately, the testcase won't run when cross compiling (e.g. for android)

Comment 15

[Mike Hommey [:glandium]]

Attachment: part 3 - Integrate elfhack with the build system

Adjusted for the testcase added in attachment #499328 [details] [diff] [review]

Comment 16

[Mike Hommey [:glandium]]

(In reply to comment #14)
> Created attachment 499328 [details] [diff] [review]
> Addition to part 2 to address review comments
> 
> This hopefully addresses all your comments. Unfortunately, the testcase won't
> run when cross compiling (e.g. for android)

There's additionally a fix that makes ./elfhack elfhack not segfault.

Comment 17

[Mike Hommey [:glandium]]

Attachment: part 2 - Import elfhack code

Updated again (with irc comments from Taras for the README), and merged both subparts.

Comment 18

[Mike Hommey [:glandium]]

Attachment: part 2 - Import elfhack code

New patch including the following changes:
http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/2873cb034cd0
http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/c4bb1f9c249d
http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/097cb9c45a5e

Comment 19

[Mike Hommey [:glandium]]

Attachment: part 3 - Integrate elfhack with the build system

As agreed on IRC with Taras, the naming of the injected sections, as implemented in the last iteration of part 2, is the last step before enabling the hack by default, which I'm now doing by removing the configure parts, and USE_ELF_HACK conditionals.

Comment 20

[Mike Hommey [:glandium]]

Attachment: part 2 - Import elfhack code

With http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/28bd20ed70d7, as discussed on irc ; carrying r+ over.

Comment 21

[Mike Hommey [:glandium]]

Attachment: part 3 - Integrate elfhack with the build system

We had a misunderstanding on irc. So we do want to keep the option to disable the hack, but we still enable it by default. Sorry for the noise. This time, this should be the last iteration.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 499396 [details] [diff] [review]
part 3 - Integrate elfhack with the build system

r+ w/nits

>diff --git a/build/unix/elfhack/Makefile.in b/build/unix/elfhack/Makefile.in

This file is a little messy, but I'm not sure how to make it better short of recursing into inject/, which I'm not thrilled about either.

>+inject:
>+	[ -d $@ ] || mkdir $@

$(NSINSTALL) -D $@?

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -7698,16 +7698,26 @@ dnl ====================================
> dnl = --enable-elf-dynstr-gc
> dnl ========================================================
> MOZ_ARG_ENABLE_BOOL(elf-dynstr-gc,
> [  --enable-elf-dynstr-gc  Enable elf dynstr garbage collector (opt builds only)],
>     USE_ELF_DYNSTR_GC=1,
>     USE_ELF_DYNSTR_GC= )
> 
> dnl ========================================================
>+dnl = --enable-elf-dynstr-gc

--disable-elf-hack?

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

I see this has tests. Can someone give a quick blurb about risk and reward of taking this? Also, have this been through try?

Comment 24

[Robert Kaiser]

Also, I think we should have a clear on/off switch for any somewhat larger thing we take into a stable/beta release. I guess in this case this is the configure code that turns USE_ELF_HACK to 1 or empty. Would it make sense in some way to land it in a way that it's very near to a one-liner to completely turn off the impact of this patch set?
I'm just talking the possibility of risk minimization and easily returning to the previous state even without a complete code-backout.

Comment 25

[Mike Hommey [:glandium]]

Attachment: part 3 - Integrate elfhack with the build system

Addressed review comments. Carrying r+ over.

Comment 26

[Mike Hommey [:glandium]]

(In reply to comment #23)
> I see this has tests. Can someone give a quick blurb about risk and reward of
> taking this? Also, have this been through try?

It's been through try during previous iterations, and going through try right now, both for all linux targets including unit tests, and other targets without unit tests to double check that the build doesn't break, i.e. that the conditionals are correct.

I'll post how much is saved on each impacted platform in terms of file sizes, according to the try builds.

(In reply to comment #24)
> Also, I think we should have a clear on/off switch for any somewhat larger
> thing we take into a stable/beta release. I guess in this case this is the
> configure code that turns USE_ELF_HACK to 1 or empty. Would it make sense in
> some way to land it in a way that it's very near to a one-liner to completely
> turn off the impact of this patch set?
> I'm just talking the possibility of risk minimization and easily returning to
> the previous state even without a complete code-backout.

Technically, replacing USE_ELF_HACK=1 with USE_ELF_HACK= before the MOZ_ARG_DISABLE_BOOL line will disable it, but still leave a --enable-elf-hack option doing the appropriate thing (which MOZ_ARG_DISABLE_BOOL conveniently implements).

(In reply to comment #22)
> >diff --git a/build/unix/elfhack/Makefile.in b/build/unix/elfhack/Makefile.in
> 
> This file is a little messy, but I'm not sure how to make it better short of
> recursing into inject/, which I'm not thrilled about either.

Eventually, I want to find a way to only need one copy of the injected code built to handle both with init and noinit cases. The Makefile will be simplified then, but that'll be for later.

Comment 27

[Mike Hommey [:glandium]]

Try build for all impacted platforms:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=edea484124b0
There's already a failure on maemo, which was addressed with http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/3a6206ac5351 but apparently, this wasn't enough.

This is the build for other platforms, that shouldn't fail to build.
http://tbpl.mozilla.org/?tree=MozillaTry&rev=028bbc995640

Comment 28

[Mike Hommey [:glandium]]

(In reply to comment #27)
> Try build for all impacted platforms:
> http://tbpl.mozilla.org/?tree=MozillaTry&rev=edea484124b0

There were oranges, but they are the same as for the parent changeset:
http://tbpl.mozilla.org/?rev=14ae20ed2bd5

As for the bustages on maemo, I identified them and found a way to reproduce. A fix should follow soon.

> This is the build for other platforms, that shouldn't fail to build.
> http://tbpl.mozilla.org/?tree=MozillaTry&rev=028bbc995640

This was all green.

As for binary size reductions:
On Linux opt:
libxul.so: Reduced by 1413008 bytes

(all others offered no gain, so were not modified by elfhack)

On Linux64 opt:
libxul.so: Reduced by 4792160 bytes

libnssckbi.so: Reduced by 73568 bytes

libnspr4.so: Reduced by 12128 bytes

libnss3.so: Reduced by 8032 bytes

libmozsqlite3.so: Reduced by 8032 bytes

libfreebl3.so: Reduced by 3936 bytes

components/libbrowsercomps.so: Reduced by 12128 bytes

libssl3.so: Reduced by 3936 bytes

libnssutil3.so: Reduced by 8032 bytes

libsoftokn3.so: Reduced by 3928 bytes


On Android:
libxul.so: Reduced by 1437580 bytes

libmozsqlite3.so: Reduced by 3980 bytes

I don't know how that reflects on the corresponding apk, I don't know where to find the corresponding apk for the parent commit to compare.

All these file size reductions are on portions that are always read on startup (except for prelinked binaries), so the gains are real.

Comment 29

[Mike Hommey [:glandium]]

Attachment: part 2 - Import elfhack code

This is the version I pushed to try for maemo and android:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=8f252fef68d0

It includes this fix:
http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/b2b392bc2df1
(I wish the ARM ELF documentation was clearer)
Maybe Jacob could take a look to the above commit diff. As far as my testing goes, this produces the correct branch call in the resulting binary.

As it modifies ARM related code, I pushed on all architectures using ARM. Other architectures are not impacted.

Comment 30

[Mike Hommey [:glandium]]

Comment on attachment 499491 [details] [diff] [review]
part 2 - Import elfhack code

Actually, I'll carry over previous r+, and request the additional review from Jacob for http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/b2b392bc2df1 which is the only change since last iteration.

Comment 31

[Mike Hommey [:glandium]]

I got maemo to build (apparently, it doesn't load libraries with LD_PRELOAD), but it turns out there is yet another problem. What I propose to go forward is to modify part 3 to exclude ARM for now (it's just a matter of removing arm from the test in toolkit/mozapps/installer/packager.mk), land the current patches, and file a followup bug for ARM issues.

Comment 32

[Robert Kaiser]

(In reply to comment #28)
> All these file size reductions are on portions that are always read on startup
> (except for prelinked binaries), so the gains are real.

Do we have perf numbers from try as well? I'd guess that we should see some impact on perf due to those size reductions (and if they're all going in the right direction, that could be a huge argument for getting this in).

Comment 33

[Mike Hommey [:glandium]]

(In reply to comment #32)
> Do we have perf numbers from try as well? I'd guess that we should see some
> impact on perf due to those size reductions (and if they're all going in the
> right direction, that could be a huge argument for getting this in).

AIUI, ts_cold was removed, and it's basically the only test that would have shown a significant improvement, despite its flaws. So, I didn't run talos on the try pushes.

Comment 34

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #33)
> (In reply to comment #32)
> > Do we have perf numbers from try as well? I'd guess that we should see some
> > impact on perf due to those size reductions (and if they're all going in the
> > right direction, that could be a huge argument for getting this in).
> 
> AIUI, ts_cold was removed, and it's basically the only test that would have
> shown a significant improvement, despite its flaws. So, I didn't run talos on
> the try pushes.

Would you mind running with full talos? I understand you don't expect a perf impact, but stranger things have happened.

Comment 35

[Mike Hommey [:glandium]]

(In reply to comment #34)
> Would you mind running with full talos? I understand you don't expect a perf
> impact, but stranger things have happened.

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=66036625795f&newRev=791e5c04c125&tests=a11y,tdhtml,tdhtml_nochrome,tp4,tp4_memset,tp4_pbytes,tp4_rss,tp4_shutdown,tp4_xres,dromaeo_basics,dromaeo_css,dromaeo_dom,dromaeo_jslib,dromaeo_sunspider,dromaeo_v8,tsspider,tsspider_nochrome,tgfx,tgfx_nochrome,tscroll,tsvg,tsvg_opacity,ts,ts_cold,ts_cold_generated_max,ts_cold_generated_max_shutdown,ts_cold_generated_med,ts_cold_generated_med_shutdown,ts_cold_generated_min,ts_cold_generated_min_shutdown,ts_cold_shutdown,ts_places_generated_max,ts_places_generated_max_shutdown,ts_places_generated_med,ts_places_generated_med_shutdown,ts_places_generated_min,ts_places_generated_min_shutdown,ts_shutdown,twinopen&submit=true

apparently, ts_cold is still running ; this is the latest nightly, compared with nightly + latest patches from this bug.

Comment 36

[Mike Hommey [:glandium]]

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=66036625795f,43a54dd56ccd,cd5a2a31cb8e&newRev=791e5c04c125

Comment 37

[Mike Hommey [:glandium]]

Attachment: part 2 - Import elfhack code

I found what the maemo issues were about:
- Original DT_INIT function was not called at all, but that didn't lead to any actual problem because the only function being called this way is call_gmon_start, which only does actual work if the binary is built with profiling stuff (either for PGO or gcov). That part was addressed in previous patch.
- Somehow, the testcase didn't run through the LD_PRELOAD variable, so I had to replace /bin/false with a binary doing the same, but linked against the test library.
- The DT_INIT_ARRAYSZ dynamic field ended up with the wrong value because of the .tbss section being logically at the same address as .init_array on maemo binaries. This is what caused the crashes, because only one init function was called (meaning most static initializers wouldn't run). This was a regression from http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/93194e8a4bcb, and I added extra code to the testcase to avoid this issue in the future. Corresponding commits are the following:
http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/755b083e2565
http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/afaaa7ffa029
http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/c13b41574183

I still would like feedback from Jacob for http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/b2b392bc2df1 though it apparently works properly.

I ran this through try, again, and nothing broke. I also tested the testcase with the broken elf.cpp code on maemo, which properly spotted the error and failed the build.

Comment 38

[Mike Hommey [:glandium]]

Attachment: part 3 - Integrate elfhack with the build system

This adds support for building the dummy binary needed for the testcase to work on maemo.

Comment 39

[Jacob Bramley [:jbramley]]

Comment on attachment 500021 [details] [diff] [review]
part 2 - Import elfhack code

I know very little about ELF so I'm not really qualified to review this. The code looks sensible enough (at a glance), and the theory appears sound. I've given it '+' for feedback, but be aware that it doesn't count for much.

Comment 40

[Mike Hommey [:glandium]]

Here are some numbers I got on my setup, over 50 startups for each:
Plain 4.0b8:
x86 	3,228.76 ± 0.57%
x86-64 	3,382.0 ± 0.51%

4.0b8+elfhack:
x86 	3,149.32 ± 0.62%
x86-64 	3,191.58 ± 0.62%

See http://glandium.org/blog/?p=1296 for more details

Comment 41

[(dormant account)]

(In reply to comment #40)
> Here are some numbers I got on my setup, over 50 startups for each:
> Plain 4.0b8:
> x86     3,228.76 ± 0.57%
> x86-64     3,382.0 ± 0.51%
> 
> 4.0b8+elfhack:
> x86     3,149.32 ± 0.62%
> x86-64     3,191.58 ± 0.62%
> 
> See http://glandium.org/blog/?p=1296 for more details

And if you read the post, it's clear that this + other changes result in a very significant startup win when combined.

Comment 42

[Benjamin Smedberg]

Comment on attachment 500022 [details] [diff] [review]
part 3 - Integrate elfhack with the build system

Is there a particular reason we're doing this at package-time instead of build time? It seems to me that we could do this in the $(SHARED_LIBRARY) target of rules.mk.

Comment 43

[Mike Hommey [:glandium]]

(In reply to comment #42)
> Comment on attachment 500022 [details] [diff] [review]
> part 3 - Integrate elfhack with the build system
> 
> Is there a particular reason we're doing this at package-time instead of build
> time?

Mostly, to catch nss files, where there's a win on x86-64.

Comment 44

[Mike Hommey [:glandium]]

Unfortunately, there are now 2 problems with Android:
- elfhack doesn't support that the injected code is thumb, which was turned on by default recently. This can be easily worked around with a patch removing the thumb arguments from CFLAGS.
- elfhack apparently unveals a dynamic linker bug that makes the last text relocation (not packed by elfhack) not applied correctly. It also looks like from /proc/pid/maps that not all PT_LOAD sections are loaded ; I couldn't find traces of the data sections in /proc/pid/maps, but that's really weird because it should fail much earlier than it does if that were actually the case.

Anyways, I'm looking into this last issue, the first one being a matter of filter-out in the Makefile. Procedure-wise, do I need review and approval for this fixup?

Comment 45

[(not currently active) Ted Mielczarek]

If it winds up being a trivial fix, then no. If you wind up making substantial changes, then it should get re-review/approval. Can we just disable this for Android for now while you fix those issues in a followup?

Comment 46

[Mike Hommey [:glandium]]

Attachment: part 3 - Integrate elfhack with the build system - as landed

Removed thumb flags (i think they are used on meego) when building elfhack and disabled for android. Will file followup bugs.

Comment 47

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/0d947763fe67
http://hg.mozilla.org/mozilla-central/rev/a1ccb1c489ba
http://hg.mozilla.org/mozilla-central/rev/9a6de1e28d4b

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This causes my build to fail on Ubuntu 10.10:

elfhack: /home/dbaron/builds/ssd/mozilla-central/mozilla/build/unix/elfhack/elf.cpp:284: Elf::Elf(std::ifstream&): Assertion `segment->getFileSize() == phdr.p_filesz' failed.

Comment 49

[[:fabrice] Fabrice Desré]

You can build with --disable-elf-hack to not hit this issue

Comment 50

[Takanori MATSUURA]

On Fedora 14 hg:ac6c96109dcf build failed at
c++ -o host_elf.o -c -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloade
d-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno
-invalid-offsetof -Wno-variadic-macros -Werror=return-type -Wno-long-long -gdwar
f-2 -fno-strict-aliasing -fshort-wchar -pthread -pipe -fexceptions  -DNDEBUG -DT
RIMMED -g -O2 -g -pipe -Wp,-D_FORTIFY_SOURCE=2 -fno-exceptions -fstack-protector
 --param=ssp-buffer-size=4 -m64 -mtune=generic  -I. -I. -I../../../dist/include 
-I../../../dist/include/nsprpub  -I/usr/include/nspr4 -I/usr/include/nss3      -
I/usr/include/nspr4 elf.cpp
elf.cpp: In constructor 'Elf::Elf(std::ifstream&)':
elf.cpp:149:54: error: exception handling disabled, use -fexceptions to enable

According to https://developer.mozilla.org/en/C___Portability_Guide#Don%27t_use_exceptions
we cannot use exceptions for Mozilla products.

Comment 51

[Takanori MATSUURA]

And we got the following warning message during compiling build/unix/elfhack.test.c
test.c:116:5: warning: C++ style comments are not allowed in ISO C90
test.c:116:5: warning: (this will be reported only once per input file)

Comment 52

[Mike Hommey [:glandium]]

(In reply to comment #50)
> elf.cpp: In constructor 'Elf::Elf(std::ifstream&)':
> elf.cpp:149:54: error: exception handling disabled, use -fexceptions to enable
> 
> According to
> https://developer.mozilla.org/en/C___Portability_Guide#Don%27t_use_exceptions
> we cannot use exceptions for Mozilla products.

It's only a linux-specific build tool, portability is not exactly important here. It doesn't end up in the final binary.
It's interesting, though, that your compiler doesn't handle the presence of both -fexceptions and -fno-exceptions like others.

(In reply to comment #51)
> And we got the following warning message during compiling
> build/unix/elfhack.test.c
> test.c:116:5: warning: C++ style comments are not allowed in ISO C90
> test.c:116:5: warning: (this will be reported only once per input file)

Not really a problem, but that can surely be easily fixed.

Comment 53

[Mike Hommey [:glandium]]

Filed bug 628593 and bug 628595

Comment 54

[Takanori MATSUURA]

(In reply to comment #52)
> (In reply to comment #50)
> It's only a linux-specific build tool, portability is not exactly important
> here. It doesn't end up in the final binary.
> It's interesting, though, that your compiler doesn't handle the presence of
> both -fexceptions and -fno-exceptions like others.

Ah, compiler option I set is complicated.
-fno-exceptions ... -fexceptions ... -fno-exceptions
So I suppose -fno-exceptions is finally set.

I found that I manually set -fno-exceptions (-fexceptions is replaced
with -fno-exceptions in Fedora xulrunner spec file).
When I remove my -fno-exceptions, the build goes well

Comment 55

[Daniel C]

This was disabled in bug 637243. The bug to re-enable it is bug 637317.