Skip to content

build: Fix shared lib linking for darwin with lld #27628

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 14, 2023
Merged

build: Fix shared lib linking for darwin with lld #27628

merged 2 commits into from
May 14, 2023

Conversation

theuni
Copy link
Member

@theuni theuni commented May 11, 2023

Solves one of the last remaining blockers for #21778. Fixes lld linking shared libs for macos via libtool.

lld fails one of libtool's earliest checks because it happens to output a warning that contains a specific string:

# If there is a non-empty error log, and "single_module"
# appears in it, assume the flag caused a linker warning

And here is the test being run:

x86_64-apple-darwin-ld: warning: Option `-single_module' is deprecated in ld64:
x86_64-apple-darwin-ld: warning: Unnecessary option: this is already the default

Because the warning is printed the test fails. So libtool falls back to a very primitive and broken link-line for shared libs.

Arguably this should be worked-around in upstream lld by changing the warning string, as otherwise every libtool project will fail to link with it.

Like many other libtool hacks, the solution is to simply disable the check and hard-code the answer we know to be correct.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 11, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK hebasto
Stale ACK fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #21778 (build: LLVM 15 & LLD based macOS toolchain by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@theuni
Copy link
Member Author

theuni commented May 11, 2023

Ping @fanquake

And @hebasto, because I know you love libtool hacks ;)

@fanquake
Copy link
Member

Concept ACK. Note that this is also shown to be working in #21778, as the build there is succeeding.

@hebasto
Copy link
Member

hebasto commented May 11, 2023

Solves one of the last remaining blockers for #21778.

Concept ACK on that.

And @hebasto, because I know you love libtool hacks ;)

I believe we'll manage to switch to CMake earlier :)

@theuni
Copy link
Member Author

theuni commented May 11, 2023

And @hebasto, because I know you love libtool hacks ;)

I believe we'll manage to switch to CMake earlier :)

Hahaha, you're probably right about that. But at least this way there's no "well it didn't work with autotools so it doesn't have to work with CMake" excuse :p

@DrahtBot DrahtBot mentioned this pull request May 12, 2023
Merged
@fanquake
Copy link
Member

Guix Build:

0e6772d00e899aea446bbda37b198142812b95830e4c8f52bde9992efd8bc570  guix-build-d576d69e139b/output/aarch64-linux-gnu/SHA256SUMS.part
f0b4ac500389b012f78c20043adef975e2a33b8e079bbee07b3e4c130c4f67a9  guix-build-d576d69e139b/output/aarch64-linux-gnu/bitcoin-d576d69e139b-aarch64-linux-gnu-debug.tar.gz
51e611952c543a7ad9933934c6b2160703ae2ebe36804b30446a874e49f85130  guix-build-d576d69e139b/output/aarch64-linux-gnu/bitcoin-d576d69e139b-aarch64-linux-gnu.tar.gz
feab71eebaf317b8fde885319ecec81a41b5b93b52903e6fa13b4f62f68f9edc  guix-build-d576d69e139b/output/arm-linux-gnueabihf/SHA256SUMS.part
5c442c9cf04be0c280e4491621bc66231b4de7423127b5ef65b80424e874dd09  guix-build-d576d69e139b/output/arm-linux-gnueabihf/bitcoin-d576d69e139b-arm-linux-gnueabihf-debug.tar.gz
f03d625e11b081962bc00c8b24120364187721f989f2f091a217893db11806b1  guix-build-d576d69e139b/output/arm-linux-gnueabihf/bitcoin-d576d69e139b-arm-linux-gnueabihf.tar.gz
e73edcd42c9c26fb52e7312a94f7c022780f8a90a528c0df2a82146ba9777735  guix-build-d576d69e139b/output/arm64-apple-darwin/SHA256SUMS.part
c2d3e008545367e6423405d06a187e104e1e90b92736a92ed2853244a669bc07  guix-build-d576d69e139b/output/arm64-apple-darwin/bitcoin-d576d69e139b-arm64-apple-darwin-unsigned.dmg
84d5aa8ab2a0054645042f1f4ea46bc6a8aaf1b1de1387ff1fc9954858d6cf4d  guix-build-d576d69e139b/output/arm64-apple-darwin/bitcoin-d576d69e139b-arm64-apple-darwin-unsigned.tar.gz
3c1fe0c80e43e0d2bdb3b77b4a4b5a421da66f5ca9612ccd001b3e1aa9689245  guix-build-d576d69e139b/output/arm64-apple-darwin/bitcoin-d576d69e139b-arm64-apple-darwin.tar.gz
cd2a0246f584d3b8513774e42b90c5ee35c5144dad9548d9406c91d0da694504  guix-build-d576d69e139b/output/dist-archive/bitcoin-d576d69e139b.tar.gz
1a21b4654f394188fe0f9c009948aefafded6c9b1c760c033f5f8f510e6155f9  guix-build-d576d69e139b/output/powerpc64-linux-gnu/SHA256SUMS.part
f92d446bf1bc69e54ba420ca343579ea647b75b1defebecb483324484bf7a6b4  guix-build-d576d69e139b/output/powerpc64-linux-gnu/bitcoin-d576d69e139b-powerpc64-linux-gnu-debug.tar.gz
a3c05d8e41a820470cfb07fc94ea64852972a0991863482cec386a70bf25fdfc  guix-build-d576d69e139b/output/powerpc64-linux-gnu/bitcoin-d576d69e139b-powerpc64-linux-gnu.tar.gz
a056a393e1b3d5b85f95348492ff6f2f56a1de048e326873d057e29b2a1d43c8  guix-build-d576d69e139b/output/powerpc64le-linux-gnu/SHA256SUMS.part
0cb8154495093f0613158d8a6a6db6a6b54476afe0a38aa5cca31a3d00846a03  guix-build-d576d69e139b/output/powerpc64le-linux-gnu/bitcoin-d576d69e139b-powerpc64le-linux-gnu-debug.tar.gz
489a25c63aee2182c1a3a01bf63c4440ffbb9b205eb273e28600333dcdf1b2dc  guix-build-d576d69e139b/output/powerpc64le-linux-gnu/bitcoin-d576d69e139b-powerpc64le-linux-gnu.tar.gz
69a34c5ab8829db0f1f88b8e139cfc34e8238a75285df24a71a494ddd6a33bc5  guix-build-d576d69e139b/output/riscv64-linux-gnu/SHA256SUMS.part
f2e528e5b5e1466672d831d4b78f50b0c85ff961b219df8de9abd0e9654e5ec2  guix-build-d576d69e139b/output/riscv64-linux-gnu/bitcoin-d576d69e139b-riscv64-linux-gnu-debug.tar.gz
c485b0b75d86a4c9e333e928bb6831f8ced838cf8f3d0194903a81de7761b4b8  guix-build-d576d69e139b/output/riscv64-linux-gnu/bitcoin-d576d69e139b-riscv64-linux-gnu.tar.gz
232355e5924eb6dad8765e7d5e19cdc62120ce09e637850cb8034367d38dc4a0  guix-build-d576d69e139b/output/x86_64-apple-darwin/SHA256SUMS.part
48cb47215c031015bc3a506eaef24b9810753a9237b51cbb8276a38f8f21011e  guix-build-d576d69e139b/output/x86_64-apple-darwin/bitcoin-d576d69e139b-x86_64-apple-darwin-unsigned.dmg
da702b5d4445f583a44c10abb7fececad9f97b55fa6ef3a5c4357b223e4f7a82  guix-build-d576d69e139b/output/x86_64-apple-darwin/bitcoin-d576d69e139b-x86_64-apple-darwin-unsigned.tar.gz
2a70bc718eefa59f6971a4969cc4a8a98b050ef01a9d136fcabc19bc1da952d8  guix-build-d576d69e139b/output/x86_64-apple-darwin/bitcoin-d576d69e139b-x86_64-apple-darwin.tar.gz
1954b4e92fb96b06ef97f023fdebee996ad321589ac1ee706629c231d49b3777  guix-build-d576d69e139b/output/x86_64-linux-gnu/SHA256SUMS.part
5c0b420a436d338c3eedeb8bef593ab6aa1fd21cfe491d478873c9b0db7c82a3  guix-build-d576d69e139b/output/x86_64-linux-gnu/bitcoin-d576d69e139b-x86_64-linux-gnu-debug.tar.gz
4f802923dba5901277beaf35281e0533e3c33aef485504dfc7efabcbf200189a  guix-build-d576d69e139b/output/x86_64-linux-gnu/bitcoin-d576d69e139b-x86_64-linux-gnu.tar.gz
223707e17c7eade2c7abb7fafefba4e0bbb9f58b9cd9a601415453570e67adb1  guix-build-d576d69e139b/output/x86_64-w64-mingw32/SHA256SUMS.part
bfc40b409516aa894d0fc83ebdaf753a789e644d49eda96c49f01f49445e6bc5  guix-build-d576d69e139b/output/x86_64-w64-mingw32/bitcoin-d576d69e139b-win64-debug.zip
bce4093ba28ac431355ebe6873fce630010e6007ea5a04f4fe284087b9c7e48d  guix-build-d576d69e139b/output/x86_64-w64-mingw32/bitcoin-d576d69e139b-win64-setup-unsigned.exe
8c5a9de26c5074dda6ec442719ccc411de798a07421a0cd84f212e7a96518778  guix-build-d576d69e139b/output/x86_64-w64-mingw32/bitcoin-d576d69e139b-win64-unsigned.tar.gz
d9703b9550cf5abbfe5231a5abcb6191ccc8639d7b7379db8a5fafb65611bf17  guix-build-d576d69e139b/output/x86_64-w64-mingw32/bitcoin-d576d69e139b-win64.zip

fanquake
fanquake approved these changes May 12, 2023
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d576d69 - also tested via 21778

hebasto
hebasto approved these changes May 12, 2023
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d576d69

Suggesting to move the mingw-related comment "By default, libtool for mingw refuses..." below the *mingw*) case.

This commit has been already incorporated into the #21778 draft.

theuni added 2 commits May 12, 2023 19:13
@theuni
build: Fix shared lib linking for darwin with lld
283d955 
libtool gets a false-positive from the warning produced by lld -single_module
because it is already the default and unneeded.

Skip the check unconditionally for Darwin linkers.
@theuni
build: cleanup comments after adding yet another libtool hack
67aacc7
@theuni theuni force-pushed the fix-lld-macos-liblink branch from d576d69 to 67aacc7 Compare May 12, 2023 19:17
@theuni
Copy link
Member Author

theuni commented May 12, 2023

Updated the original commit to quote "yes" to match the others.

Also fixed up the comments as @hebasto suggested.

hebasto
hebasto approved these changes May 12, 2023
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK 67aacc7

Guix builds:

8a8fb0b28786ca39ce748520df30008933aefd6fa10538207aa93991c7ab7c41  guix-build-67aacc73ea42/output/aarch64-linux-gnu/SHA256SUMS.part
a3f07b89ce463bfdaa91aa8240dfdbc62ffdeb734155739aa72928e205147ccf  guix-build-67aacc73ea42/output/aarch64-linux-gnu/bitcoin-67aacc73ea42-aarch64-linux-gnu-debug.tar.gz
27d111b4924c99b798bc821aa475994d280a6d1a76d4c62c12cde10b7d0e61df  guix-build-67aacc73ea42/output/aarch64-linux-gnu/bitcoin-67aacc73ea42-aarch64-linux-gnu.tar.gz
06a230d990bb532791a0302f3b358ca5870292ab8567aa4d9a388b57db255567  guix-build-67aacc73ea42/output/arm-linux-gnueabihf/SHA256SUMS.part
17261ee10e6fc2ccb94af1244afa480d1339caf9198dd8605b653524e3a9f5ed  guix-build-67aacc73ea42/output/arm-linux-gnueabihf/bitcoin-67aacc73ea42-arm-linux-gnueabihf-debug.tar.gz
20d52a7fac6f8707176c97cfe20fa798d5ecb3d6d846131bb5f600db9b6ffc85  guix-build-67aacc73ea42/output/arm-linux-gnueabihf/bitcoin-67aacc73ea42-arm-linux-gnueabihf.tar.gz
4ab662fdb1c444d209a4020b53b040354c69b2156b5616efe33425d3fe4331f2  guix-build-67aacc73ea42/output/arm64-apple-darwin/SHA256SUMS.part
73bef363949048734e2914e1ff6e83c4abf9eba088dcd66a4cf38760534fd100  guix-build-67aacc73ea42/output/arm64-apple-darwin/bitcoin-67aacc73ea42-arm64-apple-darwin-unsigned.dmg
b8d3977bd3364bdffe8015957c129fcbb07ab6b50b5c162ecfe21b63cea19ba3  guix-build-67aacc73ea42/output/arm64-apple-darwin/bitcoin-67aacc73ea42-arm64-apple-darwin-unsigned.tar.gz
c277a5459452ace5cd25b880ae59f9e0b904a289477858592f5cfaadddddb677  guix-build-67aacc73ea42/output/arm64-apple-darwin/bitcoin-67aacc73ea42-arm64-apple-darwin.tar.gz
2e65b5e14587aea8c03d3c56c1e6a76f3adbd357613ea674971fb43ca1521251  guix-build-67aacc73ea42/output/dist-archive/bitcoin-67aacc73ea42.tar.gz
30ff3653682c7999b946ee4a0a7b6032bda46c7a33c5b26e56913d22456fc0d3  guix-build-67aacc73ea42/output/powerpc64-linux-gnu/SHA256SUMS.part
1ba2c855e98b6b407552763cc10e528662da9766f7a7da54482232bbdad6a2a2  guix-build-67aacc73ea42/output/powerpc64-linux-gnu/bitcoin-67aacc73ea42-powerpc64-linux-gnu-debug.tar.gz
f78e13691faeadf06666e5def88c5dc1ffe0ebfbf5f8b22de2473170adc7459d  guix-build-67aacc73ea42/output/powerpc64-linux-gnu/bitcoin-67aacc73ea42-powerpc64-linux-gnu.tar.gz
b0f38ff145bb98df70384cca301a54a32e6ac824aebb3dd2f122cbf79f042499  guix-build-67aacc73ea42/output/powerpc64le-linux-gnu/SHA256SUMS.part
973f504f31c2abd5de42e86c3a84f3d9939faf71e4605b31ba615684fc61d742  guix-build-67aacc73ea42/output/powerpc64le-linux-gnu/bitcoin-67aacc73ea42-powerpc64le-linux-gnu-debug.tar.gz
c79147eed42443dc202aaa4e6d6f52fe6d5171cbf50d5deffa1ec1ad7f382b94  guix-build-67aacc73ea42/output/powerpc64le-linux-gnu/bitcoin-67aacc73ea42-powerpc64le-linux-gnu.tar.gz
8d1c05cf2246f58d02b2a59719b41999929f27054f29b55bf3e526beb237835e  guix-build-67aacc73ea42/output/riscv64-linux-gnu/SHA256SUMS.part
1f7bbe47902af5705202fc01f53db993191935009cdccb3e271f7d07bac9b86d  guix-build-67aacc73ea42/output/riscv64-linux-gnu/bitcoin-67aacc73ea42-riscv64-linux-gnu-debug.tar.gz
ac8a975eab24921874ecafb63832bc93e7135cfec7e5b59469eac54fdec1f0d3  guix-build-67aacc73ea42/output/riscv64-linux-gnu/bitcoin-67aacc73ea42-riscv64-linux-gnu.tar.gz
c2cbde03b51207bfb7466be9c58e8088f9c3755f4b2c484dca49bde76ef31426  guix-build-67aacc73ea42/output/x86_64-apple-darwin/SHA256SUMS.part
a730c5bcd743cb2c82e06f53da670a000cec56fbf7b00188dc8e9678db509070  guix-build-67aacc73ea42/output/x86_64-apple-darwin/bitcoin-67aacc73ea42-x86_64-apple-darwin-unsigned.dmg
bb79ba4878f20399e2a80660ca6df289e73fed7465f03d3f0d43169ec6804a13  guix-build-67aacc73ea42/output/x86_64-apple-darwin/bitcoin-67aacc73ea42-x86_64-apple-darwin-unsigned.tar.gz
d16878221be21def95810c39f7193b87833fb44a88a6f9c2307532377a1def80  guix-build-67aacc73ea42/output/x86_64-apple-darwin/bitcoin-67aacc73ea42-x86_64-apple-darwin.tar.gz
9391f51c9be4547080abdeac8f9e794114a3f62aebd7262f30e90b540fa01597  guix-build-67aacc73ea42/output/x86_64-linux-gnu/SHA256SUMS.part
9a10710d29453c7d7928815f43ecd61738fa44ec5416f8ecbc22d84637d5beb9  guix-build-67aacc73ea42/output/x86_64-linux-gnu/bitcoin-67aacc73ea42-x86_64-linux-gnu-debug.tar.gz
3ca89ac8c52a4257a671722829eed23bef3a32f7d7f1ee2fac0fccbbf1f0cdc5  guix-build-67aacc73ea42/output/x86_64-linux-gnu/bitcoin-67aacc73ea42-x86_64-linux-gnu.tar.gz
852bdd8bd85bd210221e05d20bd76a29a6ee94eb61b733d79099e763af465a89  guix-build-67aacc73ea42/output/x86_64-w64-mingw32/SHA256SUMS.part
b365b33519c49c556dafeffbb88ab91ebc1f40f506ed6ce63e939e268179a847  guix-build-67aacc73ea42/output/x86_64-w64-mingw32/bitcoin-67aacc73ea42-win64-debug.zip
91a735c8a433c56ed279bbbfc3bb9dbd830e7018c6149be70904f85cf433e759  guix-build-67aacc73ea42/output/x86_64-w64-mingw32/bitcoin-67aacc73ea42-win64-setup-unsigned.exe
fe4aec6c5a40261cdb209b2fd23f56fec2e03661cb680b9193d285873d08e6a8  guix-build-67aacc73ea42/output/x86_64-w64-mingw32/bitcoin-67aacc73ea42-win64-unsigned.tar.gz
1988d5e8f6b4588da0421566fda60671b70a5a1237ed837b0b06f5ac65140a6f  guix-build-67aacc73ea42/output/x86_64-w64-mingw32/bitcoin-67aacc73ea42-win64.zip

@DrahtBot DrahtBot requested a review from fanquake May 12, 2023 21:29
@fanquake fanquake merged commit 8bfe503 into bitcoin:master May 14, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 15, 2023
@fanquake
Merge bitcoin#27628: build: Fix shared lib linking for darwin with lld
1d95fa1 
67aacc7 build: cleanup comments after adding yet another libtool hack (Cory Fields)
283d955 build: Fix shared lib linking for darwin with lld (Cory Fields)

Pull request description:

  Solves one of the last remaining blockers for bitcoin#21778. Fixes lld linking shared libs for macos via libtool.

  lld fails one of libtool's earliest checks [because it happens to output a warning that contains a specific string](https://git.savannah.gnu.org/cgit/libtool.git/tree/m4/libtool.m4#n999):

  >     # If there is a non-empty error log, and "single_module"
  >     # appears in it, assume the flag caused a linker warning

  And here is the test being run:
  > x86_64-apple-darwin-ld: warning: Option `-single_module' is deprecated in ld64:
  > x86_64-apple-darwin-ld: warning: Unnecessary option: this is already the default

  Because the warning is printed the test fails. So libtool falls back to a very primitive and broken link-line for shared libs.

  Arguably this should be worked-around in upstream lld by changing the warning string, as otherwise every libtool project will fail to link with it.

  Like many other libtool hacks, the solution is to simply disable the check and hard-code the answer we know to be correct.

ACKs for top commit:
  hebasto:
    re-ACK 67aacc7

Tree-SHA512: 792e4d208a3a4921edb5f267f43ecd052b5b650df0db5cb2788ee1e4f3c4087413f354b22e407ff5fa2f99a22a16154ec6826d14c6654a57c00aae3b3e744bca
@theuni theuni mentioned this pull request May 22, 2023
Merged
fanquake added a commit that referenced this pull request May 25, 2023
@fanquake
Merge #27721: depends: remove redundant stdlib option
25202ca 
4fe5f3c depends: remove redundant stdlib option (Cory Fields)

Pull request description:

  Like #27628, this is another dependency of #21778, though it doesn't become obvious until used with a newer clang.

  This should be a no-op.

  Use of -stdlib++-isystem gets rid of any system c++ header include paths and negates the need for this option. In newer versions of clangs the combo produces an annoying warning that actually causes problems during configure.

ACKs for top commit:
  fanquake:
    ACK 4fe5f3c

Tree-SHA512: 904072f2b13dffbbeab2bc9ff20a74969888502fa1ea35d9030784dd397c6751e31233e6ec7dc34e9fd42574950be733b25d6653c2a93e214cc5e4eef2e0133a
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 26, 2023
@fanquake
Merge bitcoin#27721: depends: remove redundant stdlib option
4acbb43 
4fe5f3c depends: remove redundant stdlib option (Cory Fields)

Pull request description:

  Like bitcoin#27628, this is another dependency of bitcoin#21778, though it doesn't become obvious until used with a newer clang.

  This should be a no-op.

  Use of -stdlib++-isystem gets rid of any system c++ header include paths and negates the need for this option. In newer versions of clangs the combo produces an annoying warning that actually causes problems during configure.

ACKs for top commit:
  fanquake:
    ACK 4fe5f3c

Tree-SHA512: 904072f2b13dffbbeab2bc9ff20a74969888502fa1ea35d9030784dd397c6751e31233e6ec7dc34e9fd42574950be733b25d6653c2a93e214cc5e4eef2e0133a
@bitcoin bitcoin locked and limited conversation to collaborators May 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto approved these changes

@fanquake fanquake Awaiting requested review from fanquake

Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@theuni @DrahtBot @fanquake @hebasto