Fix various libbitcoinkernel DLL build problems #27146

theuni · 2023-02-22T21:26:10Z

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

This fixes libbitcoinkernel dll linking.

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

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.

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.

DrahtBot · 2023-02-22T21:26:13Z

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.

fanquake · 2023-02-23T09:40:54Z

Concept ACK

TheCharlatan · 2023-02-23T09:16:42Z

src/Makefile.am

-# bitcoin-chainstate against libbitcoinkernel as a shared or static library by
-# setting --{en,dis}able-shared.
-bitcoin_chainstate_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(PTHREAD_FLAGS)
+bitcoin_chainstate_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(PTHREAD_FLAGS) $(LIBTOOL_APP_LDFLAGS) -static


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

From libtool help:

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

TheCharlatan · 2023-02-23T09:46:55Z

src/Makefile.am

 bitcoin_chainstate_LDADD = $(LIBBITCOINKERNEL)
+
+# libtool is unable to calculate this indirect dependency, presumably because it's a subproject.
+# libsecp256k1 only needs to be linked in when libbitcoinkernel is static.


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

make distclean ./autogen.sh CONFIG_SITE=~/bitcoin/depends/x86_64-w64-mingw32/share/config.site ./configure \ --with-experimental-kernel-lib \ --enable-experimental-util-chainstate \ --with-experimental \ --prefix ~/bitcoin/install_dir \ --disable-static make install

But I'm getting a bunch of linker errors:

/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 /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 /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': ./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 /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': ./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 /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': ./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

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 theuni@1733d01 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.

Nice, I applied theuni/bitcoin@1733d01 and:

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

😄

hebasto · 2023-02-23T14:34:01Z

5da7c0b

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 · 2023-02-23T16:26:37Z

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

$ objdump -p .libs/libbitcoinconsensus-0.dll

...

[Ordinal/Name Pointer] Table
        [   0] bitcoinconsensus_verify_script
        [   1] bitcoinconsensus_verify_script_with_amount
        [   2] bitcoinconsensus_version

The Function Table (interpreted .pdata section contents)
vma:                    BeginAddress     EndAddress       UnwindData
 000000006db81000:      000000006d941000 000000006d94100c 000000006db8f000
...

TheCharlatan · 2023-02-23T21:07:58Z

ACK 5da7c0b

fanquake · 2023-02-24T16:56:02Z

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

fanquake · 2023-02-27T10:51:08Z

Guix Build:

6d03c85b270c0239b3f480dbdbdd3e2465ac666228b0c9a1a3e71005bb9fef73  guix-build-5da7c0b3e346/output/aarch64-linux-gnu/SHA256SUMS.part
df27c28a6421f571c9f2b176dc2fb3d70a74898d9f85a278a5487d734767264e  guix-build-5da7c0b3e346/output/aarch64-linux-gnu/bitcoin-5da7c0b3e346-aarch64-linux-gnu-debug.tar.gz
bfe7ae61f2b60049f31b9e67e3b3144779a20506ba8874946499e56f558317d4  guix-build-5da7c0b3e346/output/aarch64-linux-gnu/bitcoin-5da7c0b3e346-aarch64-linux-gnu.tar.gz
423401caa7cc886b0eeec501d7f1e980765c346b339a3f00c29e2bc974203e87  guix-build-5da7c0b3e346/output/arm-linux-gnueabihf/SHA256SUMS.part
0a92be521205c5a1d12715e7e3d3429d17eb00f6f5c2476b415c6be3535654be  guix-build-5da7c0b3e346/output/arm-linux-gnueabihf/bitcoin-5da7c0b3e346-arm-linux-gnueabihf-debug.tar.gz
7e8a9831033ff228a1821cf5f7e32aa1ccfb87c5b0e0d24674ca766f8b0ae659  guix-build-5da7c0b3e346/output/arm-linux-gnueabihf/bitcoin-5da7c0b3e346-arm-linux-gnueabihf.tar.gz
e5906f09e50a624154d8ca7837a62246dd50cd2800a68da0c26e011230e3f392  guix-build-5da7c0b3e346/output/arm64-apple-darwin/SHA256SUMS.part
be98470a5c9154e51b08b292cd54e181e535025ddced95090614434d9a0469bf  guix-build-5da7c0b3e346/output/arm64-apple-darwin/bitcoin-5da7c0b3e346-arm64-apple-darwin-unsigned.dmg
5f3d3eecf31d3fba82ac33f5444175caadf2fdc0ecf2b25f7288fa01818d83a3  guix-build-5da7c0b3e346/output/arm64-apple-darwin/bitcoin-5da7c0b3e346-arm64-apple-darwin-unsigned.tar.gz
72635bacd67cfb9066dc00fe5591c42b23bd4cd5b6b281040ca725856a7a2f77  guix-build-5da7c0b3e346/output/arm64-apple-darwin/bitcoin-5da7c0b3e346-arm64-apple-darwin.tar.gz
9358d7509c7b9e25d1318717fdebef6284d1a0add4737674642357a06a72743b  guix-build-5da7c0b3e346/output/dist-archive/bitcoin-5da7c0b3e346.tar.gz
a051381cf923a8b3d911cc860422519883f51c6ae85ef984fddea2ec426199b9  guix-build-5da7c0b3e346/output/powerpc64-linux-gnu/SHA256SUMS.part
59865b697f11370cdd3cd2341d7e87494530509c641220b80753fc8f2001b3bf  guix-build-5da7c0b3e346/output/powerpc64-linux-gnu/bitcoin-5da7c0b3e346-powerpc64-linux-gnu-debug.tar.gz
a7ce0ecf5c688d51514641fbf99fee2847aa89cafe0e3dd9114e9abd7a4b253b  guix-build-5da7c0b3e346/output/powerpc64-linux-gnu/bitcoin-5da7c0b3e346-powerpc64-linux-gnu.tar.gz
fd82641c1a140419771a7544483645b0771eb052cd7c94a91f7ecb0aa477cd3f  guix-build-5da7c0b3e346/output/powerpc64le-linux-gnu/SHA256SUMS.part
dbc89e0a021cb9582a843ba260227e3ce1c661e147c79451eb125b5928440104  guix-build-5da7c0b3e346/output/powerpc64le-linux-gnu/bitcoin-5da7c0b3e346-powerpc64le-linux-gnu-debug.tar.gz
f46c59c1b54276aa0e532c736aef761c9019386f2e31643487746f232842486e  guix-build-5da7c0b3e346/output/powerpc64le-linux-gnu/bitcoin-5da7c0b3e346-powerpc64le-linux-gnu.tar.gz
194ce2eb90cc87741972c4ec1dc7e7abac9fec110b66cd8911bdbc662a3d005a  guix-build-5da7c0b3e346/output/riscv64-linux-gnu/SHA256SUMS.part
88f03ec6d11ca1bae50bc4ef29c8bd4f5f507986a4117322fa118dff41cb797a  guix-build-5da7c0b3e346/output/riscv64-linux-gnu/bitcoin-5da7c0b3e346-riscv64-linux-gnu-debug.tar.gz
cb2439441f576f788d90925ef80eaa7b1ec247435d9deac42a4f2036c7c5f0a0  guix-build-5da7c0b3e346/output/riscv64-linux-gnu/bitcoin-5da7c0b3e346-riscv64-linux-gnu.tar.gz
29ce824daff60f45d7acc8d42ba850e99a7f2736aa2b221099fb00f37cfe7706  guix-build-5da7c0b3e346/output/x86_64-apple-darwin/SHA256SUMS.part
7295b23fa0bfcbef2cd58d9a0a6fed74f8f6aba07fca3a941ac8a2f912270282  guix-build-5da7c0b3e346/output/x86_64-apple-darwin/bitcoin-5da7c0b3e346-x86_64-apple-darwin-unsigned.dmg
45c14c1014aabd5f61bc97d2c58413685f4cbaa7d8d7fb50392bd033701724e8  guix-build-5da7c0b3e346/output/x86_64-apple-darwin/bitcoin-5da7c0b3e346-x86_64-apple-darwin-unsigned.tar.gz
fc59db9002566e6f5fa6e279dece0d9ed62496b3a771839eb07ecbd56896ed01  guix-build-5da7c0b3e346/output/x86_64-apple-darwin/bitcoin-5da7c0b3e346-x86_64-apple-darwin.tar.gz
f701d2563901970dc5356291b2a2e5bb55bfb582dc6e4f12ef860d302417c9f0  guix-build-5da7c0b3e346/output/x86_64-linux-gnu/SHA256SUMS.part
4133ede6b77dc66fcbafa27428542a63aaf987dd3e40d43572a9b02de6f4e427  guix-build-5da7c0b3e346/output/x86_64-linux-gnu/bitcoin-5da7c0b3e346-x86_64-linux-gnu-debug.tar.gz
33d88ed1eb6e4449037b037ec29e256ce3541ad289522054f39c9bf3f1829bcd  guix-build-5da7c0b3e346/output/x86_64-linux-gnu/bitcoin-5da7c0b3e346-x86_64-linux-gnu.tar.gz
f29e9e08ee487865f6b0dc1b77edee6c88d9949499537dd2b5b33d12437432b7  guix-build-5da7c0b3e346/output/x86_64-w64-mingw32/SHA256SUMS.part
ec201af754e1f4a7967f8a7fddaff7de05a648ce6e52ea488e54315173bbb3f8  guix-build-5da7c0b3e346/output/x86_64-w64-mingw32/bitcoin-5da7c0b3e346-win64-debug.zip
cfbe6d3946683ee6dd484b734a505a3af682b7c162d994de61300c52c234a345  guix-build-5da7c0b3e346/output/x86_64-w64-mingw32/bitcoin-5da7c0b3e346-win64-setup-unsigned.exe
66d9d132c63b4d21ee388b9a9aa09981105914f8920a205aadc570fcf8f918ee  guix-build-5da7c0b3e346/output/x86_64-w64-mingw32/bitcoin-5da7c0b3e346-win64-unsigned.tar.gz
f00786b2e68c3f0afc6c3f1464cc0b6c59adf1cfc586b73bb5008e9b7f46a1f0  guix-build-5da7c0b3e346/output/x86_64-w64-mingw32/bitcoin-5da7c0b3e346-win64.zip

hebasto · 2023-02-27T14:58:08Z

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

Confirming that this PR indeed fixes #19772.

jonesk7734

Ok

theuni added 4 commits February 22, 2023 19:04

build: don't define DLL_EXPORT for windows

9c253d2

This fixes libbitcoinkernel dll linking.

build: fix bitcoin-chainstate when libbitcoinkernel is static

545a74e

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

build: always build bitcoin-chainstate against static libbitcoinkernel

130490a

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.

build: allow libitcoinkernel dll builds now that exports are fixed

5da7c0b

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.

fanquake requested a review from TheCharlatan February 23, 2023 09:41

fanquake mentioned this pull request Feb 23, 2023

build: Build libbitcoinconsensus from its own convenience library #24994

Closed

TheCharlatan suggested changes Feb 23, 2023

View reviewed changes

fanquake added this to the 25.0 milestone Feb 27, 2023

fanquake merged commit 82793f1 into bitcoin:master Feb 27, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 27, 2023

Merge bitcoin#27146: Fix various libbitcoinkernel DLL build problems

cd8d490

jonesk7734 reviewed Sep 20, 2023

View reviewed changes

bitcoin locked and limited conversation to collaborators Sep 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix various libbitcoinkernel DLL build problems #27146

Fix various libbitcoinkernel DLL build problems #27146

Uh oh!

theuni commented Feb 22, 2023 •

edited by fanquake

Loading

Uh oh!

DrahtBot commented Feb 22, 2023 •

edited

Loading

Uh oh!

fanquake commented Feb 23, 2023

Uh oh!

TheCharlatan Feb 23, 2023

Uh oh!

theuni Feb 23, 2023

Uh oh!

TheCharlatan Feb 23, 2023

Uh oh!

theuni Feb 23, 2023

Uh oh!

TheCharlatan Feb 23, 2023

Uh oh!

hebasto commented Feb 23, 2023 •

edited

Loading

Uh oh!

theuni commented Feb 23, 2023

Uh oh!

TheCharlatan commented Feb 23, 2023

Uh oh!

fanquake commented Feb 24, 2023 •

edited

Loading

Uh oh!

fanquake commented Feb 27, 2023

Uh oh!

hebasto commented Feb 27, 2023

Uh oh!

jonesk7734 left a comment

Uh oh!

Uh oh!

Fix various libbitcoinkernel DLL build problems #27146

Fix various libbitcoinkernel DLL build problems #27146

Uh oh!

Conversation

theuni commented Feb 22, 2023 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

fanquake commented Feb 23, 2023

Uh oh!

TheCharlatan Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

theuni Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

TheCharlatan Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

theuni Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

TheCharlatan Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

hebasto commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Feb 23, 2023

Uh oh!

TheCharlatan commented Feb 23, 2023

Uh oh!

fanquake commented Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Feb 27, 2023

Uh oh!

hebasto commented Feb 27, 2023

Uh oh!

jonesk7734 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theuni commented Feb 22, 2023 •

edited by fanquake

Loading

DrahtBot commented Feb 22, 2023 •

edited

Loading

hebasto commented Feb 23, 2023 •

edited

Loading

fanquake commented Feb 24, 2023 •

edited

Loading