Develop trivial 2022 03 13 #4725
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ation test fae814c fuzz: Remove incorrect float round-trip serialization test (MarcoFalke) Pull request description: It tests the wrong way of the round-trip: `int -> float -> int`, but only `float -> int -> float` is allowed and used. See also `src/test/fuzz/float.cpp`. Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34118 ACKs for top commit: laanwj: Anyhow, ACK fae814c Tree-SHA512: 8412a7985be2225109f382b7c7ea6d6fcfbea15711671fdf2f41dd1a9adbb3b4489592863751d78bedaff98e9b0b13571d9cae06ffd92db8fbf7ce0f47874a41
…S SDK f8f772d macdeploy: alternative info to download the macOS SDK (Antoine Poinsot) Pull request description: The previous link wasn't accessible for me, this adds some instructions given to me by Hebasto on #bitcoin-core-builds as well as a shasum for the archive to quickly check the downloaded one is the right one before processing with the entire Guix build. ACKs for top commit: fanquake: ACK f8f772d Tree-SHA512: 620160b593ed8fa4ae4a748b8e72d67b93ff0ec9e6b8ef3c3ac5402c1c48ec0ac325a527b6278cdf84aaf51ba8194d4c366c412ffad141d0412add2710efcff5
…rces 5d37cc4 Generate doxygen documentation for test sources (Patrick Kamin) Pull request description: Fixes bitcoin#19248 While searching for the documentation of the test utilities I realized they were excluded from doxygen. I agree with the statement in bitcoin#19248. It's also helpful for new contributors to gain a broader understanding of the class dependencies visually (see BasicTestSetup) ACKs for top commit: laanwj: ACK 5d37cc4 Tree-SHA512: 32f0abab2970c65621af5cee7f620c2653bd9688366e125543262bd078841e64a5a1e24cf0241e9f6ec538b8759e06108d5ff056449aa1c98d5f287deef18c86
acb7aad Fix build with Boost 1.77.0 (Rafael Sadowski) Pull request description: BOOST_FILESYSTEM_C_STR changed to accept the path as an argument. ACKs for top commit: hebasto: ACK acb7aad benthecarman: ACK acb7aad fanquake: ACK acb7aad - tested the fix with Boost 1.77.0 and 1.71.0. Tree-SHA512: c25fcb56971ee7a448cfb074f8a13696b32c16c63f81076f8a76911f93aa849c8f3637555b0b4215fa0d8b958641d7e4e60d10e103b833545cbc6b1f4009b526
…th.cpp 69a439b doc: add missing copyright header to getuniquepath.cpp (fanquake) Pull request description: This was missed in bitcoin#21052. ACKs for top commit: hebasto: ACK 69a439b, but a scripted-diff using `copyright_header.py insert` looks preferable. Tree-SHA512: 1a75ffd93fa8e5e83821e1fe984353422740ebf9e203dc873debf4dffec0e23f55a1e31f806dd2d66087d3620f6dc447af69f9124f0bcc6676ef91ee35374832
fdd7144 system: skip trying to set the locale on NetBSD (fanquake) Pull request description: Just treat it the same as the other BSDs. Fixes bitcoin#17379. ACKs for top commit: laanwj: Code review ACK fdd7144 practicalswift: cr ACK fdd7144 Tree-SHA512: 5fe0a66f014279ad2683b548692a36af493377fb92d1f28b15dc4feef871190fe08ef40dcc4f5ba21a525fe365c42fb429fe4be0673a1e96db163af587c23204
…ndows c427a58 doc: Set PYTHONUTF8=1 for functional tests on Windows (Hennadii Stepanov) Pull request description: The `PYTHONUTF8` environment variable is defined in [PEP 540](https://www.python.org/dev/peps/pep-0540/), and it is actually used in our CI: https://github.com/bitcoin/bitcoin/blob/5e3380b9f59481fc18e05b9d651c3c733abe4053/.cirrus.yml#L89 This PR documents such usage to avoid users' [errors](bitcoin#22922 (comment)). ACKs for top commit: MarcoFalke: cr ACK c427a58 Tree-SHA512: 441b8cecfe47d548cfe403b0e1cd0aef25c1a70ff556434ead1f1e26372919931ac6f208a4ed6fd8dcca46e8709245e4fb06f95259a43c8e1221473ce1ee497b
5008dd8 doc: Remove stale comment for CPrivKey (Calvin Kim) Pull request description: Removes stale doc about `secure_allocator` being defined in `allocators.h`. ACKs for top commit: laanwj: ACK 5008dd8 theStack: Code-review ACK 5008dd8 Tree-SHA512: eb65aff6db5b27d0db2b86f1d1dc6e066daccdaf00f7f9f95b5bee507167fcea2601316cdbd70da4ba32f1fab1e28e440a7e3cabd7b1a72c07dd20b1367361f0
…apLocalHost` 330d3aa refactor: net: avoid duplicate map lookups to `mapLocalHost` (Sebastian Falbesoner) Pull request description: This simple refactoring PR aims to avoid duplicate lookups to `mapLocalHost`: instead of calling `count()` (to first find out whether a key is in the map) and then `operator[]` (to get the value to the passed key, or default-construct one if not found), use either * `find()` and dereference the returned iterator (for simple lookups), see https://www.cplusplus.com/reference/map/map/find/ * `emplace()` and use the returned <iterator, inserted> pair (for lookups where a new element should be inserted if the key isn't found), see https://www.cplusplus.com/reference/map/map/emplace/ ACKs for top commit: naumenkogs: ACK 330d3aa jonatack: Code review ACK 330d3aa plus rebase to master + debug build Tree-SHA512: d13da6a927ff561eee8ac6b093bf3586dfe31d6c94173a5a6d8f3698e0ee224fb394d3635155d5141c165da59d2c2c37260122eb4f2e8bcda3e8a29b901d213e
…rch64 f2747d1 build: Restrict check for CRC32C intrinsic to aarch64 (W. J. van der Laan) Pull request description: `crc32c`'s hardware accelerated code doesn't handle ARM 32-bit at all, but only 64-bit. Make the check in `configure.ac` check for this architecture explicitly. This change does not affect non-ARM architectures. For the release binaries, the current `configure.ac` check happens to work: it enables it on aarch64 but disables it for armhf. However some combination of compiler version and settings can cause this check to succeed on armhf (as reported on IRC). So make the 64-bit platform requirement explicit. (details: while the check already explicitly checks the `__crc32d` intrinsic, which strictly doesn't exist on 32-bit ARM, this is not enough! gcc happens to helpfully emulate it: > These built-in intrinsics for the ARMv8-A CRC32 extension are available when the -march=armv8-a+crc switch is used "uint32_t __crc32d (uint32_t, uint64_t)" Form of expected instruction(s): Two crc32w r0, r0, r0 instructions for AArch32 ) ACKs for top commit: luke-jr: re-tACK f2747d1 jarolrod: ACK f2747d1 Tree-SHA512: e5f05f05eeec310ac42685621d86862735586be66dc378db404ec9414ac5aaea7c53d76d76d875b15b11924eee6714076120c07b077183fd7af898704fd81823
… flags b8909b0 Run functional tests with all possible flags (Samuel Dobson) Pull request description: Functional tests which use flags like `--descriptors` or `--legacy-wallet` won't run if only the base script is given to `test_runner.py` because it doesn't match any script in the list exactly. It would be easier if it would just run both options. For example, instead of: ``` test_runner.py 'wallet_basic.py --legacy-wallet' 'wallet_basic.py --descriptors' ``` We can now just run: ``` test_runner.py wallet_basic ``` Also useful for `--usecli`, the IPv4/IPv6/nonloopback `rpc_bind.py` variations, etc. ACKs for top commit: laanwj: Code review ACK b8909b0 MarcoFalke: review ACK b8909b0 Tree-SHA512: d367037cb170e705551726d47fe4569ebc3ceadece280dd3edbb3ecb41e19f3263d6d272b407316ed6011164e850df4321fb340b1b183b34497c9f7cc439f4d8
1d44513 Squashed 'src/crc32c/' changes from b5ef9be675..0d624261ef (MarcoFalke) Pull request description: Only change is a warning fix for arm. ``` CXX crc32c/src/crc32c_libcrc32c_a-crc32c.o In file included from crc32c/src/crc32c.cc:11:0: crc32c/src/./crc32c_arm64_check.h: In function ‘bool crc32c::CanUseArm64Crc32()’: crc32c/src/./crc32c_arm64_check.h:43:37: warning: the address of ‘long unsigned int getauxval(long unsigned int)’ will never be NULL [-Waddress] unsigned long hwcap = (&getauxval != nullptr) ? getauxval(AT_HWCAP) : 0; ~~~~~~~~~~~^~~~~~~~~~ ACKs for top commit: laanwj: Code review ACK fac1c13 fanquake: ACK fac1c13 Tree-SHA512: 22a52caf67dd89092eff1f075fbf5c5d16bdca9146ba042ce5d3fcc10ce1485e950964089f8536c938ebe650676e03a789d3597fe45b19920fd2c5e72f1391ad
…32 nChild (de)serialization 7fc487a refactor: use `{Read,Write}BE32` helpers for BIP32 nChild (de)serialization (Sebastian Falbesoner) Pull request description: This small refactoring PR replaces manual bit-fiddling (de)serialization of the BIP32 child number (nChild) by the helpers `ReadBE32`/`WriteBE32`. Note that those were first introduced in dashpay#4100, almost one year _after_ the BIP32 derivation implementation has been merged (dashpay#2829, eb2c999). ACKs for top commit: sipa: utACK 7fc487a laanwj: Code review ACK 7fc487a Tree-SHA512: bbe3e411fb0429fa74c8a5705a91f4d6ed704dac9d6623ecb633563f22acf8e21f3189a16f1d0cf1aeedfc56a5b695df54ae51e9577e34eb6d7dc335de2da6de
knst
pushed a commit
to knst/dash
that referenced
this pull request
Apr 4, 2024
…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 dashpay#4725, along with the `--enable-reduce-exports` option, however when libbitcoinconsensus was added in dashpay#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
knst
pushed a commit
to knst/dash
that referenced
this pull request
Apr 6, 2024
…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 dashpay#4725, along with the `--enable-reduce-exports` option, however when libbitcoinconsensus was added in dashpay#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
knst
pushed a commit
to knst/dash
that referenced
this pull request
Apr 8, 2024
…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 dashpay#4725, along with the `--enable-reduce-exports` option, however when libbitcoinconsensus was added in dashpay#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
knst
pushed a commit
to knst/dash
that referenced
this pull request
Apr 9, 2024
…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 dashpay#4725, along with the `--enable-reduce-exports` option, however when libbitcoinconsensus was added in dashpay#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
knst
pushed a commit
to knst/dash
that referenced
this pull request
Apr 10, 2024
…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 dashpay#4725, along with the `--enable-reduce-exports` option, however when libbitcoinconsensus was added in dashpay#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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.