-
Notifications
You must be signed in to change notification settings - Fork 36.4k
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
macOS: Bump minimum required runtime version and prepare for building with upstream LLVM #27676
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Concept ACK - note that this also needs a change to |
Concept ACK. |
Note that you'll also need to update our minimum version check in: bitcoin/contrib/devtools/symbol-check.py Line 235 in 904631e
|
Thanks. Done. I used #22993 as a template for bumping, we'll see if c-i is happy. |
Pushed a test for |
Nothing. symbol/security checks are no-longer run in the CI, because the CI environment is semi-regularly changing, and the binaries produced don't represent release binaries, meaning there are some tests that will never pass. |
The CI failure is unrelated, and will be fixed post-rebase. Guix currently fails to build. i.e: time HOSTS="x86_64-apple-darwin arm64-apple-darwin" ./contrib/guix/guix-build
<snip>
Extracting libevent...
/home/ubuntu/sources/libevent-2.1.12-stable.tar.gz: OK
Preprocessing libevent...
Configuring libevent...
checking for a BSD-compatible install... /home/ubuntu/.guix-profile/bin/install -c
checking whether build environment is sane... yes
checking for x86_64-apple-darwin-strip... x86_64-apple-darwin-strip
checking for a thread-safe mkdir -p... /home/ubuntu/.guix-profile/bin/mkdir -p
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking whether make supports nested variables... yes
checking whether make supports nested variables... (cached) yes
checking whether make supports the include directive... yes (GNU style)
checking for x86_64-apple-darwin-gcc... env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH -u LIBRARY_PATH /home/ubuntu/.guix-profile/bin/clang --target=x86_64-apple-darwin -mmacosx-version-min=11.0 -B/bitcoin/depends/x86_64-apple-darwin/native/bin -mlinker-version=609 -isysroot/home/ubuntu/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers -Xclang -internal-externc-isystem -Xclang /gnu/store/2vd3s80pipf4dffzrf4niphl2mdhrjx1-clang-11.0.0/lib/clang/11.0.0/include -Xclang -internal-externc-isystem -Xclang /home/ubuntu/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers/usr/include
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... yes
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH -u LIBRARY_PATH /home/ubuntu/.guix-profile/bin/clang --target=x86_64-apple-darwin -mmacosx-version-min=11.0 -B/bitcoin/depends/x86_64-apple-darwin/native/bin -mlinker-version=609 -isysroot/home/ubuntu/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers -Xclang -internal-externc-isystem -Xclang /gnu/store/2vd3s80pipf4dffzrf4niphl2mdhrjx1-clang-11.0.0/lib/clang/11.0.0/include -Xclang -internal-externc-isystem -Xclang /home/ubuntu/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers/usr/include accepts -g... yes
checking for env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH -u LIBRARY_PATH /home/ubuntu/.guix-profile/bin/clang --target=x86_64-apple-darwin -mmacosx-version-min=11.0 -B/bitcoin/depends/x86_64-apple-darwin/native/bin -mlinker-version=609 -isysroot/home/ubuntu/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers -Xclang -internal-externc-isystem -Xclang /gnu/store/2vd3s80pipf4dffzrf4niphl2mdhrjx1-clang-11.0.0/lib/clang/11.0.0/include -Xclang -internal-externc-isystem -Xclang /home/ubuntu/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers/usr/include option to accept ISO C89... none needed
checking whether env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH -u LIBRARY_PATH /home/ubuntu/.guix-profile/bin/clang --target=x86_64-apple-darwin -mmacosx-version-min=11.0 -B/bitcoin/depends/x86_64-apple-darwin/native/bin -mlinker-version=609 -isysroot/home/ubuntu/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers -Xclang -internal-externc-isystem -Xclang /gnu/store/2vd3s80pipf4dffzrf4niphl2mdhrjx1-clang-11.0.0/lib/clang/11.0.0/include -Xclang -internal-externc-isystem -Xclang /home/ubuntu/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers/usr/include understands -c and -o together... yes
checking dependency style of env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH -u LIBRARY_PATH /home/ubuntu/.guix-profile/bin/clang --target=x86_64-apple-darwin -mmacosx-version-min=11.0 -B/bitcoin/depends/x86_64-apple-darwin/native/bin -mlinker-version=609 -isysroot/home/ubuntu/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers -Xclang -internal-externc-isystem -Xclang /gnu/store/2vd3s80pipf4dffzrf4niphl2mdhrjx1-clang-11.0.0/lib/clang/11.0.0/include -Xclang -internal-externc-isystem -Xclang /home/ubuntu/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers/usr/include... none
checking how to run the C preprocessor... /lib/cpp
configure: error: in `/bitcoin/depends/work/build/x86_64-apple-darwin/libevent/2.1.12-stable-3b53fa1ad55':
configure: error: C preprocessor "/lib/cpp" fails sanity check
See `config.log' for more details
make: *** [funcs.mk:292: /bitcoin/depends/x86_64-apple-darwin/.libevent_stamp_configured] Error 1
make: Leaving directory '/bitcoin/depends' config.log https://gist.github.com/fanquake/e0e1cc9f2976eb745dbb92a69c350994: configure:4403: checking how to run the C preprocessor
configure:4434: env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH -u LIBRARY_PATH /home/ubuntu/.guix-profile/bin/clang --target=x86_64-apple-darwin -mmacosx-version-min=11.0 -B/bitcoin/depends/x86_64-apple-darwin/native/bin -mlinker-version=609 -isysroot/home/ubuntu/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers -Xclang -internal-externc-isystem -Xclang /gnu/store/2vd3s80pipf4dffzrf4niphl2mdhrjx1-clang-11.0.0/lib/clang/11.0.0/include -Xclang -internal-externc-isystem -Xclang /home/ubuntu/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers/usr/include -E -I/bitcoin/depends/x86_64-apple-darwin/include -D_FORTIFY_SOURCE=3 conftest.c
In file included from conftest.c:13:
In file included from /gnu/store/2vd3s80pipf4dffzrf4niphl2mdhrjx1-clang-11.0.0/lib/clang/11.0.0/include/limits.h:21:
In file included from /gnu/store/5h2w4qi9hk1qzzgi1w83220ydslinr4s-glibc-2.33/include/limits.h:26:
In file included from /gnu/store/5h2w4qi9hk1qzzgi1w83220ydslinr4s-glibc-2.33/include/bits/libc-header-start.h:33:
/gnu/store/5h2w4qi9hk1qzzgi1w83220ydslinr4s-glibc-2.33/include/features.h:397:4: warning: _FORTIFY_SOURCE requires compiling with optimization (-O) [-W#warnings]
# warning _FORTIFY_SOURCE requires compiling with optimization (-O)
^
In file included from conftest.c:13:
In file included from /gnu/store/2vd3s80pipf4dffzrf4niphl2mdhrjx1-clang-11.0.0/lib/clang/11.0.0/include/limits.h:21:
In file included from /gnu/store/5h2w4qi9hk1qzzgi1w83220ydslinr4s-glibc-2.33/include/limits.h:195:
In file included from /gnu/store/5h2w4qi9hk1qzzgi1w83220ydslinr4s-glibc-2.33/include/bits/posix1_lim.h:161:
/gnu/store/5h2w4qi9hk1qzzgi1w83220ydslinr4s-glibc-2.33/include/bits/local_lim.h:38:10: fatal error: 'linux/limits.h' file not found
#include <linux/limits.h>
^~~~~~~~~~~~~~~~
1 warning and 1 error generated. |
Converting to draft while I investigate the clang issue. |
It took several days to track down, but... I believe the issue with clang >=11 comes from: llvm/llvm-project@6fa3894 . From my local tests I'm able to revert that and get back to expected results. So I guess we'll need to patch it out of guix. |
Suggestions for how to go about this are welcome. It's not at all straightforward to me how to patch out. The llvm toolchain bumps are currently blocked on this :( |
edb6ac0
to
24ce1d6
Compare
Added a custom forked guix repo containing the patch we need for llvm's build. I believe I've fixed authorization and it should just work(tm). My local guix build works now, but I've reworked things so many times it would be helpful to get a confirmation. Going to mark as ready for review since guix builds might actually work now. |
See relevant #25098. If we go forward with this we'll want the guix fork as part of our github org rather than my personal repo. |
FWIW, though we didn't end up needing it, an upstream guix "fix" is slotted to go in as well, which we can pick up in some future time-machine bump. That gives us the ability to customize our clang build if we ever need to in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 3df6070
However, Xcode 13 Release Notes state:
What effect will the |
It would be nice if someone confirms that Guix binaries run on macOS 11 BigSur. |
Guix build
|
Guix (self-signed) |
Yeah, that's a good point. The docs (and a quick glimpse at the code) shows that it should work fine on macOS 11, but as it wasn't default there it's worth confirming. I'm also unable to test as I'm on 13.x :( |
Guix DMG tested on macOS Monterey 12.6.7 (Intel). |
I can confirm that my guix dmg runs on macOS 11.1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 3df6070
ACK 3df6070 |
Guix builds:
|
ACK 3df6070 |
7f96638 contrib: add macOS fixup_chains check to security-check (fanquake) 3dca683 build: support -no_fixup_chains in ld64 (fanquake) Pull request description: Followup to #27676, adding the check for chained fixups. Somewhat annoyingly, we have to patch support for `-no_fixup_chains` into ld64. As it doesn't seem to have been added [until a later version](https://github.com/apple-oss-distributions/ld64/blob/59a99ab60399c5e6c49e6945a9e1049c42b71135/src/ld/Options.cpp#L4172). Guix Build: ```bash 0e17d462808f86aa7157e27a957da88fd1adeb491ad6c01138aca93e5ad1d018 guix-build-7f96638723a0/output/arm64-apple-darwin/SHA256SUMS.part ceb208e6374f5d7367b73128e90ca6eaeea15d50c69e49c8cf75b47212525ad7 guix-build-7f96638723a0/output/arm64-apple-darwin/bitcoin-7f96638723a0-arm64-apple-darwin-unsigned.dmg e31663554cfde8a37a9f3438c9c895dde94b90ff87e28f12f78be71ef6421d93 guix-build-7f96638723a0/output/arm64-apple-darwin/bitcoin-7f96638723a0-arm64-apple-darwin-unsigned.tar.gz 68a7bbc42418641eab391a85725b5c2f3c46d38a7acc07e7a8cef98909be07ec guix-build-7f96638723a0/output/arm64-apple-darwin/bitcoin-7f96638723a0-arm64-apple-darwin.tar.gz 38d966ad93e7384f4f1ce16faded003a675ecce7be1987e6c4eee8e4b82c0432 guix-build-7f96638723a0/output/dist-archive/bitcoin-7f96638723a0.tar.gz 9d314f595d897a715a321a9fba0d552220fbd4bf69aff84eb8c0001cdb48234f guix-build-7f96638723a0/output/x86_64-apple-darwin/SHA256SUMS.part c218ebfd0e96348c4912e6d522492b621bb043ef45b75105ff1fde979d1004d0 guix-build-7f96638723a0/output/x86_64-apple-darwin/bitcoin-7f96638723a0-x86_64-apple-darwin-unsigned.dmg 1c5ff7fa82f5c76d7d8b9582ad5202f4a82a917102ecafdc3c1fb7b783f6bc3e guix-build-7f96638723a0/output/x86_64-apple-darwin/bitcoin-7f96638723a0-x86_64-apple-darwin-unsigned.tar.gz 15fb01e5afcc842db6a3e793b42c70c05ce07bec79e0d2d605e241901ff9f639 guix-build-7f96638723a0/output/x86_64-apple-darwin/bitcoin-7f96638723a0-x86_64-apple-darwin.tar.gz ``` ACKs for top commit: theuni: utACK 7f96638. hebasto: ACK 7f96638, I have reviewed the code and the patch, and they look OK. TheCharlatan: ACK 7f96638 Tree-SHA512: 7f94710460f54b2afe3c9f5d57107b71436c59b799b15f78e5e3011c3c4f6b23a3acc1008eccea9c22226a200774c82900bad6c6236ab6c5c48a17dec3f2d5a2
7f96638 contrib: add macOS fixup_chains check to security-check (fanquake) 3dca683 build: support -no_fixup_chains in ld64 (fanquake) Pull request description: Followup to bitcoin#27676, adding the check for chained fixups. Somewhat annoyingly, we have to patch support for `-no_fixup_chains` into ld64. As it doesn't seem to have been added [until a later version](https://github.com/apple-oss-distributions/ld64/blob/59a99ab60399c5e6c49e6945a9e1049c42b71135/src/ld/Options.cpp#L4172). Guix Build: ```bash 0e17d462808f86aa7157e27a957da88fd1adeb491ad6c01138aca93e5ad1d018 guix-build-7f96638723a0/output/arm64-apple-darwin/SHA256SUMS.part ceb208e6374f5d7367b73128e90ca6eaeea15d50c69e49c8cf75b47212525ad7 guix-build-7f96638723a0/output/arm64-apple-darwin/bitcoin-7f96638723a0-arm64-apple-darwin-unsigned.dmg e31663554cfde8a37a9f3438c9c895dde94b90ff87e28f12f78be71ef6421d93 guix-build-7f96638723a0/output/arm64-apple-darwin/bitcoin-7f96638723a0-arm64-apple-darwin-unsigned.tar.gz 68a7bbc42418641eab391a85725b5c2f3c46d38a7acc07e7a8cef98909be07ec guix-build-7f96638723a0/output/arm64-apple-darwin/bitcoin-7f96638723a0-arm64-apple-darwin.tar.gz 38d966ad93e7384f4f1ce16faded003a675ecce7be1987e6c4eee8e4b82c0432 guix-build-7f96638723a0/output/dist-archive/bitcoin-7f96638723a0.tar.gz 9d314f595d897a715a321a9fba0d552220fbd4bf69aff84eb8c0001cdb48234f guix-build-7f96638723a0/output/x86_64-apple-darwin/SHA256SUMS.part c218ebfd0e96348c4912e6d522492b621bb043ef45b75105ff1fde979d1004d0 guix-build-7f96638723a0/output/x86_64-apple-darwin/bitcoin-7f96638723a0-x86_64-apple-darwin-unsigned.dmg 1c5ff7fa82f5c76d7d8b9582ad5202f4a82a917102ecafdc3c1fb7b783f6bc3e guix-build-7f96638723a0/output/x86_64-apple-darwin/bitcoin-7f96638723a0-x86_64-apple-darwin-unsigned.tar.gz 15fb01e5afcc842db6a3e793b42c70c05ce07bec79e0d2d605e241901ff9f639 guix-build-7f96638723a0/output/x86_64-apple-darwin/bitcoin-7f96638723a0-x86_64-apple-darwin.tar.gz ``` ACKs for top commit: theuni: utACK 7f96638. hebasto: ACK 7f96638, I have reviewed the code and the patch, and they look OK. TheCharlatan: ACK 7f96638 Tree-SHA512: 7f94710460f54b2afe3c9f5d57107b71436c59b799b15f78e5e3011c3c4f6b23a3acc1008eccea9c22226a200774c82900bad6c6236ab6c5c48a17dec3f2d5a2
… prepare for building with upstream LLVM 3df6070 contrib: remove macOS lazy_bind check (fanquake) 9bc357e build: explicitly opt-in to new fixup_chains functionality for darwin (Cory Fields) fb61bc0 depends: Bump MacOS minimum runtime requirement to 11.0 (Cory Fields) c2cd472 depends: bump darwin clang to 11.1 (Cory Fields) Pull request description: This (I believe) resolves the last of the blockers for [switching us away from cctools and instead using out-of-the-box llvm and lld](bitcoin#21778) for building Darwin binaries. For now, we continue building with a pre-packaged llvm and cctools, but after this PR the clang+lld combo should just work for anyone trying it. Additionally after this PR, the new runtime `fixup_chains` behavior will be in-use, as ld64 uses it as well. The commits may seem unrelated, so in detail: lld (llvm's linker) has been a work-in-progress for Darwin for years. Recently though, it has gained nearly all of the features we require. The last missing feature from ld64, `-Wl,-bind_at_load`, is not implemented in lld; as far as I can tell [lazy loading has conceptually been replaced by fixup chains](https://www.emergetools.com/blog/posts/iOS15LaunchTime). So that means we don't need ld64's `bind_at_load` as long as lld can handle `-Wl,-fixup_chains` (which it can). I've added it to our configure as a linker option mostly so that we can see it in the logs; it's default-on as long as the minimum version is >11.0. About that: the runtime functionality required for `-Wl,-fixup_chains` [requires macOS >=11.0](https://github.com/llvm/llvm-project/blob/release/16.x/lld/MachO/Driver.cpp#L1021). Hence the commit that bumps the minimum version. Our current min runtime of `10.15` has been unsupported since September 2022, so I don't expect this bump to be controversial. Lastly, with the minimum runtime version bumped to 11.0, our current version of pre-compiled clang we use for macOS is too old to understand `-mmacosx-version-min=11.0` because it expects `=10.x`. So I've made the smallest possible bump (from 10.0.1 to 11.1.0) to a version that understands. This bump is arbitrary and unfortunate, but likely to be short-lived as we may end up replacing it with llvm+lld for v26 anyway. I've held off on bumping the SDK as I think that makes sense to do as part of the lld switch instead. ACKs for top commit: hebasto: ACK 3df6070 gruve-p: ACK bitcoin@3df6070 fanquake: ACK 3df6070 TheCharlatan: ACK 3df6070 Tree-SHA512: 0200ec4a3b88df33877ae82c15b5c04d745852550923f491a354b391cac65f88e4df116a40055c23a8cbcfcdfb9a376c6ae8fdd0e898e7b966bc213dcb5857cf
This (I believe) resolves the last of the blockers for switching us away from cctools and instead using out-of-the-box llvm and lld for building Darwin binaries.
For now, we continue building with a pre-packaged llvm and cctools, but after this PR the clang+lld combo should just work for anyone trying it. Additionally after this PR, the new runtime
fixup_chains
behavior will be in-use, as ld64 uses it as well.The commits may seem unrelated, so in detail:
lld (llvm's linker) has been a work-in-progress for Darwin for years. Recently though, it has gained nearly all of the features we require. The last missing feature from ld64,
-Wl,-bind_at_load
, is not implemented in lld; as far as I can tell lazy loading has conceptually been replaced by fixup chains.So that means we don't need ld64's
bind_at_load
as long as lld can handle-Wl,-fixup_chains
(which it can). I've added it to our configure as a linker option mostly so that we can see it in the logs; it's default-on as long as the minimum version is >11.0.About that: the runtime functionality required for
-Wl,-fixup_chains
requires macOS >=11.0. Hence the commit that bumps the minimum version. Our current min runtime of10.15
has been unsupported since September 2022, so I don't expect this bump to be controversial.Lastly, with the minimum runtime version bumped to 11.0, our current version of pre-compiled clang we use for macOS is too old to understand
-mmacosx-version-min=11.0
because it expects=10.x
. So I've made the smallest possible bump (from 10.0.1 to 11.1.0) to a version that understands. This bump is arbitrary and unfortunate, but likely to be short-lived as we may end up replacing it with llvm+lld for v26 anyway. I've held off on bumping the SDK as I think that makes sense to do as part of the lld switch instead.