build: Bump clang minimum supported version to 16

maflcko commented at 5:04 pm on June 10, 2024: member

Most supported operating systems ship with clang-16 (or later), so bump the minimum to that and allow new code to drop workarounds for previous clang bugs.

For reference:

https://packages.debian.org/bookworm/clang-16
https://packages.ubuntu.com/noble/clang (clang-18)
CentOS-like 8/9 Stream: All Clang versions from 16 to 17
FreeBSD 12/13: All Clang versions from 16 to 18
OpenSuse Tumbleweed ships with https://software.opensuse.org/package/clang (clang18); No idea about OpenSuse Leap

On operating systems where the clang version is not shipped by default, the user would have to use GCC, or install clang in a different way. For example:

https://packages.debian.org/bookworm/g++ (g++-12)
https://packages.ubuntu.com/jammy/g++ (g++-11)
https://apt.llvm.org/, or nix, or guix, or compile clang from source, …

Ubuntu 22.04 LTS does not ship with clang-16, so one of the above workarounds is needed there.

macOS 13 is unaffected, and the previous minimum requirement of Xcode15.0 remains, see also https://github.com/bitcoin/bitcoin/blame/b1ba1b178f501daa1afdd91f9efec34e5ec1e294/.github/workflows/ci.yml#L93. For macOS 11 (Big Sur) and 12 (Monterey) you need to install a more recent version of llvm, this remains unchanged as well, see https://github.com/bitcoin/bitcoin/blame/b1ba1b178f501daa1afdd91f9efec34e5ec1e294/doc/build-osx.md#L54.

DrahtBot commented at 5:04 pm on June 10, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, TheCharlatan, stickies-v
Concept ACK	theuni, kevkevinpal, instagibbs

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:

#29071 (refactor: Remove Span operator==, Use std::ranges::equal by maflcko)

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.

DrahtBot added the label Build system on Jun 10, 2024

maflcko force-pushed on Jun 10, 2024

maflcko commented at 5:17 pm on June 10, 2024: member

This is needed for stuff:

std::ranges::equal, see #29071
std::source_location, see #28999 (comment)
…

theuni commented at 1:49 pm on June 11, 2024: member

Concept ACK.

FWIW on Ubuntu 20.04 I’m using pre-compiled binaries from https://github.com/llvm/llvm-project/releases/tag/llvmorg-18.1.4 with no issues.

maflcko commented at 8:56 am on June 12, 2024: member

FWIW on Ubuntu 20.04 I’m using pre-compiled binaries from https://github.com/llvm/llvm-project/releases/tag/llvmorg-18.1.4 with no issues.

Yeah, seems fine for test systems. I didn’t want to suggest it, because https://discourse.llvm.org/t/rfc-improve-binary-security/78121/56 is still WIP.

maflcko force-pushed on Jun 12, 2024

fanquake commented at 9:52 am on June 12, 2024: member

Should this be dropped: https://github.com/bitcoin/bitcoin/blob/5ee6b76c69d51158c13f6ad9ea1b264372e58d4d/src/test/fuzz/fuzz.cpp#L75

maflcko commented at 2:43 pm on June 12, 2024: member

Should this be dropped:

Not sure what is wrong with Apple, but I don’t have any insight into their XCode/Apple-Clang versioning and source code, so I think I’ll just clarify in this comment that this needs Apple-Clang 16 (the meaning of which is unclear).

maflcko commented at 2:47 pm on June 12, 2024: member

Ref:

0/Applications/Xcode_15.0.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/map:1282:24: note: in instantiation of function template specialization 'std::__tree<std::__value_type<std::string_view, FuzzTarget>, std::__map_value_compare<std::string_view, std::__value_type<std::string_view, FuzzTarget>, std::less<std::string_view>>, std::allocator<std::__value_type<std::string_view, FuzzTarget>>>::__emplace_unique_key_args<std::string_view, const std::piecewise_construct_t &, std::tuple<const std::string_view &>, std::tuple<std::function<void (Span<const unsigned char>)> &&, FuzzTargetOptions &&>>' requested here
1        return __tree_.__emplace_unique_key_args(__k,
2                       ^
3test/fuzz/fuzz.cpp:75:40: note: in instantiation of function template specialization 'std::map<std::string_view, FuzzTarget>::try_emplace<std::function<void (Span<const unsigned char>)>, FuzzTargetOptions>' requested here
4    const auto [it, ins]{FuzzTargets().try_emplace(name, std::move(target), std::move(opts))};
5                                       ^
6test/fuzz/fuzz.cpp:62:8: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided

https://github.com/bitcoin/bitcoin/actions/runs/9481386652/job/26124061545?pr=30263#step:7:2611

maflcko force-pushed on Jun 12, 2024

fanquake commented at 1:46 pm on June 13, 2024: member

Looks like the Apple Clang shipping with Xcode 15, is based on Clang 15. Xcode 16 (still in beta) will ship with a Apple Clang based on Clang 16.

kevkevinpal commented at 1:49 am on June 18, 2024: contributor

Concept ACK fa58c75

I’m on fedora and was able to build properly

 0uname -a
 1Linux fedora 6.2.15-300.fc38.x86_64 [#1](/bitcoin-bitcoin/1/) SMP PREEMPT_DYNAMIC Thu May 11 17:37:39 UTC 2023 x86_64 GNU/Linux
 2
 3clang --version
 4clang version 16.0.6 (Fedora 16.0.6-4.fc38)
 5Target: x86_64-redhat-linux-gnu
 6Thread model: posix
 7InstalledDir: /usr/bin
 8
 9clang++ --version
10clang version 16.0.6 (Fedora 16.0.6-4.fc38)
11Target: x86_64-redhat-linux-gnu
12Thread model: posix
13InstalledDir: /usr/bin

maflcko commented at 5:57 am on June 18, 2024: member

Looks like the Apple Clang shipping with Xcode 15, is based on Clang 15. Xcode 16 (still in beta) will ship with a Apple Clang based on Clang 16.

Well, for some reason it can compile ranges fine, without further changes, so at least to some extend it seems to be based on llvm 16 (maybe an intermediate random commit from the main dev branch)?

In any case, I am not changing the Xcode/macOS/Apple requirements here. Someone else can do it, if there is need for it and it is acceptable to do.

maflcko added the label DrahtBot Guix build requested on Jun 18, 2024

DrahtBot commented at 2:07 pm on June 18, 2024: contributor

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

File	commit 41544b8f96dbc9c6b8998acd6522200d67cdc16d(master)	commit 1d5a7b9540abc1f3662c1261e9d01ab1d744476d(master and this pull)
SHA256SUMS.part	`51e3228f4fe34c50...`	`47668e5cab69e29a...`
*-aarch64-linux-gnu-debug.tar.gz	`b3184bb6b00080fd...`	`ab7e231f4fd0b0ff...`
*-aarch64-linux-gnu.tar.gz	`70388ef31f3937e4...`	`6fa2e1b59e94dd81...`
*-arm-linux-gnueabihf-debug.tar.gz	`16e36a216e800e2b...`	`67c7cbdaeb32c81a...`
*-arm-linux-gnueabihf.tar.gz	`1ac17a158dffb672...`	`b5e68b2b4c0482ec...`
*-arm64-apple-darwin-unsigned.tar.gz	`ea99a5cc62e8c1fc...`	`c1e167272783f1af...`
*-arm64-apple-darwin-unsigned.zip	`672f2ab09bb45d66...`	`d3777eca3a5d9f09...`
*-arm64-apple-darwin.tar.gz	`df62083ab10be612...`	`c1ebdcd1577da3e3...`
*-powerpc64-linux-gnu-debug.tar.gz	`1a110f6135e4a742...`	`6972e945972cb41b...`
*-powerpc64-linux-gnu.tar.gz	`cdf683143ed86807...`	`c6ab842668701fac...`
*-riscv64-linux-gnu-debug.tar.gz	`9455864b53af0bb4...`	`c68c243cc7eb7f3f...`
*-riscv64-linux-gnu.tar.gz	`1325614aca564540...`	`d48fac828916c47c...`
*-x86_64-apple-darwin-unsigned.tar.gz	`50f1e5a287060f32...`	`7e0c0f31821c5d63...`
*-x86_64-apple-darwin-unsigned.zip	`54f3602f26ee19fb...`	`cef4b75a5ab1cea7...`
*-x86_64-apple-darwin.tar.gz	`d1a87f6623cee8da...`	`097efa57a48b0d8e...`
*-x86_64-linux-gnu-debug.tar.gz	`22afb6df6de53801...`	`62e5b89ae2615be3...`
*-x86_64-linux-gnu.tar.gz	`1b7ed787619fe2f4...`	`e5862e78786c3673...`
*.tar.gz	`33f4147997977ed3...`	`d5026ae59585c049...`
guix_build.log	`8886fbf0c68bc044...`	`c719841248b743c1...`
guix_build.log.diff		`3094b7482dd32565...`

DrahtBot removed the label DrahtBot Guix build requested on Jun 18, 2024

TheCharlatan approved

TheCharlatan commented at 1:24 pm on June 25, 2024: contributor

ACK fa58c75880334cc58ccc8e5d491df8f0e587bf42

DrahtBot requested review from theuni on Jun 25, 2024

build: Bump clang minimum supported version to 16 fa7462c67a

fuzz: Clarify Apple-Clang-16 workaround 9999dbc1bd

refactor: Remove no longer needed clang-15 workaround for std::span fa8f53273c

maflcko force-pushed on Jun 26, 2024

maflcko commented at 7:59 pm on June 26, 2024: member

Rebased to remove a workaround that was added in the meantime.

hebasto commented at 8:35 pm on June 27, 2024: member

@maflcko @TheCharlatan

Not a blocker at all, I just wonder if there is a verified recipe to get clang-16 for RISC-V platform?

maflcko commented at 9:10 pm on June 27, 2024: member

Not a blocker at all, I just wonder if there is a verified recipe to get clang-16 for RISC-V platform?

Not sure what you mean, but this depends on your OS. You can install it normally. For example:

0# apt install clang-16 && clang-16 --version 
1Reading package lists... Done
2Building dependency tree... Done
3Reading state information... Done
4clang-16 is already the newest version (1:16.0.6-23ubuntu4).
50 upgraded, 0 newly installed, 0 to remove and 69 not upgraded.
6Ubuntu clang version 16.0.6 (23ubuntu4)
7Target: riscv64-unknown-linux-gnu
8Thread model: posix
9InstalledDir: /usr/bin

But this seems unrelated to this pull, or am I missing something?

hebasto approved

hebasto commented at 2:49 pm on June 28, 2024: member

ACK fa8f53273c7e5965620d31a8c3fe5f223cb76888, I have reviewed the code and it looks OK.

DrahtBot requested review from TheCharlatan on Jun 28, 2024

TheCharlatan approved

TheCharlatan commented at 3:30 pm on June 28, 2024: contributor

Re-ACK fa8f53273c7e5965620d31a8c3fe5f223cb76888

in src/test/fuzz/fuzz.cpp:75 in fa8f53273c

71@@ -72,8 +72,8 @@ auto& FuzzTargets()
72 
73 void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target, FuzzTargetOptions opts)
74 {
75-    const auto it_ins{FuzzTargets().try_emplace(name, FuzzTarget /* temporary can be dropped after clang-16 */ {std::move(target), std::move(opts)})};
76-    Assert(it_ins.second);
77+    const auto [it, ins]{FuzzTargets().try_emplace(name, FuzzTarget /* temporary can be dropped after Apple-Clang-16 ? */ {std::move(target), std::move(opts)})};

hebasto commented at 3:31 pm on June 28, 2024:

I can confirm that the GHA macos-14 image with Xcode 16.0 (beta) (build 16A5171c) does not require this workaround.

maflcko commented at 10:28 am on July 3, 2024:

Thanks for checking. I’d say dropping support for macos-13+xcode should be done in a follow-up (probably far into the future).

Or was it meant as a nit to remove the ? in this line of code? If yes, I’ll address it, if I have to retouch or rebase.

hebasto commented at 10:56 am on July 3, 2024:

I did not mean any changes to the current branch :)

It was just a note (mostly for myself).

fanquake commented at 11:32 am on July 4, 2024: member

@instagibbs @sr-gi maybe @mzumsande. This will likely affect you. Any feedback?

stickies-v approved

stickies-v commented at 7:38 pm on July 5, 2024: contributor

ACK fa8f53273c7e5965620d31a8c3fe5f223cb76888

My review shouldn’t count for much as I’m not too familiar with build system stuff, but:

I want clang-16
it works for me locally
it doesn’t seem to break anything
the changes look good

instagibbs commented at 10:34 am on July 8, 2024: member

concept ACK

sr-gi commented at 1:53 pm on July 8, 2024: member

@instagibbs @sr-gi maybe @mzumsande. This will likely affect you. Any feedback?

I’m really not familiar with the build system, but I was able to build this without any changes in my setup on macOS 14.5 with clang 15.0.0

mzumsande commented at 3:15 pm on July 8, 2024: contributor

@instagibbs @sr-gi maybe @mzumsande. This will likely affect you. Any feedback?

Thanks - no objections, I’ve updated everywhere, so no longer affected by this.

fanquake merged this on Jul 8, 2024

fanquake closed this on Jul 8, 2024

maflcko deleted the branch on Jul 8, 2024

hebasto added the label Needs CMake port on Jul 13, 2024

hebasto commented at 8:01 pm on July 15, 2024: member

Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/263.

hebasto removed the label Needs CMake port on Jul 15, 2024

build: Bump clang minimum supported version to 16 #30263

Code Coverage

Reviews

Conflicts

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]