build: Do not define `PROVIDE_FUZZ_MAIN_FUNCTION` macro unconditionally #24337

hebasto commented at 12:01 PM on February 14, 2022: member

No need to define the PROVIDE_FUZZ_MAIN_FUNCTION macro when the build system has been configured with the --disable-fuzz-binary option.

See #24336#pullrequestreview-881368272.

DrahtBot added the label Build system on Feb 14, 2022

hebasto commented at 5:44 PM on February 17, 2022: member

@MarcoFalke

Do you think that the fuzz task timeout is caused by this PR change?

MarcoFalke commented at 5:47 PM on February 17, 2022: member

No

fanquake commented at 11:54 AM on February 18, 2022: member

This should be combined with the CHECK_RUNTIME_LIB check rather than another enable_fuzz_binary test.

MarcoFalke commented at 12:00 PM on February 18, 2022: member

A third refacotring alternative might be to remove it. Then, call AX_CHECK_LINK_FLAG unconditionally?

hebasto force-pushed on Feb 19, 2022

hebasto commented at 8:51 PM on February 19, 2022: member

Updated 4a1e159978792bf3edbc1c51e4b9ccdd4c164a7d -> 7e7ae428319f4813087cde1a5f4611ea20bebe97 (pr24337.01 -> pr24337.02).

This should be combined with the CHECK_RUNTIME_LIB check rather than another enable_fuzz_binary test.

A third refacotring alternative might be to remove it. Then, call AX_CHECK_LINK_FLAG unconditionally?

Chosen the former suggestion as it assumes minimal diff.

hebasto force-pushed on Feb 20, 2022

hebasto commented at 9:26 AM on February 20, 2022: member

Updated 7e7ae428319f4813087cde1a5f4611ea20bebe97 -> 8d0683da024e62999d574371e965bc8fc51a6f38 (pr24337.02 -> pr24337.03): reverted back and rebased.

This should be combined with the CHECK_RUNTIME_LIB check rather than another enable_fuzz_binary test.

Doesn't work. Fails with "multiple definition of `main'" error.

A third refacotring alternative might be to remove it. Then, call AX_CHECK_LINK_FLAG unconditionally?

Doesn't work. The -DPROVIDE_FUZZ_MAIN_FUNCTION macro still defined even configured with --disable-fuzz-binary.

MarcoFalke commented at 12:21 PM on February 20, 2022: member

I mean:

if enable_fuzz_binary:
  AX_CHECK_LINK_FLAG
  CHECK_RUNTIME_LIB
fi

hebasto force-pushed on Feb 20, 2022

hebasto renamed this:
~~build: Define `PROVIDE_FUZZ_MAIN_FUNCTION` macro only when needed~~
build: Do not define `PROVIDE_FUZZ_MAIN_FUNCTION` macro unconditionally
on Feb 20, 2022

hebasto commented at 1:15 PM on February 20, 2022: member

I mean:

if enable_fuzz_binary:
  AX_CHECK_LINK_FLAG
  CHECK_RUNTIME_LIB
fi

Thanks! Done: 8d0683da024e62999d574371e965bc8fc51a6f38 -> 121c4ba4475285dd335655e410aedcb95076ba85 (pr24337.03 -> pr24337.04).

hebasto marked this as a draft on Feb 20, 2022

hebasto marked this as ready for review on Feb 20, 2022

hebasto force-pushed on Feb 20, 2022

hebasto commented at 3:20 PM on February 20, 2022: member

I mean:
if enable_fuzz_binary:
  AX_CHECK_LINK_FLAG
  CHECK_RUNTIME_LIB
fi
Thanks! Done: 8d0683d -> 121c4ba (pr24337.03 -> pr24337.04).

Reverted back due to the CI failures.

MarcoFalke commented at 4:20 PM on February 20, 2022: member

failures

That's because it incorrectly uses a previously cached, but unrelated result. You can work around that with:

diff --git a/configure.ac b/configure.ac
index b7852ae3b5..efdeb186c3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1300,7 +1300,7 @@ fi
 if test "$enable_fuzz_binary" = "yes"; then
   AC_MSG_CHECKING([whether main function is needed for fuzz binary])
   AX_CHECK_LINK_FLAG(
-    [-fsanitize=$use_sanitizers],
+    [-fsanitize= -fsanitize=$use_sanitizers],
     [AC_MSG_RESULT([no])],
     [AC_MSG_RESULT([yes]); CPPFLAGS="$CPPFLAGS -DPROVIDE_FUZZ_MAIN_FUNCTION"],
     [],

MarcoFalke commented at 4:27 PM on February 20, 2022: member

See https://cirrus-ci.com/task/6160465456791552?logs=ci#L2772

checking whether main function is needed for fuzz binary... checking whether the linker accepts -fsanitize=address,integer,undefined... (cached) yes no

hebasto commented at 5:45 PM on February 20, 2022: member

fsanitize=address,integer,undefined... (cached) yes

Yeap, AX_CHECK_LINK_FLAG creates and uses a cached variable ax_cv_check_ldflags_$4_$1.

hebasto force-pushed on Feb 20, 2022

hebasto commented at 7:38 PM on February 20, 2022: member

Updated 8d0683da024e62999d574371e965bc8fc51a6f38 -> c0a089b242150979aefe5238a240ca5de30d4602 (pr24337.03 -> pr24337.05):

addressed @MarcoFalke's comment

DrahtBot commented at 12:14 AM on February 21, 2022: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23969 (build: remove use of TARGET_OS and BUILD_OS 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.

DrahtBot added the label Needs rebase on Mar 7, 2022

hebasto force-pushed on Apr 5, 2022

hebasto commented at 5:51 PM on April 5, 2022: member

Rebased c0a089b242150979aefe5238a240ca5de30d4602 -> 3ec7dc7c470a0c117ad601feda5fa7d45e9ecd93 (pr24337.05 -> pr24337.06).

in configure.ac:1321 in 3ec7dc7c47 outdated

1329 | @@ -1315,21 +1330,8 @@ if test "$enable_fuzz" = "yes"; then
1330 |       ]],[[
1331 |        */ int not_main() {
1332 |       ]])])
1333 | -else
1334 | -  BITCOIN_QT_INIT
1335 |  
1336 | -  dnl sets $bitcoin_enable_qt, $bitcoin_enable_qt_test, $bitcoin_enable_qt_dbus

MarcoFalke commented at 6:49 PM on April 5, 2022:

moving this seems unrelated?

hebasto commented at 7:05 PM on April 5, 2022:

It looks like a way the diff has been rendered. The actual diff is moving AX_CHECK_LINK_FLAG stuff.

in configure.ac:1321 in 3ec7dc7c47 outdated

1317 | +
1318 | +if test "$enable_fuzz_binary" = "yes"; then
1319 |    AC_MSG_CHECKING([whether main function is needed for fuzz binary])
1320 |    AX_CHECK_LINK_FLAG(
1321 | -    [-fsanitize=$use_sanitizers],
1322 | +    [-fsanitize= -fsanitize=$use_sanitizers], dnl Avoid re-use of an unrelated cached result.

fanquake commented at 7:10 PM on April 5, 2022:

This looks like a hack, and the comment doesn't really explain why it's needed. If I understand what it's meant to be doing, couldn't you check that the AC_LANG_PROGRAM compiles while passing SANITIZER_LDFLAGS as extra flags?

hebasto commented at 7:15 PM on April 5, 2022:

This looks like a hack

It is a hack...

hebasto commented at 6:47 AM on April 6, 2022:

Updated.

DrahtBot removed the label Needs rebase on Apr 5, 2022

hebasto force-pushed on Apr 6, 2022

build: Do not define `PROVIDE_FUZZ_MAIN_FUNCTION` macro unconditionally c9c4e6cadd

hebasto force-pushed on Apr 6, 2022

hebasto commented at 7:18 AM on April 6, 2022: member

Updated 3ec7dc7c470a0c117ad601feda5fa7d45e9ecd93 -> c9c4e6cadda85783a005af773a683d44a208bf67 (pr24337.06 -> pr24337.07, diff):

addressed @fanquake's comment

MarcoFalke added the label DrahtBot Guix build requested on Apr 6, 2022

DrahtBot commented at 10:43 AM on April 10, 2022: member

Guix builds

File	commit 38d3d0bfc4cae6b31c5ac30f9b0da458bd9a9e57<br>(master)	commit 7b2fdd92819d4861f27fedfc8db5e6ed70ec346b<br>(master and this pull)
SHA256SUMS.part	`5b338da8888ed74e...`	`2e10517f4f3fcf8e...`
*-aarch64-linux-gnu-debug.tar.gz	`169181d915ec8a1b...`	`a943369a6fbd0320...`
*-aarch64-linux-gnu.tar.gz	`7ceb686ca03481e2...`	`4110ac2ed8cb2b1b...`
*-arm-linux-gnueabihf-debug.tar.gz	`322bc373e08936dc...`	`6fa23007a6127157...`
*-arm-linux-gnueabihf.tar.gz	`1312ea4bb32a2e95...`	`f8d9f25ffc7afe69...`
*-arm64-apple-darwin-unsigned.dmg	`1f14f9b49309b65a...`	`38418789670b459b...`
*-arm64-apple-darwin-unsigned.tar.gz	`09abc3c6b900ccd2...`	`52fe888221dc5019...`
*-arm64-apple-darwin.tar.gz	`6bbdb6f1b9470a8e...`	`382a64f73a31b6f2...`
*-powerpc64-linux-gnu-debug.tar.gz	`8eb5fdd4a09e3848...`	`da2ce5ce8b752d8c...`
*-powerpc64-linux-gnu.tar.gz	`22926c107bc7180f...`	`92e8c7d1f73a00c1...`
*-powerpc64le-linux-gnu-debug.tar.gz	`2ce49961659115f8...`	`2521db9010aabd42...`
*-powerpc64le-linux-gnu.tar.gz	`a4e7ad11fe61f2d3...`	`3166a101fe257919...`
*-riscv64-linux-gnu-debug.tar.gz	`22f977cf97993240...`	`b7ed43b8cd7a4cde...`
*-riscv64-linux-gnu.tar.gz	`42f28fe61dee331f...`	`268a612a528635b6...`
*-win64-debug.zip	`cc3ddc0b1b213e13...`	`089f46fb944a4c74...`
*-win64-setup-unsigned.exe	`9434f90b613e9ff6...`	`101bc0ab4983a989...`
*-win64-unsigned.tar.gz	`0d3362db53cd0e0c...`	`fd184459eb9e63aa...`
*-win64.zip	`81e85958fe5ef86b...`	`5c3a579ec9ef27ff...`
*-x86_64-apple-darwin-unsigned.dmg	`74b78a90e14e8026...`	`ab58b53d274fbc45...`
*-x86_64-apple-darwin-unsigned.tar.gz	`6be471a6518fc661...`	`3b4f5d89aeed9184...`
*-x86_64-apple-darwin.tar.gz	`13253355fb41e82a...`	`dbffa5a6738b4ea4...`
*-x86_64-linux-gnu-debug.tar.gz	`9df8f207b22ad2d6...`	`e9c30b48a0573a78...`
*-x86_64-linux-gnu.tar.gz	`79347964dbbc0281...`	`9d539b8652bd3e66...`
*.tar.gz	`1ff1ea0b99723d31...`	`29ddf1f0a991e217...`
guix_build.log	`3c7b9e6a0ca97995...`	`deee81a07ee46bf4...`
guix_build.log.diff		`ee7f2c1b8a55de45...`

DrahtBot removed the label DrahtBot Guix build requested on Apr 10, 2022

MarcoFalke requested review from fanquake on Apr 11, 2022

MarcoFalke commented at 9:38 AM on April 11, 2022: member

Approach ACK c9c4e6cadda85783a005af773a683d44a208bf67 did not review or test 🐤

<details><summary>Show signature</summary>

Signature:

</details>

fanquake approved

fanquake commented at 1:28 PM on April 11, 2022: member

ACK c9c4e6cadda85783a005af773a683d44a208bf67 Checked that PROVIDE_FUZZ_MAIN_FUNCTION isn't defined when configuring with --disable-fuzz-binary.

fanquake merged this on Apr 11, 2022

fanquake closed this on Apr 11, 2022

hebasto deleted the branch on Apr 11, 2022

sidhujag referenced this in commit 31ac0a7a1a on Apr 11, 2022

DrahtBot locked this on Apr 11, 2023