Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release: use static libstdc++ and disable reduced exports by default #5819

Merged
merged 6 commits into from
Mar 3, 2015

Conversation

theuni
Copy link
Member

@theuni theuni commented Feb 24, 2015

Since the release of 0.10.0, we've seen several reports of failed sanity tests due to failing to catch basic exceptions. At the moment, Debian Wheezy fails to run the official Linux binary, and FreeBSD fails to run when compiled natively. I assume that there are several other broken setups as well.

From what I've seen, the brokenness is always due to type_info visibility. Building with -fvisibility=hidden can cause problems with old/busted stdlibs. See here for some background.

It's nearly impossible to tell (without blacklisting std libs like we already do with boost) if libstdc++ is going to play nice with --enable-reduce-exports. Rather than trying, just disable it by default. Packagers can re-enable if it's known to be safe.

For Linux, when compiling against a new libstdc++ but using an older one at runtime, exceptions can again be missed. Rather than take any chances, use -static-libstdc++. This means we don't have to worry about trying to maintain a compat wrapper.

Binaries become slightly bigger (about 1M each), but not enough to worry about imo. The final compressed sizes:

before:
17416   built/linux/bitcoin-0.10.0-linux32.tar.gz
17196   built/linux/bitcoin-0.10.0-linux64.tar.gz
after:
18920   build/out/bitcoin-0.10.99-linux32.tar.gz
18804   build/out/bitcoin-0.10.99-linux64.tar.gz

Debian Wheezy is verified fixed. @koobs has verified that building without reduced exports fixes freebsd.

This is really a packager's option. While it's helpful to encourage devs to
test this option for daily builds, it's not reliable in several real-world
use-cases. Some older libstdc++ runtimes (freebsd 9, debian wheezy, for
example) fail to properly catch exceptions due to mismatched type_info.

See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19664 for more info.
Backwards-compatibility for libstdc++ is not limited to straightforward abi
changes. Symbol visibility also needs to be taken into consideration, and
that really can't be addressed simply.

Instead, just static-link libstdc++ for backwards-compat.
…travis

For Gitian releases:
  - Windows builds remain unchanged. libstdc++ was already linked statically.
  - OSX builds remain unchanged. libstdc++ is tied to the SDK and not worth
    messing with.
  - Linux builds now statically link libstdc++.

For Travis:
  - Match the previous behavior by adding --enable-reduce-exports as
  necessary.
  - Use static libstdc++ for the full Linux build.
@theuni
Copy link
Member Author

theuni commented Feb 24, 2015

ping @gmaxwell, your thoughts would be appreciated since we discussed this a few nights ago.

@gmaxwell
Copy link
Contributor

Sad to hear that visibility control is being problematic, especially as we move towards making a library out of things. In the long term I don't think we can have the library that way (or callers will use random internal symbols). In the short term sure.

The size increase is smaller than I expected; 8% is worth a reduction in consensus risk and getting binaties working on debian wheezy esp considering how unstable in the fine details stdc++ has been historically. I'm not aware of any problems with static stdc++ of the sort that libc, and many commercial applications do this already. So indeed, it sounds good to me.

@koobs
Copy link

koobs commented Feb 24, 2015

@theuni While we're here, --disable-reduced-exports in the error messaging needs to be changed to --disable-reduce-exports

@theuni
Copy link
Member Author

theuni commented Feb 24, 2015

@koobs It's fixed, see 3448b13.

@gmaxwell As far as I'm aware, all trouble is caused by old (gcc 4.2 era) libstdc++. So eventually this won't be much of a problem (or it will be replaced with newer problems...).

But more to your point, I suppose I've explained the changes here poorly. For our official releases, we still build with hidden symbols as before, we just also link libstdc++ statically. The change here in that regard is that --disable-reduced-exports becomes the default in our configure... builders can re-enable if they'd like (as we do). So it should be effectively a no-op as far as official release symbol visibility goes.

@gmaxwell
Copy link
Contributor

@theuni I followed wrt visibility and releases. I /think/ virtually no one using the software as a library will be using our release binaries, so we can expect the excessive exports being abused. I think thats acceptable for now as none of the library interface stuff is stable to begin with.

@koobs
Copy link

koobs commented Feb 24, 2015

@theuni Just for additional reference here, your statement about libstdc++ being the issue is confirmed and evidenced in the fact that whether using cc (gcc 4.2.1) or clang (3.4.1) on FreeBSD 9-STABLE, libstdc++ is linked.

clang can use both GNU libstdc++ and libc++, depending on the -stdlib= option. On 9.x it uses the former by default, on 10.x and later the latter.

@laanwj
Copy link
Member

laanwj commented Feb 24, 2015

Good catch. Again shows that shipping portable binaries for Linux is all but impossible.
Maybe something like xdg-app sandboxing can change this in the near future.

One question: Won't statically linking libstdc++ in the gitian build give problems with Qt (as we don't like that statically, and thus the dynamic qt will imports its own libstdc++)?

uqs pushed a commit to freebsd/freebsd-ports that referenced this pull request Feb 24, 2015
- Update to 0.10.0
- Split out bitcoin-cli and bitcoin-tx into net-p2p/bitcoin-utils
- Slave out bitcoin-utils and bitcoin-daemon properly
- Canonicalize MASTER_SITES to what USE_GITHUB uses
- Update COMMENT so each port is unique
- protobuf/protoc is GUI-only dependency, remove it from unconditional
  {BUILD,LIB}_DEPENDS.
- Group and sort USES/USE_*
- Deprecate USE_AUTOTOOLS in favour of USES=autoreconf
- Remove {AUTOMAKE,ACLOCAL}_ARGS accordingly
- Remove unconditional *FLAGS and replace then with OPTIONS-conditional
  ones.
- Remove unnecessary CXXFLAGS
- Add HARDENING and TESTS options, add them to OPTIONS_DEFAULT
- Add DBUS and DEBUG options
- Define OPTIONS only where they're relevant for each port
- Rejig OPTIONS descriptions for greater clarity
- Replace hard-coded SSL inc/lib flags with USE_OPENSSL variables to
  allow for switching between Base and Ports OpenSSL's.
- Use OPTIONS helpers
- Remove post-patch target
- Canonicalize CONFIGURE_ARGS, with slave port specific overrides.
- Verbosify builds (V=1)
- Remove upstreamed patches
- Add regression-test targets (requires TESTS option)
- Add --disable-reduce-exports to CONFIGURE_ARGS until upstream PR #5819
  lands, which caused sanity test failure at run-time [1]

  P.S We now pass the test suite (not including python test, which
      has an error)

Based on patch Submitted by:

 - Andriy Voskoboinyk  <s3erios gmail.com>
 - Robert Backhaus <robbak gmail.com>

[1] bitcoin/bitcoin#5819

PR:		193424
Reviewed by:	maintainer
Approved by:	maintainer


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@379779 35697150-7ecd-e111-bb59-0022644237b5
koobs added a commit to freebsd/freebsd-ports that referenced this pull request Feb 24, 2015
- Update to 0.10.0
- Split out bitcoin-cli and bitcoin-tx into net-p2p/bitcoin-utils
- Slave out bitcoin-utils and bitcoin-daemon properly
- Canonicalize MASTER_SITES to what USE_GITHUB uses
- Update COMMENT so each port is unique
- protobuf/protoc is GUI-only dependency, remove it from unconditional
  {BUILD,LIB}_DEPENDS.
- Group and sort USES/USE_*
- Deprecate USE_AUTOTOOLS in favour of USES=autoreconf
- Remove {AUTOMAKE,ACLOCAL}_ARGS accordingly
- Remove unconditional *FLAGS and replace then with OPTIONS-conditional
  ones.
- Remove unnecessary CXXFLAGS
- Add HARDENING and TESTS options, add them to OPTIONS_DEFAULT
- Add DBUS and DEBUG options
- Define OPTIONS only where they're relevant for each port
- Rejig OPTIONS descriptions for greater clarity
- Replace hard-coded SSL inc/lib flags with USE_OPENSSL variables to
  allow for switching between Base and Ports OpenSSL's.
- Use OPTIONS helpers
- Remove post-patch target
- Canonicalize CONFIGURE_ARGS, with slave port specific overrides.
- Verbosify builds (V=1)
- Remove upstreamed patches
- Add regression-test targets (requires TESTS option)
- Add --disable-reduce-exports to CONFIGURE_ARGS until upstream PR #5819
  lands, which caused sanity test failure at run-time [1]

  P.S We now pass the test suite (not including python test, which
      has an error)

Based on patch Submitted by:

 - Andriy Voskoboinyk  <s3erios gmail.com>
 - Robert Backhaus <robbak gmail.com>

[1] bitcoin/bitcoin#5819

PR:		193424
Reviewed by:	maintainer
Approved by:	maintainer
@theuni
Copy link
Member Author

theuni commented Feb 25, 2015

@laanwj I'm really unsure. As an initial data point, it appears to run fine on Debian Wheezy (where it was broken before).

Thinking through it a bit...
bitcoin-qt's own throw/catch usage should be fine because the exception types are defined internally. Qt's throw/catch usage should be fine because their types are dynamically loaded, and we don't export them.

I think the worrisome case would be if qt throws us a std::exception. In that case, I'm really not sure what definition is used. Though from a quick grep, it looks like that never happens.

That's all conjecture though. I'd like to have hard evidence. Beyond expanding our sanity checks to other std exceptions, can you think of other tests we could add here?

@laanwj
Copy link
Member

laanwj commented Feb 25, 2015

@theuni Maybe I'm misunderstanding something. I'm not only worried about exceptions, but as I see it this will result in Qt and bitcoin-qt using different versions of libstdc++. The two versions of libstdc++ may have slightly different interpretations of types. When stdc++ types are passed across the dynamic library barrier (for example for QString::fromStdStding) this could cause potential issues. Another potential cause of issues are global data structures in libstdc++.

I know this is a common issue on Windows, at least. Maybe there is some magic workaround, but it seems brittle to me. A way to invite unknown unknowns. Hence I'm not so much convinced by 'it happens to work on X'.

The recommendation when statically linking libstdc++ statically is to also link all c++ libraries statically. See e.g. http://www.trilithium.com/johan/2005/06/static-libstdc . Dynamic linking of C libraries is fine.

For Linux, when compiling against a new libstdc++ but using an older one at runtime

  • The caveman solution that comes to mind is: build against an old libstdc++.
  • Or should we bite the bullet and link Qt statically. More and more that sounds like the only robust solution (remember the OpenSSL interop issues). Static qt5 sucks on Ubuntu, but that is cosmetic and preferable to these kinds of issues.

@gmaxwell
Copy link
Contributor

Well for example, GCC 5's libstdc++ includes an ABI incompatible string type. ... but there I expect bitcoin core won't work at all on those systems unless all C++ parts are statically linked.

@laanwj
Copy link
Member

laanwj commented Feb 25, 2015

@gmaxwell Right. Running the binary on such a system would only work if Qt is statically linked as well. Another point in favor of that.

@theuni
Copy link
Member Author

theuni commented Feb 25, 2015

@laanwj I'm all for static qt if you're up for it. I'll run a quick gitian test with the above changes + static qt to see if there are any surprises.

@theuni
Copy link
Member Author

theuni commented Feb 26, 2015

Changed to static qt. Release sizes roughly doubled :(

For testing:
https://bitcoincore.org/cfields/static-qt/bitcoin-0.10.99-linux32.tar.gz
https://bitcoincore.org/cfields/static-qt/bitcoin-0.10.99-linux64.tar.gz

Works fine on Debian Squeeze 32.

binary depends:

 0x0000000000000001 (NEEDED)             Shared library: [libX11-xcb.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libX11.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libxcb.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libfontconfig.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libfreetype.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libpthread.so.0]
 0x0000000000000001 (NEEDED)             Shared library: [libdl.so.2]
 0x0000000000000001 (NEEDED)             Shared library: [librt.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libanl.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libm.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libgcc_s.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [ld-linux-x86-64.so.2]

I'm not sure whether we should build qt using -no-xcb-xlib or not. That would drop the libX11-xcb.so.1 and libX11.so.6 deps, but I'm not clear on the implications.

@sipa
Copy link
Member

sipa commented Mar 1, 2015

No opinion.

@laanwj
Copy link
Member

laanwj commented Mar 3, 2015

That would drop the libX11-xcb.so.1 and libX11.so.6 deps, but I'm not clear on the implications.

Those are extremely common libraries, and also purely C not C++, so I have no problem with having them as dependency for the GUI.

@laanwj laanwj merged commit d23b0a2 into bitcoin:master Mar 3, 2015
laanwj added a commit that referenced this pull request Mar 3, 2015
d23b0a2 depends: always use static qt5 for linux (Cory Fields)
3448b13 build: fix typo in configure help (Cory Fields)
c95ac83 gitian: fix x86_64 build with static libstdc++ (Cory Fields)
0671516 build: change reduce exports/static libstdc++ options for gitian and travis (Cory Fields)
aa36730 build: remove libstdc++ backwards-compat (Cory Fields)
3ee028f build: disable reduced exports by default (Cory Fields)
@laanwj laanwj mentioned this pull request Jun 1, 2015
laanwj added a commit that referenced this pull request Sep 10, 2018
7177e09 depends: Remove unused Qt 4 dependencies (Chun Kuan Lee)

Pull request description:

  Remove 2 unused Qt 4 dependencies: libSM and libICE since #5819

Tree-SHA512: 5d78dc1c14f38a65e70e04bbdf66cc9f357c1c36d37222e709172246be0ef275fd8ae314b176e5daa00f15a82edf4864bfccdd581cb4850b5cf725f8cdfd4ae3
PastaPastaPasta referenced this pull request in PastaPastaPasta/dash Jun 27, 2021
7177e09 depends: Remove unused Qt 4 dependencies (Chun Kuan Lee)

Pull request description:

  Remove 2 unused Qt 4 dependencies: libSM and libICE since dashpay#5819

Tree-SHA512: 5d78dc1c14f38a65e70e04bbdf66cc9f357c1c36d37222e709172246be0ef275fd8ae314b176e5daa00f15a82edf4864bfccdd581cb4850b5cf725f8cdfd4ae3
PastaPastaPasta referenced this pull request in PastaPastaPasta/dash Jun 28, 2021
7177e09 depends: Remove unused Qt 4 dependencies (Chun Kuan Lee)

Pull request description:

  Remove 2 unused Qt 4 dependencies: libSM and libICE since dashpay#5819

Tree-SHA512: 5d78dc1c14f38a65e70e04bbdf66cc9f357c1c36d37222e709172246be0ef275fd8ae314b176e5daa00f15a82edf4864bfccdd581cb4850b5cf725f8cdfd4ae3
PastaPastaPasta referenced this pull request in PastaPastaPasta/dash Jun 29, 2021
7177e09 depends: Remove unused Qt 4 dependencies (Chun Kuan Lee)

Pull request description:

  Remove 2 unused Qt 4 dependencies: libSM and libICE since dashpay#5819

Tree-SHA512: 5d78dc1c14f38a65e70e04bbdf66cc9f357c1c36d37222e709172246be0ef275fd8ae314b176e5daa00f15a82edf4864bfccdd581cb4850b5cf725f8cdfd4ae3
PastaPastaPasta referenced this pull request in PastaPastaPasta/dash Jul 1, 2021
7177e09 depends: Remove unused Qt 4 dependencies (Chun Kuan Lee)

Pull request description:

  Remove 2 unused Qt 4 dependencies: libSM and libICE since dashpay#5819

Tree-SHA512: 5d78dc1c14f38a65e70e04bbdf66cc9f357c1c36d37222e709172246be0ef275fd8ae314b176e5daa00f15a82edf4864bfccdd581cb4850b5cf725f8cdfd4ae3
Munkybooty referenced this pull request in Munkybooty/dash Jul 1, 2021
7177e09 depends: Remove unused Qt 4 dependencies (Chun Kuan Lee)

Pull request description:

  Remove 2 unused Qt 4 dependencies: libSM and libICE since dashpay#5819

Tree-SHA512: 5d78dc1c14f38a65e70e04bbdf66cc9f357c1c36d37222e709172246be0ef275fd8ae314b176e5daa00f15a82edf4864bfccdd581cb4850b5cf725f8cdfd4ae3
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants