The default bug view has changed. See this FAQ.

Remove dead symbols with linker

RESOLVED FIXED

Status

()

Core
Build Config
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: (dormant account), Assigned: glandium)

Tracking

(Blocks: 2 bugs)

unspecified
x86
Windows NT
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ts])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Whiteboard: [ts]
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
(Reporter)

Comment 2

7 years ago
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.
OS: Windows 7 → Windows NT
Blocks: 447581
(Reporter)

Comment 3

7 years ago
Created attachment 444922 [details] [diff] [review]
gc sections
Assignee: nobody → tglek
Attachment #444922 - Flags: review?(ted.mielczarek)
Attachment #444922 - Flags: review?(ted.mielczarek) → review+
(Reporter)

Updated

7 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/bfcdd30e82bf
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5

Comment 5

7 years ago
Isn't there a footprint key word or bug to put this against? Also perf key word?
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?
(Reporter)

Comment 7

7 years ago
backed out http://hg.mozilla.org/mozilla-central/rev/714faf8ec149
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.)
(Reporter)

Comment 9

7 years ago
since we link stuff that matters via CXX we can pass linker flags there too.
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?
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.
"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.
(Assignee)

Comment 13

6 years ago
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).
(Assignee)

Comment 14

6 years ago
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...
(Assignee)

Comment 15

6 years ago
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%)
(Assignee)

Comment 16

6 years ago
(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.
(Assignee)

Comment 17

6 years ago
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.
Attachment #542130 - Flags: review?(tglek)
(Assignee)

Comment 18

6 years ago
(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.
(Assignee)

Comment 19

6 years ago
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
Attachment #542205 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 20

6 years ago
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
Attachment #542205 - Flags: review?(ted.mielczarek) → review+
(Reporter)

Updated

6 years ago
Attachment #542130 - Flags: review?(tglek) → review+
(Assignee)

Comment 21

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/e986c0494f1a
http://hg.mozilla.org/integration/mozilla-inbound/rev/ebb9315861a4
http://hg.mozilla.org/integration/mozilla-inbound/rev/2ac11e401c56

http://hg.mozilla.org/users/mh_glandium.org/elfhack/rev/7754b956ac63
Assignee: tglek → mh+mozilla
Whiteboard: [ts] → [ts] [inbound]
Target Milestone: mozilla1.9.3a5 → ---
(Assignee)

Comment 22

6 years ago
Backed out part 1 because of mochitest regressions :(
http://hg.mozilla.org/integration/mozilla-inbound/rev/69bdf578a5dd
Whiteboard: [ts] [inbound] → [ts]
(Assignee)

Comment 23

6 years ago
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.
Attachment #542411 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

6 years ago
Attachment #444922 - Attachment is obsolete: true
Comment on attachment 542411 [details] [diff] [review]
part 1 - Remove dead symbols in linker.

Review of attachment 542411 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #542411 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 25

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/f2a0b73c0bef
Whiteboard: [ts] → [ts] [inbound]

Comment 26

6 years ago
(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?
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
Whiteboard: [ts] [inbound] → [ts]
(Assignee)

Comment 28

6 years ago
Part 1 has been relanded on mozilla-inbound (comment 25)
Whiteboard: [ts] → [ts] [inbound]
http://hg.mozilla.org/mozilla-central/rev/f2a0b73c0bef
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [ts] [inbound] → [ts]
(Reporter)

Comment 30

6 years ago
Mike, 
This broke --enable-debug on linux. Gdb can't find line numbers anymore
(Assignee)

Comment 31

6 years ago
(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?
(Assignee)

Comment 32

6 years ago
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.
(Assignee)

Comment 33

6 years ago
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.
(Assignee)

Comment 34

6 years ago
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... :-/
(Assignee)

Comment 35

6 years ago
This one however fails:
gcc version 4.4.5 20101112 (Red Hat 4.4.5-2) (GCC) 
(version from fedora 13)
(Assignee)

Updated

6 years ago
Depends on: 670659
(Assignee)

Updated

6 years ago
Depends on: 675618
You need to log in before you can comment on or make changes to this bug.