As title suggests. We already do this a different way with gitian Linux builds (this can replace the linker script we use there). This is a bit of dogfooding to ensure that dev and pull-tester builds behave the same as final releases.
Hiding symbols by default also speeds up linking quite a bit, and forces good habits should we ever decide to produce shared libs.
Grr, build failures are due to pull-tester's ancient boost. See https://svn.boost.org/trac/boost/ticket/2309
Changed it to be disabled by default.
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4663_938371c2805d3a3090faf416703e1cf942e77b29/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
Why don't we do this by default? Tagging exported functions is not a ton of work, and making it a default would avoid increasing the number of configurations which are not heavily tested.
The default was set to false because ancient Boost (used by pulltester) fails with default visibility=hidden.
Right, the issue is that if an exception is thrown across a dso boundary, the receiver's symbol must be exported in order to catch it. In practice, due to the boost bug, that'd mean that we have to export all functions that may need to catch a boost exception. Imo that's not reasonable, especially since it's due to a faulty lib.
This wouldn't affect any of our final releases, though, as they're all built with a static boost.
I would very much like to see this enabled by default as well, but it would mean refusing to build against boost < 1.49. I'd be much more comfortable with enforcing that restriction at some point in the future than trying to work around it by manually exporting many unrelated functions now.
The third option would be detecting the boost version and selectively enabling reduced exports, but I'm not sure I'd be comfortable with that either.
s/export/specify default visibility/
Well we could make it a default and just use a flag to switch it off where it doesn't work. It could also autodetect that its going to fail (E.g. with an autotools compile and link test). I am generally apposed to using autotools autodetection in ways that change user (/caller) visible functionality, but leaking private symbols is not 'functionality'. :) The only harm I see with this is that we might merge something that doesn't build for people on more modern library setups, but we'd notice that quickly enough.
@gmaxwell It's ugly, but I could get on board with that. I'm not sure a link test would be enough though, I think we'd actually have to dump the resulting symbols and verify (as opposed to a try_run which would break on cross). If that's the case, it may end up being more reasonable to just use a version test.
I'll give it a shot.
Using a doctored recent boost to test, I think I've come up with a link-test using a phony intermediary shared lib. I'll push it up tomorrow after playing some more.
Enabled automatically if boost >= 1.49.
See: https://svn.boost.org/trac/boost/ticket/2309
Also, check for a default visibility attribute, so that we can mark future
api functions correctly.
As discussed with @gmaxwell on IRC. Unfortunately, there's just no way to reliably test for broken boost visibility. Instead, I've added a check to disable reducing exports if boost is < 1.49. It's not pretty, but gets the job done.
So now reduce-exports is back to being enabled by default, and disabled if any of the criteria aren't met.
Also, added a check for default visibility attribute, so that we can properly advertise our api after #4692 is merged.
Looks good to me.
Though the reason that I used a linker script in the gitian build instead of these switches is that even with -fvisibility=hidden and -Wl,--exclude-libs,ALL some symbols exports 'leaked' out of the executable.
So without verification I'm not convinced that this can replace it.
It could be verified by running contrib/devtools/symbol-check.py on the gitian executables after removing the linker script and adding this. Then see whether you get export of symbol %s not allowed errors.
(To be clear, I'm fine with merging this as well, even if it doesn't fix the problem mentioned in the gitian descriptor, its still a good change!)
Yea, works as intended. Full output is:
bin/test_bitcoin: symbol clock_gettime from unsupported version GLIBC_2.17
bin/bitcoin-qt: export of symbol _edata not allowed
bin/bitcoin-qt: export of symbol _end not allowed
bin/bitcoin-qt: export of symbol _init not allowed
bin/bitcoin-qt: export of symbol __bss_start not allowed
bin/bitcoin-qt: export of symbol _fini not allowed
bin/bitcoin-tx: symbol clock_gettime from unsupported version GLIBC_2.17
bin/test_bitcoin-qt: export of symbol _edata not allowed
bin/test_bitcoin-qt: export of symbol _end not allowed
bin/test_bitcoin-qt: export of symbol _init not allowed
bin/test_bitcoin-qt: export of symbol __bss_start not allowed
bin/test_bitcoin-qt: export of symbol _fini not allowed
bin/bitcoin-cli: symbol clock_gettime from unsupported version GLIBC_2.17
bin/bitcoind: symbol clock_gettime from unsupported version GLIBC_2.17
The clock_gettime is an incompatibility we need to fix.
_edata, _end, _init, __bss_start, and _fini are all absolute and added by the linker itself. We should update the checker to ignore those.
The clock_gettime actually comes in from my system since it's a newer glibc (that dump above is from my dev env using our depends, not from gitian). Gitian's glibc is 2.15, so that's actually not a problem until we move gitian to trusty.
I haven't tested with gitian, but it will work exactly the same as above once we start using the new depends. I guess there's no reason to hold off on that now, so I'll PR those on Monday.
@laanwj Ok, I gave it a go on gitian, and got (presumably) the same results as you. I dug around for a bit and it looks like precise’s default linker (bfd) has busted support for “—exclude-libs ALL”. Most distros these days have switched to gold anyway. I installed that and it worked as it should’ve.
So basically, all is good if you add “binutils-gold” to the packages list in the descriptor. I’ll add that into my PR on monday after verifying that it's still deterministic.
Script output from gitian looks like this:
/home/ubuntu/out/bin/x86_64-unknown-linux-gnu/test_bitcoin-qt: export of symbol _edata not allowed
/home/ubuntu/out/bin/x86_64-unknown-linux-gnu/test_bitcoin-qt: export of symbol _end not allowed
/home/ubuntu/out/bin/x86_64-unknown-linux-gnu/test_bitcoin-qt: export of symbol _init not allowed
/home/ubuntu/out/bin/x86_64-unknown-linux-gnu/test_bitcoin-qt: export of symbol __bss_start not allowed
/home/ubuntu/out/bin/x86_64-unknown-linux-gnu/test_bitcoin-qt: export of symbol _fini not allowed
/home/ubuntu/out/bin/x86_64-unknown-linux-gnu/bitcoin-cli: export of symbol std::basic_string<char, std::char_traits<char>, std::allocator<char> > std::operator+<char, std::char_traits<char>, std::allocator<char> >(char const*, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) not allowed
/home/ubuntu/out/bin/x86_64-unknown-linux-gnu/bitcoin-cli: export of symbol std::basic_string<char, std::char_traits<char>, std::allocator<char> > std::operator+<char, std::char_traits<char>, std::allocator<char> >(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) not allowed
/home/ubuntu/out/bin/x86_64-unknown-linux-gnu/bitcoin-qt: export of symbol _edata not allowed
/home/ubuntu/out/bin/x86_64-unknown-linux-gnu/bitcoin-qt: export of symbol _end not allowed
/home/ubuntu/out/bin/x86_64-unknown-linux-gnu/bitcoin-qt: export of symbol _init not allowed
/home/ubuntu/out/bin/x86_64-unknown-linux-gnu/bitcoin-qt: export of symbol __bss_start not allowed
/home/ubuntu/out/bin/x86_64-unknown-linux-gnu/bitcoin-qt: export of symbol _fini not allowed
Looks like there’s a single bug in that version’s libstdc++ that causes the strings to be visible, but that’s nothing to worry about (and it's obviously been fixed since, since it doesn't show up for me locally).
So as far as I’m concerned, the visibility changes here are enough to get us what we’re after, assuming the deps/tools aren’t busted. Using the gold linker for precise seems to be all it takes to work around.
Thanks. I'm prepared for a few reports of build problems after this (not sure what, but something's bound to come up). If so, I'll try to get 'em worked out asap.
@theuni OOPS - seems the pulltester is failing now
http://jenkins.bluematt.me/pull-tester/p4718_88fe88cf3640caede5222380383d0816a42a73b7/test.log
And I notice that it's compiling with -fvisibility=hidden.
Maybe the boost check isn't working :(