Skip to content

Commit

Permalink
Merge bitcoin#19522: build: fix building libconsensus with reduced ex…
Browse files Browse the repository at this point in the history
…ports for Darwin targets

de4238f build: consolidate reduced export checks (fanquake)
012bdec build: add building libconsensus to end-of-configure output (fanquake)
8f360e3 build: remove ax_gcc_func_attribute macro (fanquake)
f054a08 build: remove AX_GCC_FUNC_ATTRIBUTE test for dllimport (fanquake)
7cd0a69 build: test for __declspec(dllexport) in configure (fanquake)
1624e17 build: remove duplicate visibility attribute detection (fanquake)

Pull request description:

  Darwin targets do not have a `protected` visibility function attribute, see [LLVM explanation](https://github.com/llvm/llvm-project/blob/8e9a505139fbef7d2e6e9d0adfe1efc87326f9ef/clang/lib/Basic/Targets/OSTargets.h#L131). This means that the `AX_GCC_FUNC_ATTRIBUTE` check for `visibility` fails:
  ```bash
  configure:24513: checking for __attribute__((visibility))
  configure:24537: g++ -std=c++11 -o conftest -g -O2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0  -Wl,-headerpad_max_install_names conftest.cpp  >&5
  conftest.cpp:35:56: warning: target does not support 'protected' visibility; using 'default' [-Wunsupported-visibility]
                      int foo_pro( void ) __attribute__((visibility("protected")));
                                                         ^
  1 warning generated.
  configure:24537: $? = 0
  configure:24550: result: no
  ```

  This leads to `EXPORT_SYMBOL` being [defined to nothing](https://github.com/bitcoin/bitcoin/blob/f4de89edfa8be4501534fec0c662c650a4ce7ef2/src/script/bitcoinconsensus.h#L29), as `HAVE_FUNC_ATTRIBUTE_VISIBILITY` is not defined, and when building with reduced exports, you end up with a libbitcoinconsensus.dylib that doesn't export any  `_bitcoinconsensus_*` symbols.
  ```bash
  ➜  git:(master) nm -C src/.libs/libbitcoinconsensus.dylib | rg _bitcoinconsensus_
  ➜  git:(master)
  ```

  We do have a [second check](https://github.com/bitcoin/bitcoin/blob/f4de89edfa8be4501534fec0c662c650a4ce7ef2/configure.ac#L882) for the `visibility` attribute, which works for Darwin as it's only testing for default visibility, however the result of this check isn't used at all. It was added in #4725, along with the `--enable-reduce-exports` option, however when libbitcoinconsensus was added in #5235, it used the results of the added `AX_GCC_FUNC_ATTRIBUTE` calls.

  This PR removes our usage of the AX_GCC_FUNC_ATTRIBUTE macro entirely, in favour of our own checks in configure. This meant adding a check for `dllexport`, which I've tested as working with both [GCC](https://gcc.gnu.org/onlinedocs/gcc/Microsoft-Windows-Function-Attributes.html) and [Clang](https://releases.llvm.org/10.0.0/tools/clang/docs/AttributeReference.html#dllexport) when building for Windows. I haven't added an equivalent check for `dllimport`, as we weren't actually using the result of that check, we're just testing that `MSC_VER` was defined before using.

  With these changes building a libbitcoinconsensus with reduced exports, when targeting Darwin, works as expected:
  ```bash
  ./autogen.sh
  ./configure --disable-tests --disable-bench --with-utils=no --with-daemon=no --with-gui=no --disable-wallet --with-libs=yes --enable-reduce-exports
  make -j8
  ...
  nm -C src/.libs/libbitcoinconsensus.dylib | rg _bitcoinconsensus_
  000000000000a340 T _bitcoinconsensus_verify_script
  00000000000097e0 T _bitcoinconsensus_verify_script_with_amount
  000000000000a3c0 T _bitcoinconsensus_version
  ```

  ```python
  >>> import ctypes
  >>> consensus = ctypes.CDLL("src/.libs/libbitcoinconsensus.dylib")
  >>> print(consensus.bitcoinconsensus_version())
  1
  >>> exit()
  ```

  TODO: Modify a CI job to compile with --enable-reduce-exports and check for symbols in shared lib?

ACKs for top commit:
  laanwj:
    Code review ACK de4238f

Tree-SHA512: d148f3c55d14dac6e9e5b718cc65bb557bcf6f663218d24bc9044b86281bd5dd3d931ebea79c336a58e8ed50d683218c0a9e75494f2267b91097665043e252ae
  • Loading branch information
laanwj authored and knst committed Apr 10, 2024
1 parent 88a45d4 commit 318c726
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 246 deletions.
223 changes: 0 additions & 223 deletions build-aux/m4/ax_gcc_func_attribute.m4

This file was deleted.

37 changes: 21 additions & 16 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -913,10 +913,6 @@ if test x$ac_cv_sys_large_files != x &&
CPPFLAGS="$CPPFLAGS -D_LARGE_FILES=$ac_cv_sys_large_files"
fi

AX_GCC_FUNC_ATTRIBUTE([visibility])
AX_GCC_FUNC_ATTRIBUTE([dllexport])
AX_GCC_FUNC_ATTRIBUTE([dllimport])

AC_SEARCH_LIBS([clock_gettime],[rt])

if test "x$enable_gprof" = xyes; then
Expand Down Expand Up @@ -1078,13 +1074,13 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
[ AC_MSG_RESULT(no)]
)

AC_MSG_CHECKING([for visibility attribute])
AC_LINK_IFELSE([AC_LANG_SOURCE([
int foo_def( void ) __attribute__((visibility("default")));
AC_MSG_CHECKING([for default visibility attribute])
AC_COMPILE_IFELSE([AC_LANG_SOURCE([
int foo(void) __attribute__((visibility("default")));
int main(){}
])],
[
AC_DEFINE(HAVE_VISIBILITY_ATTRIBUTE,1,[Define if the visibility attribute is supported.])
AC_DEFINE(HAVE_DEFAULT_VISIBILITY_ATTRIBUTE,1,[Define if the visibility attribute is supported.])
AC_MSG_RESULT(yes)
],
[
Expand All @@ -1095,6 +1091,18 @@ AC_LINK_IFELSE([AC_LANG_SOURCE([
]
)

AC_MSG_CHECKING([for dllexport attribute])
AC_COMPILE_IFELSE([AC_LANG_SOURCE([
__declspec(dllexport) int foo(void);
int main(){}
])],
[
AC_DEFINE(HAVE_DLLEXPORT_ATTRIBUTE,1,[Define if the dllexport attribute is supported.])
AC_MSG_RESULT(yes)
],
[AC_MSG_RESULT(no)]
)

if test "x$use_thread_local" = xyes || test "x$use_thread_local" = xauto; then
TEMP_LDFLAGS="$LDFLAGS"
LDFLAGS="$TEMP_LDFLAGS $PTHREAD_CFLAGS"
Expand Down Expand Up @@ -1269,12 +1277,6 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
[ AC_MSG_RESULT(no); HAVE_STRONG_GETAUXVAL=0 ]
)

dnl Check for reduced exports
if test x$use_reduce_exports = xyes; then
AX_CHECK_COMPILE_FLAG([-fvisibility=hidden],[RE_CXXFLAGS="-fvisibility=hidden"],
[AC_MSG_ERROR([Cannot set default symbol visibility. Use --disable-reduce-exports.])])
fi

AC_MSG_CHECKING([for std::system])
AC_LINK_IFELSE(
[ AC_LANG_PROGRAM(
Expand Down Expand Up @@ -1486,9 +1488,11 @@ fi
BOOST_LIBS="$BOOST_LDFLAGS $BOOST_SYSTEM_LIB $BOOST_FILESYSTEM_LIB"
fi

dnl Check for reduced exports
if test x$use_reduce_exports = xyes; then
CXXFLAGS="$CXXFLAGS $RE_CXXFLAGS"
AX_CHECK_LINK_FLAG([[-Wl,--exclude-libs,ALL]], [RELDFLAGS="-Wl,--exclude-libs,ALL"],, [[$LDFLAG_WERROR]])
AX_CHECK_COMPILE_FLAG([-fvisibility=hidden],[CXXFLAGS="$CXXFLAGS -fvisibility=hidden"],
[AC_MSG_ERROR([Cannot set hidden symbol visibility. Use --disable-reduce-exports.])],[[$CXXFLAG_WERROR]])
AX_CHECK_LINK_FLAG([[-Wl,--exclude-libs,ALL]],[RELDFLAGS="-Wl,--exclude-libs,ALL"],,[[$LDFLAG_WERROR]])
fi

if test x$use_tests = xyes; then
Expand Down Expand Up @@ -1951,6 +1955,7 @@ echo
echo "Options used to compile and link:"
echo " boost process = $with_boost_process"
echo " multiprocess = $build_multiprocess"
echo " with libs = $build_bitcoin_libs"
echo " with wallet = $enable_wallet"
echo " with gui / qt = $bitcoin_enable_qt"
if test "x$enable_wallet" != "xno"; then
Expand Down
12 changes: 5 additions & 7 deletions src/script/bitcoinconsensus.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,12 @@
#if defined(BUILD_BITCOIN_INTERNAL) && defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#if defined(_WIN32)
#if defined(DLL_EXPORT)
#if defined(HAVE_FUNC_ATTRIBUTE_DLLEXPORT)
#define EXPORT_SYMBOL __declspec(dllexport)
#else
#define EXPORT_SYMBOL
#endif
#if defined(HAVE_DLLEXPORT_ATTRIBUTE)
#define EXPORT_SYMBOL __declspec(dllexport)
#else
#define EXPORT_SYMBOL
#endif
#elif defined(HAVE_FUNC_ATTRIBUTE_VISIBILITY)
#elif defined(HAVE_DEFAULT_VISIBILITY_ATTRIBUTE)
#define EXPORT_SYMBOL __attribute__ ((visibility ("default")))
#endif
#elif defined(MSC_VER) && !defined(STATIC_LIBBITCOINCONSENSUS)
Expand Down

0 comments on commit 318c726

Please sign in to comment.