cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET

hebasto commented at 3:45 pm on January 15, 2025: member

(Almost?) every CMake check internally uses the try_compile() command, whose behaviour, in turn, depends on the CMAKE_TRY_COMPILE_TARGET_TYPE variable:

The default value, EXECUTABLE, enables both compiler and linker checks.

The STATIC_LIBRARY value enables only compiler checks.

To mimic Autotools’ behaviour, we disabled linker checks by setting CMAKE_TRY_COMPILE_TARGET_TYPE to STATIC_LIBRARY globally (perhaps not the best design). This effectively separates the entire CMake script into regions where CMAKE_TRY_COMPILE_TARGET_TYPE is:

unset

set to STATIC_LIBRARY

set to EXECUTABLE

From #31359 (comment):

This seems very fragile and unintuitive, and the fact that this could silently break at any point is not documented in any way. I don’t think other bad design decisions should lead to us having to write even more boilerplate code to fix things that should “just work” (minus the upstream bugs).

Agreed. I forgot that we set CMAKE_TRY_COMPILE_TARGET_TYPE globally. And even worse, it’s buried in a module. If that upsets CMake internal tests, I think we should undo that.

This PR ensures that CMAKE_TRY_COMPILE_TARGET_TYPE is modified only within local scopes.

Additionally, the FUZZ_LIBS variable has been introduced to handle additional libraries required for linking, rather than link options, in certain build environment, such as OSS-Fuzz.

hebasto added the label Build system on Jan 15, 2025

DrahtBot commented at 3:45 pm on January 15, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31662.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, theuni

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:

#31802 (Add bitcoin-{node,gui} to release binaries for IPC by Sjors)
#31741 (multiprocess: Add libmultiprocess git subtree by ryanofsky)
#30975 (ci: build multiprocess on most jobs by Sjors)

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 commented at 4:06 pm on January 15, 2025: member

High-level question: Do we actually need the old autotools behavior? If this is just about shaving off some time when running the linker, I’d rather skip that optimization. Is there actually a case where a static-lib compile-test would succeed, an executable test would fail, and also we would be ok with that failure?

If not, we could just skip the complexity and not modify the policy at all.

theuni commented at 4:10 pm on January 15, 2025: member

High-level question: Do we actually need the old autotools behavior? If this is just about shaving off some time when running the linker, I’d rather skip that optimization. Is there actually a case where a static-lib compile-test would succeed, an executable test would fail, and also we would be ok with that failure?

If not, we could just skip the complexity and not modify the policy at all.

~~From a quick look at the new uses of bitcoin_check_cxx_source_compiles here, only check_evhttp_connection_get_peer would be affected.~~

Edit: Actually, that one already has CMAKE_REQUIRED_LIBRARIES set so it should be ok. @hebasto Is there an obvious reason I’m missing to not just use CMake’s default behavior here rather than fighting it?

theuni commented at 4:45 pm on January 15, 2025: member

I tried making this change (simply removing the global CMAKE_TRY_COMPILE_TARGET_TYPE assignment) and ended up with an identical bitcoin-build-config.h.

It was .4 sec slower, but that’s not enough to care about :)

hebasto commented at 4:59 pm on January 15, 2025: member

High-level question: Do we actually need the old autotools behavior?

That was a strict requirement for the staging branch, and mapping the Autotools’ AC_PREPROC_IFELSE, AC_COMPILE_IFELSE and AC_LINK_IFELSE macros to CMake functions was essential.

Is there actually a case where a static-lib compile-test would succeed, an executable test would fail, and also we would be ok with that failure?

I don’t think so.

Is there an obvious reason I’m missing to not just use CMake’s default behavior here rather than fighting it?

I’d love to live with CMake’s default behavior!

It was .4 sec slower, but that’s not enough to care about :)

It might be even slower with LTO enabled, though.

theuni commented at 5:16 pm on January 15, 2025: member

High-level question: Do we actually need the old autotools behavior?

That was a strict requirement for the staging branch, and mapping the Autotools’ AC_PREPROC_IFELSE, AC_COMPILE_IFELSE and AC_LINK_IFELSE macros to CMake functions was essential.

Sure, I didn’t mean to imply that we were doing the wrong thing here. Only that we’ve learned that it causes problems and the 1:1 matching with autotools is problematic.

Is there an obvious reason I’m missing to not just use CMake’s default behavior here rather than fighting it?

I’d love to live with CMake’s default behavior!

Proposed alternative to this PR: https://github.com/theuni/bitcoin/commits/cmake-trycompile-exe/ What do you think?

It was .4 sec slower, but that’s not enough to care about :)

It might be even slower with LTO enabled, though.

Possibly, but the test stubs are so small it can’t matter much.

hebasto force-pushed on Jan 15, 2025

hebasto commented at 5:59 pm on January 15, 2025: member

Proposed alternative to this PR: https://github.com/theuni/bitcoin/commits/cmake-trycompile-exe/ What do you think?

Thank you. It looks great!

I also pushed an updated branch, which has a cleaner resulting code.

However, I’m happy to switch to your branch, if you prefer it.

hebasto force-pushed on Jan 15, 2025

hebasto marked this as a draft on Jan 15, 2025

hebasto force-pushed on Jan 15, 2025

DrahtBot added the label CI failed on Jan 15, 2025

hebasto marked this as ready for review on Jan 15, 2025

hebasto commented at 6:37 pm on January 15, 2025: member

A typo has been fixed (

DrahtBot removed the label CI failed on Jan 15, 2025

in CMakeLists.txt:381 in 583c5af4d8 outdated

377       #include <cstdint>
378       #include <cstddef>
379       extern \"C\" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { return 0; }
380       // No main() function.
381     " FUZZ_BINARY_LINKS_WITHOUT_MAIN_FUNCTION
382+    CXXFLAGS ${SANITIZER_LDFLAGS}

theuni commented at 7:32 pm on January 15, 2025:

Could you add an LDFLAGS param to the function as LINK_OPTIONS and use that here instead?

Edit: or maybe just call it LINK_OPTIONS for symmetry.

hebasto commented at 7:59 pm on January 15, 2025:

LDFLAGS might make more sense because, for example, in the OSS-Fuzz environment, the SANITIZER_LDFLAGS variable can be set to -fsanitize=fuzzer or /usr/lib/libFuzzingEngine.a.

hebasto commented at 8:42 pm on January 15, 2025:

The LDFLAGS option has been added.

theuni commented at 7:30 pm on January 23, 2025:

OSS-Fuzz

Wait, they’re using a completely undocumented var. That shouldn’t be any reason for us to change our behavior here. That’s waaaay to brittle for us to have to worry about, no?

Why are they not using CMAKE_EXE_LINKER_FLAGS or LDFLAGS ?

hebasto commented at 10:04 am on January 24, 2025:

Why are they not using CMAKE_EXE_LINKER_FLAGS or LDFLAGS ?

Both variants will work though.

cc the OSS-Fuzz crowd: @maflcko @dergoegge @fanquake @achow101

theuni commented at 2:50 pm on January 24, 2025:

Is it not an issue to link the fuzzer lib with our non-fuzz binaries? If not, yeah, let’s just do that :)

hebasto commented at 3:08 pm on January 24, 2025:

Is it not an issue to link the fuzzer lib with our non-fuzz binaries?

I don’t think so: https://github.com/bitcoin/bitcoin/blob/2d07384243c9552aa8e95c80d7a279e2d224a753/CMakeLists.txt#L211-L231

theuni commented at 4:31 pm on January 24, 2025:

Oh, right! Still.. I could imagine it breaking CMake checks. But sure, if it works, that’d be better than hacking around it here.

in cmake/module/TestAppendRequiredLibraries.cmake:31 in 2934a7124c outdated

29-  check_cxx_source_links("${check_socket_source}" IFADDR_LINKS_WITHOUT_LIBSOCKET)
30+  include(CheckCXXSourceCompiles)
31+  check_cxx_source_compiles("${check_socket_source}" IFADDR_LINKS_WITHOUT_LIBSOCKET)
32   if(NOT IFADDR_LINKS_WITHOUT_LIBSOCKET)
33-    check_cxx_source_links_with_libs(socket "${check_socket_source}" IFADDR_NEEDS_LINK_TO_LIBSOCKET)
34+    set(CMAKE_REQUIRED_LIBRARIES socket)

theuni commented at 7:33 pm on January 15, 2025:

Could you add a LINK_LIBRARIES option to check_cxx_source_compiles_with_flags and use that here instead?

hebasto commented at 8:41 pm on January 15, 2025:

Thanks! Done.

hebasto force-pushed on Jan 15, 2025

hebasto commented at 8:40 pm on January 15, 2025: member

@theuni

Thank you for your review! Your comments have been addressed.

in cmake/module/CheckSourceCompilesAndLinks.cmake:37 in dbb91982dd outdated

39+
40+]=]
41+function(check_cxx_source_compiles_with_flags source result_var)
42+  cmake_parse_arguments(PARSE_ARGV 2 _ "" "" "CXXFLAGS;LDFLAGS;LINK_LIBRARIES")
43+  list(JOIN __CXXFLAGS " " CMAKE_REQUIRED_FLAGS)
44+  set(CMAKE_REQUIRED_LIBRARIES ${__LDFLAGS} ${__LINK_LIBRARIES})

theuni commented at 4:44 pm on January 16, 2025:

Why does CMAKE_REQUIRED_LIBRARIES get LDFLAGS rather than setting LINK_OPTIONS?

theuni commented at 4:47 pm on January 16, 2025:

Ah, I see your comment here: #31662 (review)

I think a static lib would work fine as an option though? Or -l/path/to/libfoo.a otherwise?

hebasto commented at 4:50 pm on January 16, 2025:

I think a static lib would work fine as an option though?

Correct. This is documented by CMake.

theuni commented at 7:02 pm on January 16, 2025:

I meant as LINK_OPTIONS. I’d rather have LDFLAGS forward to that unless there’s a reason not to.

hebasto commented at 8:02 pm on January 16, 2025:

Okay, I’ve checked CMake docs again.

CMAKE_REQUIRED_LINK_OPTIONS is forwarded to target_link_options(), which is designed “to add any link options”.

~~I’ll update the PR shortly.~~

hebasto commented at 8:15 pm on January 16, 2025:

I meant as LINK_OPTIONS. I’d rather have LDFLAGS forward to that unless there’s a reason not to.

In that case a static library (e.g., /usr/lib/libFuzzingEngine.a) would be placed before object files being linked, and the linker might fail.

theuni commented at 8:29 pm on January 23, 2025:

Yeah, I really don’t think we should be worrying about their busted flags.

For OSS-Fuzz, we can change it to pass: -DSANITIZER_LDFLAGS="-Wl,$LIB_FUZZING_ENGINE". That should fix the ordering problem if it’s an absolute file while still working with -lFuzzingEngine as well.

Edit: nevermind, that didn’t affect the ordering as I’d assumed it would.

theuni commented at 8:46 pm on January 23, 2025:

Blah, ok, sorry. I’ve been messing around with this. Final answer:

Let’s add a SANITIZER_LIBS var
add BOTH LINK_LIBRARIES and LDFLAGS to check_cxx_source_compiles_with_flags
pass them both for the sanitizers
update oss-fuzz to use SANITIZER_LIBS instead.

That way our stuff is well-defined and makes sense and we’re not coding around oss-fuzz, but we still expose the option we need there.

hebasto commented at 10:00 am on January 24, 2025:

cc the OSS-Fuzz crowd: @maflcko @dergoegge @fanquake @achow101

theuni commented at 6:19 pm on February 12, 2025:

@fanquake Are you ok if we break OSS-Fuzz here with a proper fix (to avoid future technical debt), and PR the one-liner to fix it there post-merge?

fanquake commented at 11:44 am on February 13, 2025:

Yes

hebasto commented at 2:49 pm on February 13, 2025:

Blah, ok, sorry. I’ve been messing around with this. Final answer:
* Let's add a `SANITIZER_LIBS` var

* add BOTH `LINK_LIBRARIES` and `LDFLAGS` to check_cxx_source_compiles_with_flags

* pass them both for the sanitizers

* update oss-fuzz to use `SANITIZER_LIBS` instead.
That way our stuff is well-defined and makes sense and we’re not coding around oss-fuzz, but we still expose the option we need there.

Reworked. A new FUZZ_LIBS variable has been added, which in turn will require a corresponding change in OSS-Fuzz scripts.

theuni added this to the milestone 29.0 on Feb 12, 2025

hebasto force-pushed on Feb 13, 2025

DrahtBot added the label CI failed on Feb 13, 2025

DrahtBot commented at 4:34 pm on February 13, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/37171033299

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

fanquake commented at 4:41 pm on February 13, 2025: member

https://github.com/bitcoin/bitcoin/pull/31662/checks?check_run_id=37171033299:

 0[10:53:16.049] gmake[2]: Entering directory '/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu'
 1[10:53:16.093] [ 72%] Linking CXX executable fuzz
 2[10:53:16.093] cd /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz && /usr/bin/cmake -E cmake_link_script CMakeFiles/fuzz.dir/link.txt --verbose=1
 3[10:53:16.117] /usr/bin/clang++-20 -ftrivial-auto-var-init=pattern -O2 -g -fsanitize=fuzzer,address,undefined,float-divide-by-zero,integer -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -fPIE -pie CMakeFiles/fuzz.dir/addition_overflow.cpp.o CMakeFiles/fuzz.dir/addrman.cpp.o CMakeFiles/fuzz.dir/asmap.cpp.o CMakeFiles/fuzz.dir/asmap_direct.cpp.o CMakeFiles/fuzz.dir/autofile.cpp.o CMakeFiles/fuzz.dir/banman.cpp.o CMakeFiles/fuzz.dir/base_encode_decode.cpp.o CMakeFiles/fuzz.dir/bech32.cpp.o CMakeFiles/fuzz.dir/bip324.cpp.o CMakeFiles/fuzz.dir/bitdeque.cpp.o CMakeFiles/fuzz.dir/bitset.cpp.o CMakeFiles/fuzz.dir/block.cpp.o CMakeFiles/fuzz.dir/block_header.cpp.o CMakeFiles/fuzz.dir/block_index.cpp.o CMakeFiles/fuzz.dir/blockfilter.cpp.o CMakeFiles/fuzz.dir/bloom_filter.cpp.o CMakeFiles/fuzz.dir/buffered_file.cpp.o CMakeFiles/fuzz.dir/chain.cpp.o CMakeFiles/fuzz.dir/checkqueue.cpp.o CMakeFiles/fuzz.dir/cluster_linearize.cpp.o CMakeFiles/fuzz.dir/coins_view.cpp.o CMakeFiles/fuzz.dir/coinscache_sim.cpp.o CMakeFiles/fuzz.dir/connman.cpp.o CMakeFiles/fuzz.dir/crypto.cpp.o CMakeFiles/fuzz.dir/crypto_aes256.cpp.o CMakeFiles/fuzz.dir/crypto_aes256cbc.cpp.o CMakeFiles/fuzz.dir/crypto_chacha20.cpp.o CMakeFiles/fuzz.dir/crypto_chacha20poly1305.cpp.o CMakeFiles/fuzz.dir/crypto_common.cpp.o CMakeFiles/fuzz.dir/crypto_diff_fuzz_chacha20.cpp.o CMakeFiles/fuzz.dir/crypto_hkdf_hmac_sha256_l32.cpp.o CMakeFiles/fuzz.dir/crypto_poly1305.cpp.o CMakeFiles/fuzz.dir/cuckoocache.cpp.o CMakeFiles/fuzz.dir/decode_tx.cpp.o CMakeFiles/fuzz.dir/descriptor_parse.cpp.o CMakeFiles/fuzz.dir/deserialize.cpp.o CMakeFiles/fuzz.dir/eval_script.cpp.o CMakeFiles/fuzz.dir/feefrac.cpp.o CMakeFiles/fuzz.dir/fee_rate.cpp.o CMakeFiles/fuzz.dir/feeratediagram.cpp.o CMakeFiles/fuzz.dir/fees.cpp.o CMakeFiles/fuzz.dir/flatfile.cpp.o CMakeFiles/fuzz.dir/float.cpp.o CMakeFiles/fuzz.dir/golomb_rice.cpp.o CMakeFiles/fuzz.dir/headerssync.cpp.o CMakeFiles/fuzz.dir/hex.cpp.o CMakeFiles/fuzz.dir/http_request.cpp.o CMakeFiles/fuzz.dir/i2p.cpp.o CMakeFiles/fuzz.dir/integer.cpp.o CMakeFiles/fuzz.dir/key.cpp.o CMakeFiles/fuzz.dir/key_io.cpp.o CMakeFiles/fuzz.dir/kitchen_sink.cpp.o CMakeFiles/fuzz.dir/load_external_block_file.cpp.o CMakeFiles/fuzz.dir/locale.cpp.o CMakeFiles/fuzz.dir/merkleblock.cpp.o CMakeFiles/fuzz.dir/message.cpp.o CMakeFiles/fuzz.dir/miniscript.cpp.o CMakeFiles/fuzz.dir/minisketch.cpp.o CMakeFiles/fuzz.dir/mini_miner.cpp.o CMakeFiles/fuzz.dir/muhash.cpp.o CMakeFiles/fuzz.dir/multiplication_overflow.cpp.o CMakeFiles/fuzz.dir/net.cpp.o CMakeFiles/fuzz.dir/net_permissions.cpp.o CMakeFiles/fuzz.dir/netaddress.cpp.o CMakeFiles/fuzz.dir/netbase_dns_lookup.cpp.o CMakeFiles/fuzz.dir/node_eviction.cpp.o CMakeFiles/fuzz.dir/overflow.cpp.o CMakeFiles/fuzz.dir/p2p_handshake.cpp.o CMakeFiles/fuzz.dir/p2p_headers_presync.cpp.o CMakeFiles/fuzz.dir/p2p_transport_serialization.cpp.o CMakeFiles/fuzz.dir/package_eval.cpp.o CMakeFiles/fuzz.dir/parse_hd_keypath.cpp.o CMakeFiles/fuzz.dir/parse_iso8601.cpp.o CMakeFiles/fuzz.dir/parse_numbers.cpp.o CMakeFiles/fuzz.dir/parse_script.cpp.o CMakeFiles/fuzz.dir/parse_univalue.cpp.o CMakeFiles/fuzz.dir/partially_downloaded_block.cpp.o CMakeFiles/fuzz.dir/policy_estimator.cpp.o CMakeFiles/fuzz.dir/policy_estimator_io.cpp.o CMakeFiles/fuzz.dir/poolresource.cpp.o CMakeFiles/fuzz.dir/pow.cpp.o CMakeFiles/fuzz.dir/prevector.cpp.o CMakeFiles/fuzz.dir/primitives_transaction.cpp.o CMakeFiles/fuzz.dir/process_message.cpp.o CMakeFiles/fuzz.dir/process_messages.cpp.o CMakeFiles/fuzz.dir/protocol.cpp.o CMakeFiles/fuzz.dir/psbt.cpp.o CMakeFiles/fuzz.dir/random.cpp.o CMakeFiles/fuzz.dir/rbf.cpp.o CMakeFiles/fuzz.dir/rolling_bloom_filter.cpp.o CMakeFiles/fuzz.dir/rpc.cpp.o CMakeFiles/fuzz.dir/script.cpp.o CMakeFiles/fuzz.dir/script_assets_test_minimizer.cpp.o CMakeFiles/fuzz.dir/script_descriptor_cache.cpp.o CMakeFiles/fuzz.dir/script_flags.cpp.o CMakeFiles/fuzz.dir/script_format.cpp.o CMakeFiles/fuzz.dir/script_interpreter.cpp.o CMakeFiles/fuzz.dir/script_ops.cpp.o CMakeFiles/fuzz.dir/script_parsing.cpp.o CMakeFiles/fuzz.dir/script_sigcache.cpp.o CMakeFiles/fuzz.dir/script_sign.cpp.o CMakeFiles/fuzz.dir/scriptnum_ops.cpp.o CMakeFiles/fuzz.dir/secp256k1_ec_seckey_import_export_der.cpp.o CMakeFiles/fuzz.dir/secp256k1_ecdsa_signature_parse_der_lax.cpp.o CMakeFiles/fuzz.dir/signature_checker.cpp.o CMakeFiles/fuzz.dir/signet.cpp.o CMakeFiles/fuzz.dir/socks5.cpp.o CMakeFiles/fuzz.dir/span.cpp.o CMakeFiles/fuzz.dir/string.cpp.o CMakeFiles/fuzz.dir/strprintf.cpp.o CMakeFiles/fuzz.dir/system.cpp.o CMakeFiles/fuzz.dir/timeoffsets.cpp.o CMakeFiles/fuzz.dir/torcontrol.cpp.o CMakeFiles/fuzz.dir/transaction.cpp.o CMakeFiles/fuzz.dir/txdownloadman.cpp.o CMakeFiles/fuzz.dir/tx_in.cpp.o CMakeFiles/fuzz.dir/tx_out.cpp.o CMakeFiles/fuzz.dir/tx_pool.cpp.o CMakeFiles/fuzz.dir/txorphan.cpp.o CMakeFiles/fuzz.dir/txrequest.cpp.o CMakeFiles/fuzz.dir/utxo_snapshot.cpp.o CMakeFiles/fuzz.dir/utxo_total_supply.cpp.o CMakeFiles/fuzz.dir/validation_load_mempool.cpp.o CMakeFiles/fuzz.dir/vecdeque.cpp.o CMakeFiles/fuzz.dir/versionbits.cpp.o CMakeFiles/fuzz.dir/__/__/wallet/test/fuzz/coincontrol.cpp.o CMakeFiles/fuzz.dir/__/__/wallet/test/fuzz/coinselection.cpp.o CMakeFiles/fuzz.dir/__/__/wallet/test/fuzz/crypter.cpp.o CMakeFiles/fuzz.dir/__/__/wallet/test/fuzz/fees.cpp.o CMakeFiles/fuzz.dir/__/__/wallet/test/fuzz/notifications.cpp.o CMakeFiles/fuzz.dir/__/__/wallet/test/fuzz/scriptpubkeyman.cpp.o CMakeFiles/fuzz.dir/__/__/wallet/test/fuzz/spend.cpp.o CMakeFiles/fuzz.dir/__/__/wallet/test/fuzz/wallet_bdb_parser.cpp.o -o fuzz  util/libtest_fuzz.a ../../libbitcoin_cli.a ../../libbitcoin_common.a ../../util/libbitcoin_util.a ../../../libminisketch.a ../../../libleveldb.a ../../univalue/libunivalue.a ../../secp256k1/lib/libsecp256k1.a ../../wallet/libbitcoin_wallet.a ../util/libtest_util.a ../../libbitcoin_node.a ../../../libminisketch.a ../../../libleveldb.a ../../../libcrc32c.a ../../../libcrc32c_sse42.a /usr/lib/x86_64-linux-gnu/libevent_extra.so /usr/lib/x86_64-linux-gnu/libevent_core.so /usr/lib/x86_64-linux-gnu/libevent_pthreads.so /usr/lib/x86_64-linux-gnu/libevent.so ../../libbitcoin_common.a ../../util/libbitcoin_util.a ../../libbitcoin_consensus.a ../../secp256k1/lib/libsecp256k1.a ../../crypto/libbitcoin_crypto.a ../../crypto/libbitcoin_crypto_sse41.a ../../crypto/libbitcoin_crypto_avx2.a ../../crypto/libbitcoin_crypto_x86_shani.a ../../univalue/libunivalue.a /usr/lib/x86_64-linux-gnu/libsqlite3.so  
 4[10:53:16.586] /usr/bin/ld: util/libtest_fuzz.a(fuzz.cpp.o): in function `main':
 5[10:53:16.600] /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/util/./test/fuzz/fuzz.cpp:237: multiple definition of `main'; /usr/lib/llvm-20/lib/clang/20/lib/linux/libclang_rt.fuzzer-x86_64.a(FuzzerMain.cpp.o):(.text.main+0x0): first defined here
 6[10:53:28.557] clang++-20: error: linker command failed with exit code 1 (use -v to see invocation)
 7[10:53:28.564] gmake[2]: *** [src/test/fuzz/CMakeFiles/fuzz.dir/build.make:2238: src/test/fuzz/fuzz] Error 1
 8[10:53:28.564] gmake[2]: Leaving directory '/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu'
 9[10:53:28.565] gmake[1]: *** [CMakeFiles/Makefile2:998: src/test/fuzz/CMakeFiles/fuzz.dir/all] Error 2
10[10:53:28.565] gmake[1]: Leaving directory '/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu'
11[10:53:28.566] gmake: *** [Makefile:136: all] Error 2

in CMakeLists.txt:396 in a66a1d7a03 outdated

392       #include <cstdint>
393       #include <cstddef>
394       extern \"C\" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { return 0; }
395       // No main() function.
396     " FUZZ_BINARY_LINKS_WITHOUT_MAIN_FUNCTION
397-    LDFLAGS ${SANITIZER_LDFLAGS}

theuni commented at 8:07 pm on February 13, 2025:

This still needs LDFLAGS ${SANITIZER_LDFLAGS} as before in order for the check to pass, so I think the sanitizer_interface changes above should be undone.

hebasto commented at 8:39 pm on February 13, 2025:

Fixed.

hebasto force-pushed on Feb 13, 2025

DrahtBot removed the label CI failed on Feb 13, 2025

theuni approved

theuni commented at 6:25 pm on February 18, 2025: member

utACK 9e7eb06a24b670e8e72e0233a63bbc67733d4adb

A reminder that this will require a change in OSS-Fuzz: s/SANITIZER_LDFLAGS/FUZZ_LIBS/

in cmake/module/CheckSourceCompilesAndLinks.cmake:10 in 1b2ca134b9 outdated

 5@@ -6,9 +6,6 @@ include_guard(GLOBAL)
 6 include(CheckCXXSourceCompiles)
 7 include(CMakePushCheckState)
 8 
 9-# This avoids running the linker.
10-set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
11-
12 macro(check_cxx_source_links source)
13   set(CMAKE_TRY_COMPILE_TARGET_TYPE EXECUTABLE)

TheCharlatan commented at 3:54 pm on February 19, 2025:

In commit 1b2ca134b9e94a26af0bcfc9d9142d0048db6089:

Nit (since this does get resolved eventually): Does removing the global in this commit actually have any effect if we still modify it in this macro? Seems confusing to say in the commit message that this commit removes the overrides when they are still there a few lines down.

theuni commented at 6:25 pm on February 19, 2025:

Probably not here. IIRC hebasto split this out of a larger commit. Agree that it’s confusing, but since it’s resolved later I’m not too worried. I’d say it’s up to hebasto if he wants to fix it for the sake of history or not.

hebasto commented at 12:11 pm on February 20, 2025:

I agree.

I’ve picked that commit from one of @theuni’s branch. Perhaps it fell out of the correct context.

hebasto commented at 12:43 pm on February 20, 2025:

Thanks! Reworked.

in CMakeLists.txt:386 in 9e7eb06a24 outdated

383@@ -383,9 +384,9 @@ if(SANITIZERS)
384     message(FATAL_ERROR "Linker did not accept requested flags, you are missing required libraries.")
385   endif()
386 endif()
387-target_link_options(sanitize_interface INTERFACE ${SANITIZER_LDFLAGS})

TheCharlatan commented at 4:15 pm on February 19, 2025:

In commit 9e7eb06a24b670e8e72e0233a63bbc67733d4adb Nit: Is this change related to the actual change introducing FUZZ_LIBS in this commit?

hebasto commented at 12:42 pm on February 20, 2025:

Unrelated changes have been dropped.

TheCharlatan approved

TheCharlatan commented at 4:18 pm on February 19, 2025: contributor

ACK 9e7eb06a24b670e8e72e0233a63bbc67733d4adb

Can you add the note about FUZZ_LIBS usage to the PR description?

dergoegge commented at 4:26 pm on February 19, 2025: member

s/SANITIZER_LDFLAGS/FUZZ_LIBS/

Happy to take care of this once this gets merged.

hebasto commented at 12:03 pm on February 20, 2025: member

Can you add the note about FUZZ_LIBS usage to the PR description?

Sure! Added.

build: Don't override CMake's default try_compile target

CMake assumes the default value internally, so overriding this causes
problems. The minimal speedup of skipping the linker isn't worth the
complexity of setting it to static.

09e8fd25b1

cmake: Delete `check_cxx_source_links_with_flags` macro 88ee6800c9

cmake: Convert `check_cxx_source_compiles_with_flags` to a function 71bf8294a9

cmake: Delete `check_cxx_source_links*` macros 8d238c1dfd

scripted-diff: Rename CMake helper module

-BEGIN VERIFY SCRIPT-
git mv cmake/module/CheckSourceCompilesAndLinks.cmake cmake/module/CheckSourceCompilesWithFlags.cmake
sed -i 's|\<CheckSourceCompilesAndLinks\>|CheckSourceCompilesWithFlags|g' $(git grep -l 'CheckSourceCompilesAndLinks')
-END VERIFY SCRIPT-

ea929c0848

cmake: Introduce `FUZZ_LIBS`

CMake distinguishes recommended methods for handling (1) linker options
and (2) libraries used during linking. Therefore, it is both reasonable
and consistent to introduce a dedicated variable for the latter,
particularly when a build environment, such as OSS-Fuzz, requires
linking against additional libraries.

2c4b229c90

hebasto force-pushed on Feb 20, 2025

hebasto commented at 12:43 pm on February 20, 2025: member

Addressed the latest @TheCharlatan’s feedback.

TheCharlatan approved

TheCharlatan commented at 2:55 pm on February 20, 2025: contributor

Re-ACK 2c4b229c906de6250500d3af2b44808e90b9ce0b

DrahtBot requested review from theuni on Feb 20, 2025

theuni approved

theuni commented at 4:48 pm on February 20, 2025: member

utACK 2c4b229c906de6250500d3af2b44808e90b9ce0b

I didn’t re-review each commit in great detail, but the diff from before is as I’d expect.

fanquake referenced this in commit d39c45fcb4 on Feb 20, 2025

fanquake merged this on Feb 20, 2025

fanquake closed this on Feb 20, 2025

fanquake commented at 8:07 pm on February 20, 2025: member

Opened https://github.com/google/oss-fuzz/pull/13081.

DavidKorczynski referenced this in commit 269f5e1ffd on Feb 20, 2025

hebasto deleted the branch on Feb 20, 2025

cmake: Do not modify CMAKE_TRY_COMPILE_TARGET_TYPE globally #31662

Code Coverage & Benchmarks

Reviews

Conflicts

cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally #31662