build: Fix undefined reference to _

hebasto commented at 9:19 pm on May 7, 2021: member

When compiling with clang on 32-bit systems the __mulodi4 symbol is defined in compiler-rt only.

Fixes #21294.

See more:

hebasto commented at 9:20 pm on May 7, 2021: member

@rebroad @MarcoFalke

Does this fix work in your cases?

hebasto force-pushed on May 7, 2021

DrahtBot added the label Build system on May 7, 2021

MarcoFalke commented at 9:22 am on May 8, 2021: member

Concept and tested-only ACK 207b4e2c2bdbb882035cb0f595b55f9415557d6f didn’t review

MarcoFalke commented at 9:24 am on May 8, 2021: member

This allows to remove the workaround in oss-fuzz:

 0oss-fuzz# git diff
 1diff --git a/projects/bitcoin-core/build.sh b/projects/bitcoin-core/build.sh
 2index 66d827f8..b2c4f36e 100755
 3--- a/projects/bitcoin-core/build.sh
 4+++ b/projects/bitcoin-core/build.sh
 5@@ -19,13 +19,6 @@
 6 # This will also force static builds
 7 if [ "$ARCHITECTURE" = "i386" ]; then
 8   export BUILD_TRIPLET="i686-pc-linux-gnu"
 9-  # Temporary workaround for:
10-  #   CXXLD    test/fuzz/fuzz
11-  # test/fuzz/test_fuzz_fuzz-multiplication_overflow.o: In function `void (anonymous namespace)::TestMultiplicationOverflow<long long>(FuzzedDataProvider&)':
12-  # /src/bitcoin-core/src/test/fuzz/multiplication_overflow.cpp:30: undefined reference to `__mulodi4'
13-  # clang-12: error: linker command failed with exit code 1 (use -v to see invocation)
14-  # Makefile:5495: recipe for target 'test/fuzz/fuzz' failed
15-  sed -i 's|defined(HAVE_BUILTIN_MUL_OVERFLOW)|defined(IGNORE_BUILTIN_MUL_OVERFLOW)|g' "./src/test/fuzz/multiplication_overflow.cpp"
16 else
17   export BUILD_TRIPLET="x86_64-pc-linux-gnu"
18 fi

rebroad commented at 10:23 am on May 9, 2021: contributor

@rebroad @MarcoFalke

Does this fix work in your cases?

I’ll have a look and let you know

laanwj commented at 11:24 am on May 10, 2021: member

Does this issue only affect building the fuzz binary? This wasn’t clear for me from the OP in #21294.

hebasto commented at 8:34 pm on May 10, 2021: member

@laanwj

Does this issue only affect building the fuzz binary? This wasn’t clear for me from the OP in #21294.

Yes, the OP in #21294 points to some other places, but the exact version of the code is still unknown (or I missed it).

On the current master the bug is reproducible for fuzz binary only (using Armbian 21.02.3 Focal).

in configure.ac:1814 in 207b4e2c2b outdated

1809+    [AC_LANG_SOURCE([bitcoin_mul_overflow_test])],
1810+    [AC_MSG_RESULT([yes])],
1811+    [
1812+      AC_MSG_RESULT([no])
1813+      AX_CHECK_LINK_FLAG([--rtlib=compiler-rt -lgcc_s],
1814+                         [FUZZ_BINARY_LDFLAGS="--rtlib=compiler-rt -lgcc_s"],

fanquake commented at 5:21 am on May 12, 2021:

Is linking against GCCs runtime library actually needed?

hebasto commented at 3:18 pm on May 14, 2021:

Yes, it is.

After adding --rtlib=compiler-rt the new linking error appears:

 0  CXXLD    bitcoind
 1  CXXLD    bitcoin-cli
 2  CXXLD    bitcoin-util
 3  CXXLD    test/fuzz/fuzz
 4/usr/bin/ld: test/fuzz/fuzz-addition_overflow.o: undefined reference to symbol '__aeabi_unwind_cpp_pr0@@GCC_3.5'
 5/usr/bin/ld: /lib/arm-linux-gnueabihf/libgcc_s.so.1: error adding symbols: DSO missing from command line
 6clang: error: linker command failed with exit code 1 (use -v to see invocation)
 7make[2]: *** [Makefile:6141: test/fuzz/fuzz] Error 1
 8make[2]: *** Waiting for unfinished jobs....
 9make[2]: Leaving directory '/home/hebasto/bitcoin/src'
10make[1]: *** [Makefile:15996: all-recursive] Error 1
11make[1]: Leaving directory '/home/hebasto/bitcoin/src'
12make: *** [Makefile:821: all-recursive] Error 1

On master with the following diff

0--- a/src/Makefile.test.include
1+++ b/src/Makefile.test.include
2@@ -251,7 +251,6 @@ test_fuzz_fuzz_SOURCES = \
3  test/fuzz/merkleblock.cpp \
4  test/fuzz/message.cpp \
5  test/fuzz/muhash.cpp \
6- test/fuzz/multiplication_overflow.cpp \
7  test/fuzz/net.cpp \
8  test/fuzz/net_permissions.cpp \
9  test/fuzz/netaddress.cpp \

no error fires.

https://lists.linaro.org/pipermail/linaro-toolchain/2011-April/001101.html:

__aeabi_unwind_cpp_pr0 is part of the standard ARM exception handling code…

laanwj commented at 12:20 pm on May 12, 2021: member

On the current master the bug is reproducible for fuzz binary only (using Armbian 21.02.3 Focal).

Makes sense, thanks for letting me know, maybe we can wait a bit for tested confirmation that this solves the referenced issues.

MarcoFalke commented at 12:49 pm on May 12, 2021: member

I did test this in #21882 (comment), but I did not review

MarcoFalke commented at 5:20 pm on May 13, 2021: member

The steps to reproduce from #21920#issue-641280284 can be used here as well

fanquake commented at 4:04 am on May 14, 2021: member

This should probably be broken out into it’s own macro, with an explanation of why it’s needed.

Is this actually limited to 32-bit? When compiling with Clang 11, on a 64-bit system, it is easy to recreate the same problem. i.e:

0int f(__int128 a, __int128 b, __int128 *p) {
1  return __builtin_mul_overflow(a, b, p);
2}
3int main() {}

0# clang --version
1Ubuntu clang version 11.0.0-2~ubuntu20.04.1
2Target: x86_64-pc-linux-gnu
3
4# clang test.c
5/usr/bin/ld: /tmp/test-de745b.o: in function 'f':
6test.c:(.text+0x61): undefined reference to '__muloti4'
7
8# clang test.c --rtlib=compiler-rt
9<success>

Why isn’t this broken for 64-bit builds?

hebasto commented at 1:30 pm on May 14, 2021: member

@fanquake

Is this actually limited to 32-bit? When compiling with Clang 11, on a 64-bit system, it is easy to recreate the same problem. i.e:
0int f(__int128 a, __int128 b, __int128 *p) {
1  return __builtin_mul_overflow(a, b, p);
2}
3int main() {}

Do we use __builtin_mul_overflow with 128-bit arguments?

Why isn’t this broken for 64-bit builds?

https://bugs.webkit.org/show_bug.cgi?id=190208#c4

for 64 bit arguments, Clang cannot produce the code so calls __mulodi4 on 32-bit architectures

hebasto marked this as a draft on May 14, 2021

hebasto marked this as ready for review on May 14, 2021

hebasto force-pushed on May 14, 2021

hebasto commented at 5:09 pm on May 14, 2021: member

Updated 207b4e2c2bdbb882035cb0f595b55f9415557d6f -> 70d89ff9f6a883d5a8cc6049adf4dc173bced73a (pr21882.02 -> pr21882.03):

addressed @fanquake’s comment

This should probably be broken out into it’s own macro, with an explanation of why it’s needed.

rebased

practicalswift commented at 7:53 am on May 15, 2021: contributor

Concept ACK

laanwj commented at 12:02 pm on May 17, 2021: member

On some level this problem is really strange. You’re absolutely not supposed to have to link anything special to be able to use this basic C functionality. Functions with double underscore __ are internal compiler details, unless you’re deep into embedded development territory there should be no reason for application builders to be concerned with them.

If this is a workaround for a bug in a specific compiler version, please make this more clear and add a comment so that we know when it can be removed again.

hebasto commented at 12:06 pm on May 17, 2021: member

On some level this problem is really strange. You’re absolutely not supposed to have to link anything special to be able to use this basic C functionality. Functions with double underscore __ are internal compiler details, unless you’re deep into embedded development territory there should be no reason for application builders to be concerned with them.

We are already use double-underscore-named functions: https://github.com/bitcoin/bitcoin/blob/7b87fca930ff7129f267906e71be217851146ade/src/test/fuzz/multiplication_overflow.cpp#L30

hebasto commented at 12:11 pm on May 17, 2021: member

@laanwj

If this is a workaround for a bug in a specific compiler version, please make this more clear and add a comment so that we know when it can be removed again.

I’m not sure about a specific clang version. From https://bugs.llvm.org/show_bug.cgi?id=28629 I can assume that all supported clang versions are affected.

laanwj commented at 1:53 pm on May 17, 2021: member

We are already use double-underscore-named functions:

Even there it’s a compiler builtin / intrinsic. It’s supposed to be part of the compiler or its support code, not a library you have to explicitly link. And I see no calls to __mulodi4 specifically. To be clear it’s not meant as a NACK or anything, but I sometimes worry about the number of one-shot compiler bug workarounds we’re accumulating.

MarcoFalke commented at 3:00 pm on May 18, 2021: member

Could rebase to simplify testing?

hebasto force-pushed on May 18, 2021

hebasto commented at 3:38 pm on May 18, 2021: member

Could rebase to simplify testing?

Done.

MarcoFalke commented at 8:32 am on May 20, 2021: member

tested-only ACK bd55f62549ef16b2bbe8273ee8d98c05520aa7b4

On vanilla Ubuntu Focal:

0export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git && cd bitcoin && apt install build-essential libtool autotools-dev automake pkg-config bsdmainutils python3-zmq make automake cmake curl clang llvm g++-multilib libtool binutils-gold bsdmainutils pkg-config python3 patch bison -y  && ( cd depends && make DEBUG=1 HOST=i686-pc-linux-gnu NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_UPNP=1 NO_NATPMP=1 -j $(nproc) ) && ./autogen.sh && CONFIG_SITE="$PWD/depends/i686-pc-linux-gnu/share/config.site" ./configure CC='clang -m32' CXX='clang++ -m32' --enable-fuzz --with-sanitizers=fuzzer && make  -j $(nproc)

Before:

 0  AR       libbitcoin_server.a
 1  AR       libbitcoin_common.a
 2  AR       libbitcoin_util.a
 3  AR       libtest_util.a
 4  AR       crypto/libbitcoin_crypto_base.a
 5  AR       libbitcoin_consensus.a
 6  AR       crc32c/libcrc32c.a
 7  AR       leveldb/libmemenv.a
 8  AR       leveldb/libleveldb.a
 9  CXXLD    test/fuzz/fuzz
10/usr/bin/ld: test/fuzz/fuzz-multiplication_overflow.o: in function `void (anonymous namespace)::TestMultiplicationOverflow<long long>(FuzzedDataProvider&)':
11multiplication_overflow.cpp:(.text._ZN12_GLOBAL__N_126TestMultiplicationOverflowIxEEvR18FuzzedDataProvider[_ZN12_GLOBAL__N_126TestMultiplicationOverflowIxEEvR18FuzzedDataProvider$f74c2a0666fdfec672dc24de475b04f3]+0x8d): undefined reference to `__mulodi4'
12clang: error: linker command failed with exit code 1 (use -v to see invocation)
13make[2]: *** [Makefile:6140: test/fuzz/fuzz] Error 1
14make[2]: Leaving directory '/bitcoin/src'
15make[1]: *** [Makefile:15995: all-recursive] Error 1
16make[1]: Leaving directory '/bitcoin/src'
17make: *** [Makefile:820: all-recursive] Error 1

After:

Clean

in build-aux/m4/bitcoin_runtime_lib.m4:24 in bd55f62549 outdated

19+
20+AC_DEFUN([CHECK_RUNTIME_LIB], [
21+
22+  AC_LANG_PUSH(C++)
23+
24+  AC_MSG_CHECKING([whether __builtin_mul_overflow can be used without compiler-rt])

luke-jr commented at 10:59 pm on June 11, 2021:

AIUI, this test succeeds with non-clang, but __builtin_mul_overflow doesn’t actually get checked at all. Maybe the test itself should be conditional on using clang?

luke-jr commented at 7:03 pm on June 13, 2021: member

Perhaps something like this would be better?

 0diff --git a/build-aux/m4/bitcoin_runtime_lib.m4 b/build-aux/m4/bitcoin_runtime_lib.m4
 1index 6af5514e6b7..9834b6c7965 100644
 2--- a/build-aux/m4/bitcoin_runtime_lib.m4
 3+++ b/build-aux/m4/bitcoin_runtime_lib.m4
 4@@ -5,14 +5,10 @@
 5 # - https://bugs.llvm.org/show_bug.cgi?id=28629
 6 
 7 m4_define([_CHECK_RUNTIME_testbody], [[
 8-  #if defined(__clang__) && defined(__has_builtin)
 9-  #if __has_builtin(__builtin_mul_overflow)
10   bool f(long long x, long long y, long long* p)
11   {
12     return __builtin_mul_overflow(x, y, p);
13   }
14-  #endif
15-  #endif
16 
17   int main() { return 0; }
18 ]])
19@@ -21,18 +17,23 @@ AC_DEFUN([CHECK_RUNTIME_LIB], [
20 
21   AC_LANG_PUSH(C++)
22 
23-  AC_MSG_CHECKING([whether __builtin_mul_overflow can be used without compiler-rt])
24+  AC_MSG_CHECKING([for __builtin_mul_overflow])
25 
26   AC_LINK_IFELSE([AC_LANG_SOURCE([_CHECK_RUNTIME_testbody])], [
27-      AC_MSG_RESULT([yes])
28-    ], [
29+    AC_MSG_RESULT([yes])
30+    AC_DEFINE([HAVE_BUILTIN_MUL_OVERFLOW], [1], [Define if you have a working __builtin_mul_overflow])
31+  ], [
32+    ax_check_save_flags="$LDFLAGS"
33+    LDFLAGS="$LDFLAGS --rtlib=compiler-rt -lgcc_s"
34+    AC_LINK_IFELSE([AC_LANG_SOURCE([_CHECK_RUNTIME_testbody])], [
35+      AC_MSG_RESULT([with additional linker flags])
36+      RUNTIME_LDFLAGS="--rtlib=compiler-rt -lgcc_s"
37+      AC_DEFINE([HAVE_BUILTIN_MUL_OVERFLOW], [1], [Define if you have a working __builtin_mul_overflow])
38+    ],[
39       AC_MSG_RESULT([no])
40-      AX_CHECK_LINK_FLAG([--rtlib=compiler-rt -lgcc_s],
41-                         [RUNTIME_LDFLAGS="--rtlib=compiler-rt -lgcc_s"],
42-                         [AC_MSG_FAILURE([cannot figure out how to use __builtin_mul_overflow])],
43-                         [$LDFLAG_WERROR],
44-                         [AC_LANG_SOURCE([_CHECK_RUNTIME_testbody])])
45     ])
46+    LDFLAGS=$ax_check_save_flags
47+  ])
48 
49   AC_LANG_POP
50   AC_SUBST(RUNTIME_LDFLAGS)
51diff --git a/src/test/fuzz/multiplication_overflow.cpp b/src/test/fuzz/multiplication_overflow.cpp
52index a4b158c18b5..9f617668569 100644
53--- a/src/test/fuzz/multiplication_overflow.cpp
54+++ b/src/test/fuzz/multiplication_overflow.cpp
55@@ -2,6 +2,10 @@
56 // Distributed under the MIT software license, see the accompanying
57 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
58 
59+#if defined(HAVE_CONFIG_H)
60+#include <config/bitcoin-config.h>
61+#endif
62+
63 #include <test/fuzz/FuzzedDataProvider.h>
64 #include <test/fuzz/fuzz.h>
65 #include <test/fuzz/util.h>
66@@ -10,14 +14,6 @@
67 #include <string>
68 #include <vector>
69 
70-#if defined(__has_builtin)
71-#if __has_builtin(__builtin_mul_overflow)
72-#define HAVE_BUILTIN_MUL_OVERFLOW
73-#endif
74-#elif defined(__GNUC__) && (__GNUC__ >= 5)
75-#define HAVE_BUILTIN_MUL_OVERFLOW
76-#endif
77-
78 namespace {
79 template <typename T>
80 void TestMultiplicationOverflow(FuzzedDataProvider& fuzzed_data_provider)

That way failure to make __builtin_mul_overflow work gracefully takes the “don’t have __builtin_mul_overflow” path, and there’s less compiler special-case checks involved.

luke-jr referenced this in commit 07954a4924 on Jun 27, 2021

hebasto force-pushed on Jul 27, 2021

build: Fix undefined reference to __mulodi4

When compiling with clang on 32-bit systems the __mulodi4 symbol is
defined in compiler-rt only.

e4c8bb62e4

hebasto force-pushed on Jul 27, 2021

hebasto commented at 11:55 am on July 27, 2021: member

Updated bd55f62549ef16b2bbe8273ee8d98c05520aa7b4 -> e4c8bb62e4a6873c45f42d0d2a24927cb241a0ea (pr21882.04 -> pr21882.05, diff):

addressed @luke-jr’s comment

luke-jr approved

luke-jr commented at 10:56 pm on July 28, 2021: member

utACK e4c8bb62e4a6873c45f42d0d2a24927cb241a0ea

MarcoFalke commented at 7:09 am on July 29, 2021: member

tested-only ACK e4c8bb62e4a6873c45f42d0d2a24927cb241a0ea

Could suffix title with “… in fuzz binary”?

MarcoFalke requested review from fanquake on Jul 29, 2021

fanquake approved

fanquake commented at 12:52 pm on July 29, 2021: member

ACK e4c8bb62e4a6873c45f42d0d2a24927cb241a0ea - it’s a bit of an awkward workaround to carry, but at-least it’s contained to the fuzzers.

fanquake merged this on Jul 29, 2021

fanquake closed this on Jul 29, 2021

hebasto deleted the branch on Jul 29, 2021

sidhujag referenced this in commit bb887e3e4d on Jul 29, 2021

fanquake referenced this in commit c16e030376 on Jul 30, 2021

fanquake referenced this in commit 01df3ac613 on Aug 4, 2021

inferno-chromium referenced this in commit ec3c914f22 on Aug 4, 2021

luke-jr referenced this in commit 9656810bba on Dec 14, 2021

rebroad commented at 7:54 am on December 19, 2021: contributor

This is still an issue with clang, as of ac92ab6da58e34993d0641b98eef5b5f55b6cbf9:-

0  CXXLD    qt/bitcoin-qt
1qt/libbitcoinqt.a(qt_libbitcoinqt_a-rpcconsole.o): In function `bool std::__detail::__raise_and_add<unsigned long>(unsigned long&, int, unsigned char)':
2/usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/charconv:410: undefined reference to `__muloti4'
3libbitcoin_server.a(libbitcoin_server_a-mining.o): In function `_ZSt10from_charsIlENSt9enable_ifIXsr22__is_int_to_chars_typeIT_EE5valueESt17from_chars_resultE4typeEPKcS6_RS1_i':
4/usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/charconv:632: undefined reference to `__muloti4'
5clang: error: linker command failed with exit code 1 (use -v to see invocation)
6Makefile:5767: recipe for target 'qt/bitcoin-qt' failed
7make: *** [qt/bitcoin-qt] Error 1

hebasto commented at 9:14 am on December 19, 2021: member

@rebroad

This is still an issue with clang, as of ac92ab6:-

0  CXXLD    qt/bitcoin-qt
1qt/libbitcoinqt.a(qt_libbitcoinqt_a-rpcconsole.o): In function `bool std::__detail::__raise_and_add<unsigned long>(unsigned long&, int, unsigned char)':
2/usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/charconv:410: undefined reference to `__muloti4'
3libbitcoin_server.a(libbitcoin_server_a-mining.o): In function `_ZSt10from_charsIlENSt9enable_ifIXsr22__is_int_to_chars_typeIT_EE5valueESt17from_chars_resultE4typeEPKcS6_RS1_i':
4/usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/charconv:632: undefined reference to `__muloti4'
5clang: error: linker command failed with exit code 1 (use -v to see invocation)
6Makefile:5767: recipe for target 'qt/bitcoin-qt' failed
7make: *** [qt/bitcoin-qt] Error 1

Are you talking about 0.21? If so it’s because this PR has not been backported to 0.21.

rebroad commented at 10:55 pm on December 19, 2021: contributor

@hebasto no, the master branch as of 9 days ago.

kittywhiskers referenced this in commit 980859a4cd on Oct 16, 2022

kittywhiskers referenced this in commit 3ec6c10154 on Oct 16, 2022

kittywhiskers referenced this in commit 618d4ff931 on Oct 16, 2022

kittywhiskers referenced this in commit b7e9773b53 on Oct 16, 2022

kittywhiskers referenced this in commit 7ef86a7094 on Oct 16, 2022

kittywhiskers referenced this in commit 53da946cb2 on Oct 20, 2022

PastaPastaPasta referenced this in commit 06b95ac908 on Oct 20, 2022

PastaPastaPasta referenced this in commit 6851dbf908 on Oct 20, 2022

PastaPastaPasta referenced this in commit 5e2eacbbce on Oct 20, 2022

DrahtBot locked this on Dec 19, 2022

build: Fix undefined reference to __mulodi4 #21882