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

build: pass -dead_strip_dylibs to ld on macOS #17663

Merged
merged 1 commit into from Dec 6, 2019

Conversation

@fanquake
Copy link
Member

fanquake commented Dec 3, 2019

This strips some unused dylibs from bitcoin-qt.

otool -L src/qt/bitcoin-qt
  /usr/lib/libSystem.B.dylib
- /System/Library/Frameworks/DiskArbitration.framework/Versions/A/DiskArbitration
  /System/Library/Frameworks/IOKit.framework/Versions/A/IOKit
  /System/Library/Frameworks/Foundation.framework/Versions/C/Foundation
  /System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices
  /System/Library/Frameworks/AppKit.framework/Versions/C/AppKit
  /System/Library/Frameworks/ApplicationServices.framework/Versions/A/ApplicationServices
  /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation 
-/System/Library/Frameworks/Security.framework/Versions/A/Security 
  /System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration 
  /System/Library/Frameworks/CoreGraphics.framework/Versions/A/CoreGraphics 
  /System/Library/Frameworks/OpenGL.framework/Versions/A/OpenGL 
-/System/Library/Frameworks/AGL.framework/Versions/A/AGL 
  /System/Library/Frameworks/Carbon.framework/Versions/A/Carbon 
  /usr/lib/libc++.1.dylib 
  /System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork 
  /System/Library/Frameworks/CoreText.framework/Versions/A/CoreText 
  /System/Library/Frameworks/ImageIO.framework/Versions/A/ImageIO
  /usr/lib/libobjc.A.dylib

AGL - ObjC wrapper for OpenGL.
DiskArbitration - mount/unmount notifications and events.
Security - low level security operations, authentication services.

From man ld:

Remove dylibs that are unreachable by the entry point or exported symbols. 
That is, suppresses the generation of load command commands for dylibs 
which supplied no symbols during the link. This option should not be 
used when linking against a dylib which is required at runtime for 
some indirect reason such as the dylib has an important initializer.
This strips some unused dylibs from bitcoin-qt.

From man ld:
Remove dylibs that are unreachable by the entry point or exported symbols. 
That is, suppresses the generation of load command commands for dylibs 
which supplied no symbols during the link. This option should not be 
used when linking against a dylib which is required at runtime for 
some indirect reason such as the dylib has an important initializer.
@promag

This comment has been minimized.

Copy link
Member

promag commented Dec 4, 2019

Any guess why those were included in the first place?

Concept ACK.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Dec 4, 2019

Concept ACK: less cruft is better

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 4, 2019

I think it's, in principle, nice to depend on fewer system libs but…

Remove dylibs that are unreachable by the entry point or exported symbols.

How risky is this option? Worded like this, it seems like quite an extensive analysis pass (checking the call tree all the way from the entry point?—does this take into account indirect/lazy dependencies through vtables, function pointers, threads, etc?).

E.g. is there a chance of false negatives? what will happen?

@theuni
theuni approved these changes Dec 4, 2019
Copy link
Member

theuni left a comment

ACK bd44711.

@laanwj I agree with the caution here, but I think this is a good idea. I believe Gnu ld/gold already work this way by default. I actually lost $1 to @fanquake on a bet about this at lunch yesterday, because I thought -dead_strip was already doing this.

Note that we're already using -dead_strip, which goes pretty far:

-dead_strip
            Remove functions and data that are unreachable by the entry point or exported symbols.

This just goes a step further and doesn't add the dylib entry in the binary, so we only need to worry about potential startup side-effects. And if those do exist, I think we'd like to flesh them out.

Related: Are there any parts of qt in depends that we can disable to prevent these from being added?

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Dec 4, 2019

Gitian builds

File commit bce4408
(master)
commit ec03741
(master and this pull)
bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz 40caf742a2674966... 760aba924759c652...
bitcoin-0.19.99-aarch64-linux-gnu.tar.gz 34d078b55cc81ffa... 2f49d30de054fc8b...
bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz 0e4bfdc2db429350... 07241fd3f20d4986...
bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz 66f9553d61083c86... efc10188d84af908...
bitcoin-0.19.99-i686-pc-linux-gnu-debug.tar.gz b94d6da73b0724fc... ebb12a6160a05aa5...
bitcoin-0.19.99-i686-pc-linux-gnu.tar.gz 9a7eb5bfc03dc3c7... 8ca72c6582062d1f...
bitcoin-0.19.99-osx-unsigned.dmg 080fc64ce42ac922... 11df1880e894c631...
bitcoin-0.19.99-osx64.tar.gz 25c44e85f72a9718... 7a88927eaf77b9fa...
bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz ebf248bcad5d1362... 60284e9b7403ce94...
bitcoin-0.19.99-riscv64-linux-gnu.tar.gz 90d60c22deb5c3d9... 1a3b6357ae77b887...
bitcoin-0.19.99-win64-debug.zip 4b7041137b8551bb... b6299b1fcb569bb3...
bitcoin-0.19.99-win64-setup-unsigned.exe fafa36315d943962... c109107172ceed8a...
bitcoin-0.19.99-win64.zip 45fd276418a2e762... 3176d7f8f440a405...
bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz ad62fd169f3b94f0... ac37accb9fc2780b...
bitcoin-0.19.99-x86_64-linux-gnu.tar.gz dcbdcfabbc11788d... a63bf5c01da603a4...
bitcoin-0.19.99.tar.gz bb241ff718f90db7... f8f4bacfe4d45608...
bitcoin-core-linux-0.20-res.yml f0e1f78f4c673bfb... da3737e3cb1edc25...
bitcoin-core-osx-0.20-res.yml e7372ee66c33c5b6... f453ced912847847...
bitcoin-core-win-0.20-res.yml b99b459febc7ce3a... 3d27c25efa0fa60f...
linux-build.log 1d19bc4efecc2987... 0a4083ec9c4344eb...
osx-build.log 05276d8599182586... 6da7423d4f13031b...
win-build.log a1f5423cb7e486dc... 8e88920e8ac87f49...
bitcoin-core-linux-0.20-res.yml.diff 72b308ceb1cec962...
bitcoin-core-osx-0.20-res.yml.diff d274105d23eb9b74...
bitcoin-core-win-0.20-res.yml.diff 6d57343059d853cf...
linux-build.log.diff d40c8aa828fd6df2...
osx-build.log.diff 71c66f8d79ce952e...
win-build.log.diff eefa6f3e788ebddd...
@fanquake

This comment has been minimized.

Copy link
Member Author

fanquake commented Dec 4, 2019

Related: Are there any parts of qt in depends that we can disable to prevent these from being added?

I'll look at this as a follow up.

laanwj added a commit that referenced this pull request Dec 6, 2019
bd44711 build: pass -dead_strip_dylibs to ld on macOS (fanquake)

Pull request description:

  This strips some unused dylibs from bitcoin-qt.

  ```diff
  otool -L src/qt/bitcoin-qt
    /usr/lib/libSystem.B.dylib
  - /System/Library/Frameworks/DiskArbitration.framework/Versions/A/DiskArbitration
    /System/Library/Frameworks/IOKit.framework/Versions/A/IOKit
    /System/Library/Frameworks/Foundation.framework/Versions/C/Foundation
    /System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices
    /System/Library/Frameworks/AppKit.framework/Versions/C/AppKit
    /System/Library/Frameworks/ApplicationServices.framework/Versions/A/ApplicationServices
    /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
  -/System/Library/Frameworks/Security.framework/Versions/A/Security
    /System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration
    /System/Library/Frameworks/CoreGraphics.framework/Versions/A/CoreGraphics
    /System/Library/Frameworks/OpenGL.framework/Versions/A/OpenGL
  -/System/Library/Frameworks/AGL.framework/Versions/A/AGL
    /System/Library/Frameworks/Carbon.framework/Versions/A/Carbon
    /usr/lib/libc++.1.dylib
    /System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork
    /System/Library/Frameworks/CoreText.framework/Versions/A/CoreText
    /System/Library/Frameworks/ImageIO.framework/Versions/A/ImageIO
    /usr/lib/libobjc.A.dylib
  ```

  `AGL` - ObjC wrapper for OpenGL.
  `DiskArbitration` - mount/unmount notifications and events.
  `Security` - low level security operations, authentication services.

  From `man ld`:
  ```
  Remove dylibs that are unreachable by the entry point or exported symbols.
  That is, suppresses the generation of load command commands for dylibs
  which supplied no symbols during the link. This option should not be
  used when linking against a dylib which is required at runtime for
  some indirect reason such as the dylib has an important initializer.
  ```

ACKs for top commit:
  theuni:
    ACK bd44711.

Tree-SHA512: 9592ce2966d28cb6c58e01efd401f56a4baa5dc5be5313f4fe8454632b578608be65a23c8602772049cd4655a9cb020fdd40d6622a244c301920d8c3db43f99a
@laanwj laanwj merged commit bd44711 into bitcoin:master Dec 6, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fanquake fanquake deleted the fanquake:macos_ld_dead_strip_dylibs branch Dec 6, 2019
sidhujag added a commit to syscoin/syscoin that referenced this pull request Dec 6, 2019
bd44711 build: pass -dead_strip_dylibs to ld on macOS (fanquake)

Pull request description:

  This strips some unused dylibs from bitcoin-qt.

  ```diff
  otool -L src/qt/bitcoin-qt
    /usr/lib/libSystem.B.dylib
  - /System/Library/Frameworks/DiskArbitration.framework/Versions/A/DiskArbitration
    /System/Library/Frameworks/IOKit.framework/Versions/A/IOKit
    /System/Library/Frameworks/Foundation.framework/Versions/C/Foundation
    /System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices
    /System/Library/Frameworks/AppKit.framework/Versions/C/AppKit
    /System/Library/Frameworks/ApplicationServices.framework/Versions/A/ApplicationServices
    /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
  -/System/Library/Frameworks/Security.framework/Versions/A/Security
    /System/Library/Frameworks/SystemConfiguration.framework/Versions/A/SystemConfiguration
    /System/Library/Frameworks/CoreGraphics.framework/Versions/A/CoreGraphics
    /System/Library/Frameworks/OpenGL.framework/Versions/A/OpenGL
  -/System/Library/Frameworks/AGL.framework/Versions/A/AGL
    /System/Library/Frameworks/Carbon.framework/Versions/A/Carbon
    /usr/lib/libc++.1.dylib
    /System/Library/Frameworks/CFNetwork.framework/Versions/A/CFNetwork
    /System/Library/Frameworks/CoreText.framework/Versions/A/CoreText
    /System/Library/Frameworks/ImageIO.framework/Versions/A/ImageIO
    /usr/lib/libobjc.A.dylib
  ```

  `AGL` - ObjC wrapper for OpenGL.
  `DiskArbitration` - mount/unmount notifications and events.
  `Security` - low level security operations, authentication services.

  From `man ld`:
  ```
  Remove dylibs that are unreachable by the entry point or exported symbols.
  That is, suppresses the generation of load command commands for dylibs
  which supplied no symbols during the link. This option should not be
  used when linking against a dylib which is required at runtime for
  some indirect reason such as the dylib has an important initializer.
  ```

ACKs for top commit:
  theuni:
    ACK bd44711.

Tree-SHA512: 9592ce2966d28cb6c58e01efd401f56a4baa5dc5be5313f4fe8454632b578608be65a23c8602772049cd4655a9cb020fdd40d6622a244c301920d8c3db43f99a
laanwj added a commit that referenced this pull request Dec 9, 2019
2359a47 depends: don't use OpenGL in Qt on macOS (fanquake)
ba0cad2 build: pass -dead_strip_dylibs to ld on macOS (fanquake)

Pull request description:

  Based on #17663. OpenGL on macOS was also deprecated in 10.14.

  This also removes the `/System/Library/Frameworks/OpenGL.framework/Versions/A/OpenGL` dylib from `bitcoin-qt`.

ACKs for top commit:
  laanwj:
    ACK 2359a47
  jonasschnelli:
    utACK 2359a47
  hebasto:
    ACK 2359a47

Tree-SHA512: 39b0151832c829f6ebdc4910eb28ebbeba64539cd04eba6ce3ec75fc0f231569956ca51a1e0bffc76dd27e85643c65a155320b9b450c49e9841e12b108406d41
sidhujag added a commit to syscoin/syscoin that referenced this pull request Dec 9, 2019
2359a47 depends: don't use OpenGL in Qt on macOS (fanquake)
ba0cad2 build: pass -dead_strip_dylibs to ld on macOS (fanquake)

Pull request description:

  Based on bitcoin#17663. OpenGL on macOS was also deprecated in 10.14.

  This also removes the `/System/Library/Frameworks/OpenGL.framework/Versions/A/OpenGL` dylib from `bitcoin-qt`.

ACKs for top commit:
  laanwj:
    ACK 2359a47
  jonasschnelli:
    utACK 2359a47
  hebasto:
    ACK 2359a47

Tree-SHA512: 39b0151832c829f6ebdc4910eb28ebbeba64539cd04eba6ce3ec75fc0f231569956ca51a1e0bffc76dd27e85643c65a155320b9b450c49e9841e12b108406d41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.