Skip to content
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

Merged
merged 6 commits into from
Feb 12, 2021

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Jul 15, 2020

Darwin targets do not have a protected visibility function attribute, see LLVM explanation. This means that the AX_GCC_FUNC_ATTRIBUTE check for visibility fails:

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

➜  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 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 and Clang 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:

./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
>>> 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?

@fanquake
Copy link
Member Author

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 AX_GCC_FUNC_ATTRIBUTE macro reintroduced the checks for protected and internal, which had been removed in ee64c53. However I take it for the past 4 years, no-one has built and used a libbitcoinconsensus for a Darwin target, with --enable-reduced-exports.

The alternative approach here would be to re-remove the two visibility attributes we don't care about from the AX_GCC_FUNC_ATTRIBUTE macro (rather than 7a60033, 1381e9c and 01bc0b6), noting that we've modified it from upstream, and then remove the unused visibility check from #4725. In that case, the remaining changes would still be relevant improvements.

Big anti-props me.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 15, 2020

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, 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.

@DrahtBot
Copy link
Contributor

Guix builds

File commit f4de89e
(master)
commit 65ee660
(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz 08cfaea788a07ceb... 363d65bf69cb4ed3...
*-aarch64-linux-gnu.tar.gz f22c76019e0ccd7f... 9f4e6b10e84a4742...
*-arm-linux-gnueabihf-debug.tar.gz 60a5d1855b55f0a8... fd5e5127f9a70f72...
*-arm-linux-gnueabihf.tar.gz ebd8c0e33838e1fa... 4ad3bbead17991e9...
*-riscv64-linux-gnu-debug.tar.gz 5bdbfcff97558737... 5fcd68e92c9ab05a...
*-riscv64-linux-gnu.tar.gz a826ce80c8c40316... 93ad8954a8eb6852...
*-win-unsigned.tar.gz f5adfa1b298f3ef6... 6f8d30b0d88ff7ba...
*-win64-debug.zip 6c8a495cf2d815cf... 053acb2b03e6c22c...
*-win64-setup-unsigned.exe 3c1135852587c8d0... 84b2797d2fadbd41...
*-win64.zip 1e6c4a7013557b19... 160133580ec04756...
*-x86_64-linux-gnu-debug.tar.gz b2f27029c311cc3b... 89a0fd504956290f...
*-x86_64-linux-gnu.tar.gz e182a4c29c6961d0... c6d49aa87cc67fcb...
*.tar.gz f142bfa8f69d3205... 10f65d5c9df2a708...
guix_build.log 49d4490a44c4a052... 936b426e8678d14c...
guix_build.log.diff ca1fd062c0261430...

@DrahtBot
Copy link
Contributor

Gitian builds

File commit 3864219
(master)
commit d401e95
(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz 234341edaaa03287... 1f56f09dfb88aaf3...
*-aarch64-linux-gnu.tar.gz fe6c16d320115507... a6a1427684d33416...
*-arm-linux-gnueabihf-debug.tar.gz 42a6b4a487d81b8f... 76c65185b837c05f...
*-arm-linux-gnueabihf.tar.gz a47bf9a033950563... e4d223079b9f7e4c...
*-osx-unsigned.dmg 6b39d6bb9fcba41e... 254fea74e349e72f...
*-osx64.tar.gz 9de75640b54a62ad... f2a081a0f3f262f2...
*-riscv64-linux-gnu-debug.tar.gz f2b02a01562d8c1e... 46146d5758fa1531...
*-riscv64-linux-gnu.tar.gz 0270bb629d738fc0... ffb7f90a465edf7e...
*-win64-debug.zip 38645188383170e2... 0db1f1e01357801b...
*-win64-setup-unsigned.exe 273a380bf2f19bf4... bb3ad193464dd190...
*-win64.zip bf93c8f5d2457a57... 36891f6dc2ea9fb5...
*-x86_64-linux-gnu-debug.tar.gz 2c2a010fb9114dd5... 6d543a9d3ecca3ee...
*-x86_64-linux-gnu.tar.gz 8b953c29cccb10d7... 655695eea728b52b...
*.tar.gz 17eb6dc576244bd1... d07b584351d2cde9...
bitcoin-core-linux-0.21-res.yml 8a85867b0f5145d2... 64c59fba43184855...
bitcoin-core-osx-0.21-res.yml e6a4c03cdb1fa972... 1a37785727ed9201...
bitcoin-core-win-0.21-res.yml 5e5f75dcf64ee726... 446e2e48863cefa4...
linux-build.log 8747037a8d02910b... 1bdfecc908d058c3...
osx-build.log 03a1f2575139de06... e64a3f107c6f9f35...
win-build.log 54377010d59fa536... 68136fbbfe3df63c...
bitcoin-core-linux-0.21-res.yml.diff d0bb7fce873c59ef...
bitcoin-core-osx-0.21-res.yml.diff cc1f8f928b74e353...
bitcoin-core-win-0.21-res.yml.diff 73ee2a3c66b5b403...
linux-build.log.diff 795ad3d3b77eeffe...
osx-build.log.diff 0483109ffe3f6558...
win-build.log.diff 247cd0f8744e81c8...

@fanquake fanquake force-pushed the libconsensus_visibility_clang branch from ee6b341 to 49c4d56 Compare July 21, 2020 05:05
@fanquake
Copy link
Member Author

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 RE_CXXFLAGS, and conditionally output them and the RELDFLAGS at the end of configure.

@theuni
Copy link
Member

theuni commented Sep 4, 2020

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

@fanquake fanquake force-pushed the libconsensus_visibility_clang branch from 49c4d56 to 2ed0325 Compare September 11, 2020 06:33
@fanquake
Copy link
Member Author

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

Yea fair call. I've dropped the RE specifics and have added to the CXX and LD flags output.

@DrahtBot
Copy link
Contributor

🐙 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".

@fanquake fanquake force-pushed the libconsensus_visibility_clang branch from 2ed0325 to 8959aec Compare September 15, 2020 13:11
@@ -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)
Copy link
Member

@laanwj laanwj Oct 7, 2020

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.

Copy link
Member

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.

Copy link
Member

@theuni theuni Oct 8, 2020

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).
@fanquake fanquake force-pushed the libconsensus_visibility_clang branch from 8959aec to 1df549f Compare February 12, 2021 07:58
@fanquake fanquake force-pushed the libconsensus_visibility_clang branch from 1df549f to de4238f Compare February 12, 2021 08:02
@laanwj
Copy link
Member

laanwj commented Feb 12, 2021

Code review ACK de4238f
nice diff +26 −246

@laanwj laanwj merged commit 54b66a6 into bitcoin:master Feb 12, 2021
@fanquake fanquake deleted the libconsensus_visibility_clang branch February 12, 2021 12:28
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 12, 2021
…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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants