Fix various libbitcoinkernel DLL build problems

theuni commented at 9:26 pm on February 22, 2023: member

Fixes #25008. Fixes #19772.

Fixup the build defines so that exports are clean.
Work around a libtool issue wrt dependency calculation
Simplify everything by only ever building in-tree bitcoin-chainstate against a static libbitcoinkernel
Remove Windows-only hack that disabled dll creation

build: don't define DLL_EXPORT for windows

This fixes libbitcoinkernel dll linking.

9c253d2398

build: fix bitcoin-chainstate when libbitcoinkernel is static

Libtool is unable to calculate dependencies correctly so give it some help.

545a74ef32

build: always build bitcoin-chainstate against static libbitcoinkernel

Building binaries against our uninstalled shared libs is impractical. Instead,
to test them, we'll need to work on a runtime shared-lib execution harness.

130490aef9

build: allow libitcoinkernel dll builds now that exports are fixed

Symbol visibility issues are not actually fixed yet because we have not yet
defined an api and exported symbols, but everything is now in place for that.

5da7c0b3e3

DrahtBot commented at 9:26 pm on February 22, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan
Concept ACK	fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

in src/Makefile.am:858 in 5da7c0b3e3

854@@ -855,11 +855,12 @@ bitcoin_chainstate_SOURCES = bitcoin-chainstate.cpp
855 bitcoin_chainstate_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BOOST_CPPFLAGS)
856 bitcoin_chainstate_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
857 
858-# $(LIBTOOL_APP_LDFLAGS) deliberately omitted here so that we can test linking
859-# bitcoin-chainstate against libbitcoinkernel as a shared or static library by
860-# setting --{en,dis}able-shared.
861-bitcoin_chainstate_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(PTHREAD_FLAGS)
862+bitcoin_chainstate_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(PTHREAD_FLAGS) $(LIBTOOL_APP_LDFLAGS) -static

TheCharlatan commented at 9:16 am on February 23, 2023:

Can you comment on what this -static flag is here for?

theuni commented at 4:16 pm on February 23, 2023:

From libtool help:

0-static           do not do any dynamic linking of uninstalled libtool libraries

So, by default, use a static libbitcoinkernel for bitcoin-chainstate. This matches the default of our other executables linked to libs.

Reason being: otherwise test harnesses, c-i, benchmarks, etc have to play LD_PRELOAD or wrapper tricks.

fanquake commented at 9:40 am on February 23, 2023: member

Concept ACK

fanquake requested review from TheCharlatan on Feb 23, 2023

in src/Makefile.am:862 in 5da7c0b3e3

861-bitcoin_chainstate_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(PTHREAD_FLAGS)
862+bitcoin_chainstate_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(PTHREAD_FLAGS) $(LIBTOOL_APP_LDFLAGS) -static
863 bitcoin_chainstate_LDADD = $(LIBBITCOINKERNEL)
864+
865+# libtool is unable to calculate this indirect dependency, presumably because it's a subproject.
866+# libsecp256k1 only needs to be linked in when libbitcoinkernel is static.

TheCharlatan commented at 9:46 am on February 23, 2023:

Is there a way to check this? I tried running:

0make distclean
1./autogen.sh 
2CONFIG_SITE=~/bitcoin/depends/x86_64-w64-mingw32/share/config.site ./configure \
3--with-experimental-kernel-lib \
4--enable-experimental-util-chainstate \
5--with-experimental \
6--prefix ~/bitcoin/install_dir \
7--disable-static 
8make install

But I’m getting a bunch of linker errors:

0/usr/bin/x86_64-w64-mingw32-ld: /usr/lib/gcc/x86_64-w64-mingw32/10-posix/libssp.a(ssp.o):(.text+0xe0): multiple definition of `__stack_chk_fail'; ./.libs/libbitcoinkernel.dll.a(libbitcoinkernel_0_dll_d003613.o):(.text+0x0): first defined here
1/usr/bin/x86_64-w64-mingw32-ld: /usr/lib/gcc/x86_64-w64-mingw32/10-posix/libgcc_eh.a(unwind-seh.o):(.text+0x3d0): multiple definition of `_Unwind_Resume'; ./.libs/libbitcoinkernel.dll.a(libbitcoinkernel_0_dll_d000032.o):(.text+0x0): first defined here
2/usr/bin/x86_64-w64-mingw32-ld: /usr/lib/gcc/x86_64-w64-mingw32/10-posix/../../../../x86_64-w64-mingw32/lib/libpthread.a(libwinpthread_la-mutex.o): in function `pthread_mutex_lock':
3./build/x86_64-w64-mingw32-x86_64-w64-mingw32-librarieswinpthreads/./mingw-w64-libraries/winpthreads/src/mutex.c:187: multiple definition of `pthread_mutex_lock'; ./.libs/libbitcoinkernel.dll.a(libbitcoinkernel_0_dll_d003768.o):(.text+0x0): first defined here
4/usr/bin/x86_64-w64-mingw32-ld: /usr/lib/gcc/x86_64-w64-mingw32/10-posix/../../../../x86_64-w64-mingw32/lib/libpthread.a(libwinpthread_la-mutex.o): in function `pthread_mutex_unlock':
5./build/x86_64-w64-mingw32-x86_64-w64-mingw32-librarieswinpthreads/./mingw-w64-libraries/winpthreads/src/mutex.c:207: multiple definition of `pthread_mutex_unlock'; ./.libs/libbitcoinkernel.dll.a(libbitcoinkernel_0_dll_d003771.o):(.text+0x0): first defined here
6/usr/bin/x86_64-w64-mingw32-ld: /usr/lib/gcc/x86_64-w64-mingw32/10-posix/../../../../x86_64-w64-mingw32/lib/libpthread.a(libwinpthread_la-mutex.o): in function `pthread_mutex_trylock':
7./build/x86_64-w64-mingw32-x86_64-w64-mingw32-librarieswinpthreads/./mingw-w64-libraries/winpthreads/src/mutex.c:233: multiple definition of `pthread_mutex_trylock'; ./.libs/libbitcoinkernel.dll.a(libbitcoinkernel_0_dll_d003770.o):(.text+0x0): first defined here

theuni commented at 3:30 pm on February 23, 2023:

Our symbol visibility is not under control yet, meaning that the linker doesn’t know what to do when it finds (for ex) an exported pthread_mutex_unlock in libbitcoinkernel.dll as well as libpthread. If you cherry-pick https://github.com/theuni/bitcoin/commit/1733d0199d68b2de2a72dfaa70802756540214a0 on top of this PR, linking should succeed.

This is why I’ve opted to allow for the dll to be built/linked here, but it’s off by default (meaning that you have to do something like --disable-static to hit this behavior). I think that’s ok, but I’d be fine with keeping it forced to static if it introduces confusion.

TheCharlatan commented at 7:57 pm on February 23, 2023:

Nice, I applied theuni@1733d01 and:

0objdump -p install_dir/bin/bitcoin-chainstate.exe | grep dll
1	DLL Name: libbitcoinkernel-0.dll
2	DLL Name: ADVAPI32.dll
3	DLL Name: KERNEL32.dll
4	DLL Name: msvcrt.dll

:smile:

TheCharlatan changes_requested

hebasto commented at 2:34 pm on February 23, 2023: member

5da7c0b3e34626ca57d1f0773db61e7d8351d8c7

Symbol visibility issues are not actually fixed yet because we have not yet defined an api and exported symbols, but everything is now in place for that.

Do “symbol visibility issues” include #26698?

I mean, while this PR targets libbitcoinkernel, will it benefit for libbitcoinconsensus as well?

theuni commented at 4:26 pm on February 23, 2023: member

@hebasto: I’m not sure how it looked before, but after this PR the exports look nice and clean:

 0$ objdump -p .libs/libbitcoinconsensus-0.dll
 1
 2...
 3
 4[Ordinal/Name Pointer] Table
 5        [   0] bitcoinconsensus_verify_script
 6        [   1] bitcoinconsensus_verify_script_with_amount
 7        [   2] bitcoinconsensus_version
 8
 9The Function Table (interpreted .pdata section contents)
10vma:                    BeginAddress     EndAddress       UnwindData
11 000000006db81000:      000000006d941000 000000006d94100c 000000006db8f000
12...

TheCharlatan commented at 9:07 pm on February 23, 2023: contributor

ACK 5da7c0b3e34626ca57d1f0773db61e7d8351d8c7

fanquake commented at 4:56 pm on February 24, 2023: member

This also fixes #19772. (updated PR description).

fanquake commented at 10:51 am on February 27, 2023: member

Guix Build:

 06d03c85b270c0239b3f480dbdbdd3e2465ac666228b0c9a1a3e71005bb9fef73  guix-build-5da7c0b3e346/output/aarch64-linux-gnu/SHA256SUMS.part
 1df27c28a6421f571c9f2b176dc2fb3d70a74898d9f85a278a5487d734767264e  guix-build-5da7c0b3e346/output/aarch64-linux-gnu/bitcoin-5da7c0b3e346-aarch64-linux-gnu-debug.tar.gz
 2bfe7ae61f2b60049f31b9e67e3b3144779a20506ba8874946499e56f558317d4  guix-build-5da7c0b3e346/output/aarch64-linux-gnu/bitcoin-5da7c0b3e346-aarch64-linux-gnu.tar.gz
 3423401caa7cc886b0eeec501d7f1e980765c346b339a3f00c29e2bc974203e87  guix-build-5da7c0b3e346/output/arm-linux-gnueabihf/SHA256SUMS.part
 40a92be521205c5a1d12715e7e3d3429d17eb00f6f5c2476b415c6be3535654be  guix-build-5da7c0b3e346/output/arm-linux-gnueabihf/bitcoin-5da7c0b3e346-arm-linux-gnueabihf-debug.tar.gz
 57e8a9831033ff228a1821cf5f7e32aa1ccfb87c5b0e0d24674ca766f8b0ae659  guix-build-5da7c0b3e346/output/arm-linux-gnueabihf/bitcoin-5da7c0b3e346-arm-linux-gnueabihf.tar.gz
 6e5906f09e50a624154d8ca7837a62246dd50cd2800a68da0c26e011230e3f392  guix-build-5da7c0b3e346/output/arm64-apple-darwin/SHA256SUMS.part
 7be98470a5c9154e51b08b292cd54e181e535025ddced95090614434d9a0469bf  guix-build-5da7c0b3e346/output/arm64-apple-darwin/bitcoin-5da7c0b3e346-arm64-apple-darwin-unsigned.dmg
 85f3d3eecf31d3fba82ac33f5444175caadf2fdc0ecf2b25f7288fa01818d83a3  guix-build-5da7c0b3e346/output/arm64-apple-darwin/bitcoin-5da7c0b3e346-arm64-apple-darwin-unsigned.tar.gz
 972635bacd67cfb9066dc00fe5591c42b23bd4cd5b6b281040ca725856a7a2f77  guix-build-5da7c0b3e346/output/arm64-apple-darwin/bitcoin-5da7c0b3e346-arm64-apple-darwin.tar.gz
109358d7509c7b9e25d1318717fdebef6284d1a0add4737674642357a06a72743b  guix-build-5da7c0b3e346/output/dist-archive/bitcoin-5da7c0b3e346.tar.gz
11a051381cf923a8b3d911cc860422519883f51c6ae85ef984fddea2ec426199b9  guix-build-5da7c0b3e346/output/powerpc64-linux-gnu/SHA256SUMS.part
1259865b697f11370cdd3cd2341d7e87494530509c641220b80753fc8f2001b3bf  guix-build-5da7c0b3e346/output/powerpc64-linux-gnu/bitcoin-5da7c0b3e346-powerpc64-linux-gnu-debug.tar.gz
13a7ce0ecf5c688d51514641fbf99fee2847aa89cafe0e3dd9114e9abd7a4b253b  guix-build-5da7c0b3e346/output/powerpc64-linux-gnu/bitcoin-5da7c0b3e346-powerpc64-linux-gnu.tar.gz
14fd82641c1a140419771a7544483645b0771eb052cd7c94a91f7ecb0aa477cd3f  guix-build-5da7c0b3e346/output/powerpc64le-linux-gnu/SHA256SUMS.part
15dbc89e0a021cb9582a843ba260227e3ce1c661e147c79451eb125b5928440104  guix-build-5da7c0b3e346/output/powerpc64le-linux-gnu/bitcoin-5da7c0b3e346-powerpc64le-linux-gnu-debug.tar.gz
16f46c59c1b54276aa0e532c736aef761c9019386f2e31643487746f232842486e  guix-build-5da7c0b3e346/output/powerpc64le-linux-gnu/bitcoin-5da7c0b3e346-powerpc64le-linux-gnu.tar.gz
17194ce2eb90cc87741972c4ec1dc7e7abac9fec110b66cd8911bdbc662a3d005a  guix-build-5da7c0b3e346/output/riscv64-linux-gnu/SHA256SUMS.part
1888f03ec6d11ca1bae50bc4ef29c8bd4f5f507986a4117322fa118dff41cb797a  guix-build-5da7c0b3e346/output/riscv64-linux-gnu/bitcoin-5da7c0b3e346-riscv64-linux-gnu-debug.tar.gz
19cb2439441f576f788d90925ef80eaa7b1ec247435d9deac42a4f2036c7c5f0a0  guix-build-5da7c0b3e346/output/riscv64-linux-gnu/bitcoin-5da7c0b3e346-riscv64-linux-gnu.tar.gz
2029ce824daff60f45d7acc8d42ba850e99a7f2736aa2b221099fb00f37cfe7706  guix-build-5da7c0b3e346/output/x86_64-apple-darwin/SHA256SUMS.part
217295b23fa0bfcbef2cd58d9a0a6fed74f8f6aba07fca3a941ac8a2f912270282  guix-build-5da7c0b3e346/output/x86_64-apple-darwin/bitcoin-5da7c0b3e346-x86_64-apple-darwin-unsigned.dmg
2245c14c1014aabd5f61bc97d2c58413685f4cbaa7d8d7fb50392bd033701724e8  guix-build-5da7c0b3e346/output/x86_64-apple-darwin/bitcoin-5da7c0b3e346-x86_64-apple-darwin-unsigned.tar.gz
23fc59db9002566e6f5fa6e279dece0d9ed62496b3a771839eb07ecbd56896ed01  guix-build-5da7c0b3e346/output/x86_64-apple-darwin/bitcoin-5da7c0b3e346-x86_64-apple-darwin.tar.gz
24f701d2563901970dc5356291b2a2e5bb55bfb582dc6e4f12ef860d302417c9f0  guix-build-5da7c0b3e346/output/x86_64-linux-gnu/SHA256SUMS.part
254133ede6b77dc66fcbafa27428542a63aaf987dd3e40d43572a9b02de6f4e427  guix-build-5da7c0b3e346/output/x86_64-linux-gnu/bitcoin-5da7c0b3e346-x86_64-linux-gnu-debug.tar.gz
2633d88ed1eb6e4449037b037ec29e256ce3541ad289522054f39c9bf3f1829bcd  guix-build-5da7c0b3e346/output/x86_64-linux-gnu/bitcoin-5da7c0b3e346-x86_64-linux-gnu.tar.gz
27f29e9e08ee487865f6b0dc1b77edee6c88d9949499537dd2b5b33d12437432b7  guix-build-5da7c0b3e346/output/x86_64-w64-mingw32/SHA256SUMS.part
28ec201af754e1f4a7967f8a7fddaff7de05a648ce6e52ea488e54315173bbb3f8  guix-build-5da7c0b3e346/output/x86_64-w64-mingw32/bitcoin-5da7c0b3e346-win64-debug.zip
29cfbe6d3946683ee6dd484b734a505a3af682b7c162d994de61300c52c234a345  guix-build-5da7c0b3e346/output/x86_64-w64-mingw32/bitcoin-5da7c0b3e346-win64-setup-unsigned.exe
3066d9d132c63b4d21ee388b9a9aa09981105914f8920a205aadc570fcf8f918ee  guix-build-5da7c0b3e346/output/x86_64-w64-mingw32/bitcoin-5da7c0b3e346-win64-unsigned.tar.gz
31f00786b2e68c3f0afc6c3f1464cc0b6c59adf1cfc586b73bb5008e9b7f46a1f0  guix-build-5da7c0b3e346/output/x86_64-w64-mingw32/bitcoin-5da7c0b3e346-win64.zip

fanquake added this to the milestone 25.0 on Feb 27, 2023

fanquake merged this on Feb 27, 2023

fanquake closed this on Feb 27, 2023

hebasto commented at 2:58 pm on February 27, 2023: member

This also fixes #19772. (updated PR description).

Confirming that this PR indeed fixes #19772.

sidhujag referenced this in commit cd8d490f50 on Feb 27, 2023

jonesk7734 commented at 4:21 am on September 20, 2023: none

Ok

bitcoin locked this on Sep 19, 2024

Fix various libbitcoinkernel DLL build problems #27146

Reviews