-
Notifications
You must be signed in to change notification settings - Fork 36.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build: fix building libconsensus with reduced exports for Darwin targets #19522
build: fix building libconsensus with reduced exports for Darwin targets #19522
Conversation
A PR is never truly open until a moment of realization right after hitting the big green button. This is actually something I broke in #7711. Updating the The alternative approach here would be to re-remove the two visibility attributes we don't care about from the Big anti-props me. |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Gitian builds
|
ee6b341
to
49c4d56
Compare
Consensus from the latest build meeting was that removing the macro would be ok, so I'm going to leave this with the current approach. I've also modified the last commit to re-split the |
Concept ACK, changes look good at a glance. Thanks for fixing this. The attribute check has been broken for years. Any reason for not just appending to CXXFLAGS/LDFLAGS for the configure output? I'm not sure that RE_FOO would anything to anyone other than reviewers of this pull request :) |
49c4d56
to
2ed0325
Compare
Yea fair call. I've dropped the RE specifics and have added to the CXX and LD flags output. |
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
2ed0325
to
8959aec
Compare
src/script/bitcoinconsensus.h
Outdated
@@ -12,13 +12,13 @@ | |||
#include <config/bitcoin-config.h> | |||
#if defined(_WIN32) | |||
#if defined(DLL_EXPORT) | |||
#if defined(HAVE_FUNC_ATTRIBUTE_DLLEXPORT) | |||
#if defined(HAVE_DLLEXPORT_ATTRIBUTE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also always assume __declspec(dllexport)
to be present on windows? It seems something is very wrong with the compiler if it is not.
I mean: two checks make sense here: test for HAVE_DLLEXPORT_ATTRIBUTE
or test for _WIN32
. Both is overkill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also always assume __declspec(dllexport) to be present on windows? It seems something is very wrong with the compiler if it is not.
Agree. Also, this is our internal build, so we're generally aware of what we support, and we wouldn't support such a broken compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, the dllexport can be moved to a Windows-only check in configure, which errors if it fails.
Edit: Windows-only, and only if building the lib.
We are already testing for this, and our test works correctly with a Darwin target, where the macro does not. Darwin targets do not support "protected" visibility.
This should work for GCC and Clang when building for Windows targets.
The result of this test isn't currently used anywhere (we use dllimport based on MSC_VER in libconsensus).
This is no-longer used.
8959aec
to
1df549f
Compare
1df549f
to
de4238f
Compare
Code review ACK de4238f |
…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 bitcoin#4725, along with the `--enable-reduce-exports` option, however when libbitcoinconsensus was added in bitcoin#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
Darwin targets do not have a
protected
visibility function attribute, see LLVM explanation. This means that theAX_GCC_FUNC_ATTRIBUTE
check forvisibility
fails:This leads to
EXPORT_SYMBOL
being defined to nothing, asHAVE_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.➜ git:(master) nm -C src/.libs/libbitcoinconsensus.dylib | rg _bitcoinconsensus_ ➜ git:(master)
We do have a second check 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 addedAX_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 and Clang when building for Windows. I haven't added an equivalent check fordllimport
, as we weren't actually using the result of that check, we're just testing thatMSC_VER
was defined before using.With these changes building a libbitcoinconsensus with reduced exports, when targeting Darwin, works as expected:
./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
TODO: Modify a CI job to compile with --enable-reduce-exports and check for symbols in shared lib?