Bug 33466 - benchmark whether -Wl,--gc-sections is actually a good idea
Summary: benchmark whether -Wl,--gc-sections is actually a good idea
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: All All
: low minor
Assignee: Simon McVittie
QA Contact: John (J5) Palmieri
URL:
Whiteboard:
Keywords: patch
Depends on:
Blocks: dbus-1.4
  Show dependency treegraph
 
Reported: 2011-01-25 06:47 UTC by Simon McVittie
Modified: 2011-06-07 06:39 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Allow --gc-sections to be disabled, again to work around broken toolchains (1.60 KB, patch)
2011-02-01 11:23 UTC, Simon McVittie
Details | Splinter Review
Add support for --disable-gc-sections for broken toolchains (v2) (3.09 KB, patch)
2011-02-21 09:20 UTC, Simon McVittie
Details | Splinter Review
[patch] Remove support for -Wl,--gc-sections altogether (3.50 KB, patch)
2011-06-06 02:18 UTC, Simon McVittie
Details | Splinter Review
[alternative patch] Turn off -ffunction-sections -fdata-sections -Wl,--gc-sections by default (2.23 KB, patch)
2011-06-06 02:19 UTC, Simon McVittie
Details | Splinter Review

Description Simon McVittie 2011-01-25 06:47:46 UTC
See mailing list thread started by Pavel Strashkin with Subject: dbus-daemon segfault under Solaris. It appears Solaris' ld gets overenthusiastic and deletes things we still need with that flag.

I'm tempted to say "get a better ld" but --gc-sections seems to be problematic on quite a few systems (see Bug #16621).
Comment 1 Simon McVittie 2011-01-27 10:13:08 UTC
For the record, this is
<http://lists.freedesktop.org/archives/dbus/2010-December/013847.html>.
Comment 2 Simon McVittie 2011-02-01 11:23:19 UTC
Created attachment 42822 [details] [review]
Allow --gc-sections to be disabled, again to work around broken toolchains

As with Bug #16621, I've redone the patch to make it explicit that this is a workaround for broken toolchains.
Comment 3 Simon McVittie 2011-02-21 09:20:45 UTC
Created attachment 43614 [details] [review]
Add support for --disable-gc-sections for broken toolchains (v2)

Also use AC_LINK_IFELSE rather than reinventing it as a shell function.
This was the last user of ld_supports_flag, so, delete it.
    
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=33466

(As seen here, this depends on the first of the current patches on Bug #16621 - it could be disentangled if necessary)
Comment 4 Colin Walters 2011-04-26 09:50:02 UTC
(In reply to comment #3)
> Created an attachment (id=43614) [details]
> Add support for --disable-gc-sections for broken toolchains (v2)

Looks OK, though I find myself wondering whether Havoc or whoever did the change originally benchmarked it; the GCC manual says:

     Only use these options when there are significant benefits from
     doing so.  When you specify these options, the assembler and
     linker will create larger object and executable files and will
     also be slower.  You will not be able to use `gprof' on all
     systems if you specify this option and you may have problems with
     debugging if you specify both this option and `-g'.
Comment 5 Simon McVittie 2011-04-27 08:26:42 UTC
(In reply to comment #4)
> Looks OK

Fixed in git for 1.4.10, 1.5.2

> though I find myself wondering whether Havoc or whoever did the
> change originally benchmarked it

Bug retitled and left open
Comment 6 Simon McVittie 2011-06-02 04:34:33 UTC
The version of GNU ld currently in Debian unstable seems be one of these broken toolchains - it segfaults while linking dbus-daemon on mips, mipsel, armel. Yay, rarely-used options...

Using current Debian unstable to build dbus-1.4 on x86-64, I get:

                bin    lib   libexec
gc              428K   1.2M  48K
no gc           480K   924K  292K
no gc, ld -O1   484K   928K  292K
no gc, cc -Os   424K   848K  252K

(All with --disable-tests, --disable-verbose-mode, make install-strip, sizes measured with du -sh.)

The size increase in lib with --gc-sections is mostly the static library.

The size increase in bin without --gc-sections is mostly dbus-daemon (the ancillary tools remain small, because they do so little already).

The only thing in libexec is the dbus-daemon-launch-helper. Its size goes down dramatically with --gc-sections, but it's so small anyway that I'm not convinced I care enough to keep support for --gc-sections in configure.ac.

People who particularly care about size optimization should probably experiment with different flags themselves anyway, and if armel and mips are among the platforms where --gc-sections doesn't work, I think that demonstrates that embedded users who use general-purpose toolchains don't really care that much about ±300K.

I note with amusement that our dbus-daemon and dbus-daemon-launch-helper are probably only as fat as they are because they (effectively) statically link libdbus; people who really care about size optimizations should probably look into linking them dynamically, although that'd require some combination of moving a lot of libdbus' private API to be public and/or moving some of the tools' functionality to be public API in libdbus.
Comment 7 Colin Walters 2011-06-03 10:34:41 UTC
Thanks for posting numbers!

As far as dbus-daemon duplicating libdbus; I think the most interesting thing to do would be to try http://gcc.gnu.org/wiki/LinkTimeOptimization

(Apparently gcc has a hacky version of this called "-combine" we could use right now too, though the disadvantage is it doesn't support incremental rebuilds, so it has to be a "packaging" option)
Comment 8 Simon McVittie 2011-06-06 02:14:04 UTC
(In reply to comment #7)
> As far as dbus-daemon duplicating libdbus; I think the most interesting thing
> to do would be to try http://gcc.gnu.org/wiki/LinkTimeOptimization

If you want to try it you're welcome to do so, but I'm not planning to.

I've written two alternative patches for this: one changes the default to not use -Wl,--gc-sections, and the other removes that feature completely.

Rationale for leaving --enable-gc-sections in: we currently only use it in bus/ where it's likely to be most useful; if we took it out completely, the only way to enable it would be to put it in LDFLAGS at configure time, which would mean it affected the entire tree.

Rationale for not: like the "hardening" flags on Bug #16621, this is the sort of obscure option where the maintainers of a particular toolchain/arch/OS combination are likely to know the advantages and disadvantages much better than we do - and if they don't, the right answer is probably "don't use it".

I'm currently leaning towards removing it completely, but I could be convinced either way.
Comment 9 Simon McVittie 2011-06-06 02:18:22 UTC
Created attachment 47591 [details] [review]
[patch] Remove support for -Wl,--gc-sections altogether

Packagers should only enable this flag if they have confirmed that it
actually works on their toolchain (it's the sort of rarely used feature
that frequently regresses on obscure architectures/OSs without anyone
noticing), and also confirmed that it is actually a significant size win
for their configuration.
Comment 10 Simon McVittie 2011-06-06 02:19:05 UTC
Created attachment 47592 [details] [review]
[alternative patch] Turn off -ffunction-sections -fdata-sections  -Wl,--gc-sections by default

It's an obscure feature that is often broken in toolchains for less common
architectures/OSs, and the size reduction turns out not to be particularly
significant in practice, so it seems better to make it opt-in.

Removing this feature altogether would result in a less targeted application
of -Wl,--gc-sections when used: at the moment -Wl,--gc-sections is only
applied within the bus directory (the only place it's likely to be useful)
whereas if added to LDFLAGS by a packager, it would affect the entire package.
Comment 11 Colin Walters 2011-06-06 08:53:26 UTC
Review of attachment 47592 [details] [review]:

Looks good.
Comment 12 Colin Walters 2011-06-06 08:53:29 UTC
Review of attachment 47592 [details] [review]:

Looks good.
Comment 13 Simon McVittie 2011-06-07 06:39:43 UTC
(In reply to comment #9)
> Created an attachment (id=47591) [details]
> [patch] Remove support for -Wl,--gc-sections altogether

I applied this one, based on discussion with Colin on IRC.

Fixed in git for 1.4.12, 1.5.4.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.