build: Fix undefined reference to __mulodi4 #21882

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

Fixes #21294.

See more:

• https://bugs.llvm.org/show_bug.cgi?id=16404
• https://bugs.llvm.org/show_bug.cgi?id=28629

@rebroad @MarcoFalke

Does this fix work in your cases?

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

This allows to remove the workaround in oss-fuzz:

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

@rebroad @MarcoFalke

Does this fix work in your cases?

I'll have a look and let you know

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

@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).

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.

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

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

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:

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

# clang --version
Ubuntu clang version 11.0.0-2~ubuntu20.04.1
Target: x86_64-pc-linux-gnu

# clang test.c
/usr/bin/ld: /tmp/test-de745b.o: in function 'f':
test.c:(.text+0x61): undefined reference to '__muloti4'

# clang test.c --rtlib=compiler-rt
<success>

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

@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:

int f(__int128 a, __int128 b, __int128 *p) {
  return __builtin_mul_overflow(a, b, p);
}
int 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

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

Concept ACK

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.

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

@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.

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.

Could rebase to simplify testing?

Could rebase to simplify testing?

Done.

tested-only ACK bd55f62549ef16b2bbe8273ee8d98c05520aa7b4

On vanilla Ubuntu Focal:

export 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:

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

After:

Clean

Perhaps something like this would be better?

diff --git a/build-aux/m4/bitcoin_runtime_lib.m4 b/build-aux/m4/bitcoin_runtime_lib.m4
index 6af5514e6b7..9834b6c7965 100644
--- a/build-aux/m4/bitcoin_runtime_lib.m4
+++ b/build-aux/m4/bitcoin_runtime_lib.m4
@@ -5,14 +5,10 @@
 # - https://bugs.llvm.org/show_bug.cgi?id=28629
 
 m4_define([_CHECK_RUNTIME_testbody], [[
-  #if defined(__clang__) && defined(__has_builtin)
-  #if __has_builtin(__builtin_mul_overflow)
   bool f(long long x, long long y, long long* p)
   {
     return __builtin_mul_overflow(x, y, p);
   }
-  #endif
-  #endif
 
   int main() { return 0; }
 ]])
@@ -21,18 +17,23 @@ AC_DEFUN([CHECK_RUNTIME_LIB], [
 
   AC_LANG_PUSH(C++)
 
-  AC_MSG_CHECKING([whether __builtin_mul_overflow can be used without compiler-rt])
+  AC_MSG_CHECKING([for __builtin_mul_overflow])
 
   AC_LINK_IFELSE([AC_LANG_SOURCE([_CHECK_RUNTIME_testbody])], [
-      AC_MSG_RESULT([yes])
-    ], [
+    AC_MSG_RESULT([yes])
+    AC_DEFINE([HAVE_BUILTIN_MUL_OVERFLOW], [1], [Define if you have a working __builtin_mul_overflow])
+  ], [
+    ax_check_save_flags="$LDFLAGS"
+    LDFLAGS="$LDFLAGS --rtlib=compiler-rt -lgcc_s"
+    AC_LINK_IFELSE([AC_LANG_SOURCE([_CHECK_RUNTIME_testbody])], [
+      AC_MSG_RESULT([with additional linker flags])
+      RUNTIME_LDFLAGS="--rtlib=compiler-rt -lgcc_s"
+      AC_DEFINE([HAVE_BUILTIN_MUL_OVERFLOW], [1], [Define if you have a working __builtin_mul_overflow])
+    ],[
       AC_MSG_RESULT([no])
-      AX_CHECK_LINK_FLAG([--rtlib=compiler-rt -lgcc_s],
-                         [RUNTIME_LDFLAGS="--rtlib=compiler-rt -lgcc_s"],
-                         [AC_MSG_FAILURE([cannot figure out how to use __builtin_mul_overflow])],
-                         [$LDFLAG_WERROR],
-                         [AC_LANG_SOURCE([_CHECK_RUNTIME_testbody])])
     ])
+    LDFLAGS=$ax_check_save_flags
+  ])
 
   AC_LANG_POP
   AC_SUBST(RUNTIME_LDFLAGS)
diff --git a/src/test/fuzz/multiplication_overflow.cpp b/src/test/fuzz/multiplication_overflow.cpp
index a4b158c18b5..9f617668569 100644
--- a/src/test/fuzz/multiplication_overflow.cpp
+++ b/src/test/fuzz/multiplication_overflow.cpp
@@ -2,6 +2,10 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#if defined(HAVE_CONFIG_H)
+#include <config/bitcoin-config.h>
+#endif
+
 #include <test/fuzz/FuzzedDataProvider.h>
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
@@ -10,14 +14,6 @@
 #include <string>
 #include <vector>
 
-#if defined(__has_builtin)
-#if __has_builtin(__builtin_mul_overflow)
-#define HAVE_BUILTIN_MUL_OVERFLOW
-#endif
-#elif defined(__GNUC__) && (__GNUC__ >= 5)
-#define HAVE_BUILTIN_MUL_OVERFLOW
-#endif
-
 namespace {
 template <typename T>
 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.

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

• addressed @luke-jr's comment

utACK e4c8bb62e4a6873c45f42d0d2a24927cb241a0ea

tested-only ACK e4c8bb62e4a6873c45f42d0d2a24927cb241a0ea

Could suffix title with "... in fuzz binary"?

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

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

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

@rebroad

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

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

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