Last Comment Bug 537857 - Remove dead symbols with linker
: Remove dead symbols with linker
Status: RESOLVED FIXED
[ts]
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: x86 Windows NT
: -- normal (vote)
: ---
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on: 670659 675618
Blocks: 447581 536427
  Show dependency treegraph
 
Reported: 2010-01-04 18:15 PST by (dormant account)
Modified: 2011-08-01 07:26 PDT (History)
14 users (show)
khuey: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
gc sections (568 bytes, patch)
2010-05-12 11:18 PDT, (dormant account)
ted: review+
Details | Diff | Splinter Review
part 2 - Avoid --gc-sections removing anything from elfhack test case (1.56 KB, patch)
2011-06-27 05:14 PDT, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Splinter Review
part 3 - Fix assert failure in dump_syms in some cases when the linker discarded symbols (1.56 KB, patch)
2011-06-27 10:40 PDT, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Splinter Review
part 1 - Remove dead symbols in linker. (1.39 KB, patch)
2011-06-28 03:35 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review

Description (dormant account) 2010-01-04 18:15:28 PST
On Linux  -fdata-sections -ffunction-sections  gcc flags and -Wl,--gc-sections results in a 5% smaller libxul.so

Windows has similar options accessible via /Gy compiler flag and /OPT:REF linker flags.
Comment 1 Ted Mielczarek [:ted.mielczarek] 2010-01-05 04:21:25 PST
We use -OPT:REF when building with MOZ_DEBUG_SYMBOLS (which our nightlies and release builds use):
http://mxr.mozilla.org/mozilla-central/source/config/config.mk#258
Comment 2 (dormant account) 2010-01-05 15:20:05 PST
Ted you are right, -Gy and and -opt:ref are turned on in windows. 

This is mostly a linux issue. Mac has -dead_strip enabled, but not -ffunction-sections -fdata-sections.
Comment 3 (dormant account) 2010-05-12 11:18:30 PDT
Created attachment 444922 [details] [diff] [review]
gc sections
Comment 4 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-05-23 10:53:06 PDT
http://hg.mozilla.org/mozilla-central/rev/bfcdd30e82bf
Comment 5 Worcester12345 2010-05-24 13:15:30 PDT
Isn't there a footprint key word or bug to put this against? Also perf key word?
Comment 6 Boris Zbarsky [:bz] 2010-05-24 14:58:48 PDT
So... this patch made my Linux debug builds (--enable-debug="-g3") have no debug symbols.  Why exactly are we doing this symbol removal in debug builds?
Comment 7 (dormant account) 2010-05-24 15:22:52 PDT
backed out http://hg.mozilla.org/mozilla-central/rev/714faf8ec149
Comment 8 Ted Mielczarek [:ted.mielczarek] 2010-05-25 05:16:39 PDT
We probably want to stick the CFLAGS in MOZ_OPTIMIZE_FLAGS instead. If the LDFLAGS aren't safe to use on debug builds, then we'll have to figure something out (we don't have a MOZ_OPTIMIZE_LDFLAGS, but we could make one.)
Comment 9 (dormant account) 2010-05-25 13:57:31 PDT
since we link stuff that matters via CXX we can pass linker flags there too.
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-05-31 11:45:35 PDT
How will that work with optimized+debug?  Isn't that what we do for all our official builds, so that we have debug info for breakpad?
Comment 11 Ted Mielczarek [:ted.mielczarek] 2010-06-01 07:59:54 PDT
No, we do optimized+debug symbols (--enable-optimize --enable-debug-symbols).

If we use my suggestion from comment 8, then these symbols will be stripped even if you do a --enable-debug --enable-optimize build, but I think that's ok. If you --enable-optimize, then you should recognize that the compiler is going to do funny things to your code.
Comment 12 Boris Zbarsky [:bz] 2010-06-01 08:07:08 PDT
"funny things" and "not able to debug" are not the same thing, though...  Fwiw, all my optimized builds are -g and I actually use that reasonably often, because some bugs only show up in optimized builds.  There's obviously some damage from optimization, but not as extensive as stripping debug symbols completely.
Comment 13 Mike Hommey [:glandium] 2011-06-03 20:51:32 PDT
This might well have been a gcc bug.

I did two try runs with this patch applied:
- one with gcc 4.5: http://tbpl.mozilla.org/?tree=Try&rev=dde2135a5c39
- one with gcc 4.3: http://tbpl.mozilla.org/?tree=Try&rev=5957b0abdf30

The resulting opt builds apparently all have all symbols, but debug build with gcc 4.3 lacks symbols for libxul.so while the one with gcc 4.5 doesn't.

I think we can re-enable this patch, provided we find some reliable way to automatically opt-out on broken gcc (which means finding exactly what's going wrong).
Comment 14 Mike Hommey [:glandium] 2011-06-03 22:03:03 PDT
Mmmmm it actually only failed on linux x86 debug, not linux x64 debug, and it was dump_syms not supporting some of the dwarf content:

dump_syms: ../../../../../../toolkit/crashreporter/google-breakpad/src/common/dwarf_cu_to_module.cc:770: void google_breakpad::DwarfCUToModule::AssignLinesToFunctions(): Assertion `!line || current < line->address || within(*line, current)' failed.

I'll do another try with gcc 4.3 without PGO...
Comment 15 Mike Hommey [:glandium] 2011-06-03 22:16:35 PDT
As for sizes:

                    Before     After     Difference
linux libxul.so    21747668  20809236   938432 (4.31%)
linux .tar.bz2     15389810  14984793   405017 (2.63%)

linux64 libxul.so  27209600  25887424  1322176 (4.85%)
linux64 .tar.bz2   17002585  16545591   456994 (2.68%)
Comment 16 Mike Hommey [:glandium] 2011-06-03 22:56:53 PDT
(In reply to comment #14)
> Mmmmm it actually only failed on linux x86 debug, not linux x64 debug, and
> it was dump_syms not supporting some of the dwarf content:
> 
> dump_syms:
> ../../../../../../toolkit/crashreporter/google-breakpad/src/common/
> dwarf_cu_to_module.cc:770: void
> google_breakpad::DwarfCUToModule::AssignLinesToFunctions(): Assertion `!line
> || current < line->address || within(*line, current)' failed.
> 
> I'll do another try with gcc 4.3 without PGO...

It's better to be awake... obviously, this won't change anything, since the build was a debug build, thus without PGO.
Comment 17 Mike Hommey [:glandium] 2011-06-27 05:14:17 PDT
Created attachment 542130 [details] [diff] [review]
part 2 - Avoid --gc-sections removing anything from elfhack test case

--gc-sections creates two problems with elfhack:
- it removes some placeholders that are used to subtly modify test.so's layout to make sure some things are tested
- it makes the version of GNU ld used on the buildbots crash (segv), which is a side effect of the first problem (fwiw, the crash happens when all thread local variables are gc'ed)

Putting everything under the default visibility instead of hidden ensures that nothing is gc'ed.
Comment 18 Mike Hommey [:glandium] 2011-06-27 10:36:34 PDT
(In reply to comment #14)
> Mmmmm it actually only failed on linux x86 debug, not linux x64 debug, and
> it was dump_syms not supporting some of the dwarf content:
> 
> dump_syms:
> ../../../../../../toolkit/crashreporter/google-breakpad/src/common/
> dwarf_cu_to_module.cc:770: void
> google_breakpad::DwarfCUToModule::AssignLinesToFunctions(): Assertion `!line
> || current < line->address || within(*line, current)' failed.

From my testing, this is an actual bug in our breakpad code, which is fixed in upstream google-breakpad. The upstream issue is http://breakpad.appspot.com/227001 . I'm not sure how we hit something that is supposedly related to gold with our ancient GNU ld, but we apparently do, and the patch attached there looks like it does the right thing. I'll do some more checks that it does indeed.
Comment 19 Mike Hommey [:glandium] 2011-06-27 10:40:09 PDT
Created attachment 542205 [details] [diff] [review]
part 3 - Fix assert failure in dump_syms in some cases when the linker discarded symbols

See comment 18
Comment 20 Mike Hommey [:glandium] 2011-06-27 10:45:12 PDT
Comment on attachment 542205 [details] [diff] [review]
part 3 - Fix assert failure in dump_syms in some cases when the linker discarded symbols

<ted> if it got review upstream, then just land it with r=whoever
Comment 22 Mike Hommey [:glandium] 2011-06-28 01:07:12 PDT
Backed out part 1 because of mochitest regressions :(
http://hg.mozilla.org/integration/mozilla-inbound/rev/69bdf578a5dd
Comment 23 Mike Hommey [:glandium] 2011-06-28 03:35:31 PDT
Created attachment 542411 [details] [diff] [review]
part 1 - Remove dead symbols in linker.

The reason we had a mochitest regression is that using --gc-sections when linking the firefox binary was striping the jemalloc_stats function. That's because linking binaries doesn't export symbols, so anything that is not explicitely used is dropped. An jemalloc_stats is only there to be used by libxul.so, so for the linker it's not explicitely used...

Putting the option in DSO_LDOPTS instead of LDFLAGS ensures only shared libraries will be built with --gc-sections.
Comment 24 Ted Mielczarek [:ted.mielczarek] 2011-06-28 05:02:00 PDT
Comment on attachment 542411 [details] [diff] [review]
part 1 - Remove dead symbols in linker.

Review of attachment 542411 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 26 Worcester12345 2011-06-28 07:31:17 PDT
(In reply to comment #5)
> Isn't there a footprint key word or bug to put this against? Also perf key
> word?

Since it is blocking a "perf" meta bug, should not this also be a "perf" bug?
Comment 27 Joe Drew (not getting mail) 2011-06-28 09:16:20 PDT
Part 1 was backed out of mozilla-inbound. Parts 2 and 3 got merged in to mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/ebb9315861a4
http://hg.mozilla.org/mozilla-central/rev/2ac11e401c56
Comment 28 Mike Hommey [:glandium] 2011-06-28 09:36:06 PDT
Part 1 has been relanded on mozilla-inbound (comment 25)
Comment 29 Joe Drew (not getting mail) 2011-06-28 18:49:20 PDT
http://hg.mozilla.org/mozilla-central/rev/f2a0b73c0bef
Comment 30 (dormant account) 2011-06-30 15:14:24 PDT
Mike, 
This broke --enable-debug on linux. Gdb can't find line numbers anymore
Comment 31 Mike Hommey [:glandium] 2011-06-30 22:41:05 PDT
(In reply to comment #30)
> Mike, 
> This broke --enable-debug on linux. Gdb can't find line numbers anymore

It works for me. Are you sure you weren't using stripped binaries?
Comment 32 Mike Hommey [:glandium] 2011-07-01 12:42:47 PDT
It seems to be a problem with some versions of GNU ld, and it seems the one on buildbots is not affected nor the one currently in Debian unstable. The one in Fedora 12 is apparently affected, but I'm going to double check.
Comment 33 Mike Hommey [:glandium] 2011-07-01 15:59:50 PDT
In the end, this very much looks like a gcc problem. gcc 4.4 from fc12 fails, while gcc 4.5 from fc14 works. (gcc 4.4 from fc12 with ld from fc14 fails too).
As far as my testing goes with the build bots it looks like gcc 4.3 works too, so this might be a gcc 4.4 only issue, but I'd like to double check that.
Comment 34 Mike Hommey [:glandium] 2011-07-05 08:13:36 PDT
And it's not even a gcc 4.4 issue... gcc version 4.4.5 (Debian 4.4.5-8) gets me a working build... :-/
Comment 35 Mike Hommey [:glandium] 2011-07-05 10:57:21 PDT
This one however fails:
gcc version 4.4.5 20101112 (Red Hat 4.4.5-2) (GCC) 
(version from fedora 13)

Note You need to log in before you can comment on or make changes to this bug.