`libbitcoinkernel`: Building `mingw-w64` dll's broken

dongcarl commented at 3:58 pm on April 27, 2022: contributor

We’re unable to build a dll for libbitcoinkernel right now because of unique problems arising out of the combination of mingw-w64 + winpthread + libtool + dll's

https://github.com/bitcoin/bitcoin/blob/466c6161505f0c9c68e4c4f41e3e40b8660d3a7a/src/Makefile.am#L816-L822

Various methods have been proposed and we should discuss here.

dongcarl commented at 3:59 pm on April 27, 2022: contributor

Previous discussion originally found here by theuni reproduced:

@theuni

does the switch to libtool libs mean that more objects end up being built (other than the second set for the lib)? Or does -all-static basically fix that?

Unfortunately, libtool seems to always want to build twice… Not sure if we can fix that.

I think I’ve come up with a reasonably clean solution to this here: https://github.com/theuni/bitcoin/commit/9ffbd707afc8188fe0b8cc5316f6c583f7ae21d0

libtool tries to build twice by default: once for static, once for shared. With this change, there’s only static by default, so default configs continue to only build one set of objects.

With this commit, if you want shared libs/dlls, you’d need to configure with --enable-shared, which will then build everything twice. For a project that’s mostly about executables, that seems reasonable to me. (Also, amazingly --enable-shared --disable-static does the right thing and also builds a single copy of everything for shared libs)

This also means we don’t have to special-case the Windows case in this pr. We’ll just know that --enable-shared is busted on windows (and maybe warn as such for that config?)

The only downside I can think of is that devs aren’t going to be linking the lib every day as part of their typical workflows so link problems may show up around release time. IMO that’s a reasonable trade-off.

dongcarl commented at 4:00 pm on April 27, 2022: contributor

Previous discussion originally found here by theuni reproduced:

This commit fixes Windows dll linking for me: https://github.com/theuni/bitcoin/commit/9612f5f0f1e3295065dbd8a0e0f472f8f020bbd9 . Untested, but it now links. I’ll be testing it out as a next step.

Thanks to @dongcarl for pointing out the problem.

Edit: I’m unable to find what’s actually affected by this define in my local mingw tree and in depends. Maybe gcc does something internally with it or something?

Edit2: Found it!: https://github.com/mingw-w64/mingw-w64/blob/f79f89f976dc5345090db557fb20d5deba913d9f/mingw-w64-libraries/winpthreads/include/pthread.h#L86

That seems like heavy-handed behavior for mingw-w64, much more useful would be for them to define and use PTHREADS_DLL_EXPORT instead. I’ll work to understand this better and upstream a patch if necessary.

hebasto commented at 4:06 pm on April 27, 2022: member

Could another approach suggested in #24994 be useful (did not test yet)?

theuni commented at 4:27 pm on April 27, 2022: member

I believe that https://github.com/theuni/bitcoin/commit/9612f5f0f1e3295065dbd8a0e0f472f8f020bbd9 is the clean/proper fix for us for the issue. We don’t want pthreads symbols exported, and that fixes the specific issue.

To me, the only remaining question is why other projects aren’t equally annoyed by this. I’ll work on upstreaming the fix (mingw-w64 pthreads using its own specific define instead of the generic DLL_EXPORT) and see how it’s received.

hebasto commented at 4:35 pm on April 27, 2022: member

@theuni

What are default values of lt_cv_prog_compiler_pic and lt_cv_prog_compiler_pic_CXX for mingw-w64?

theuni commented at 8:52 pm on April 27, 2022: member

@hebasto Sorry, that wasn’t clear from the change.

Without this hack, they contain:-DDLL_EXPORT -DPIC. So the override here simply removes the DLL_EXPORT define.

hebasto commented at 7:00 pm on April 28, 2022: member

I’ve tested @theuni’s patch, and it works for me.

It fixes #19772 and #24972 as well.

Considering this patch as libtool hack, still researching non-hack ways to fix the problem..

fanquake commented at 7:51 am on April 29, 2022: member

I believe that https://github.com/theuni/bitcoin/commit/9612f5f0f1e3295065dbd8a0e0f472f8f020bbd9 is the clean/proper fix for us for the issue. We don’t want pthreads symbols exported, and that fixes the specific issue.

ACK https://github.com/theuni/bitcoin/commit/9612f5f0f1e3295065dbd8a0e0f472f8f020bbd9, NACK #24972, #24994 etc.

theuni commented at 5:19 pm on April 29, 2022: member

I’ve opened a pull-request upstream to add a fix for this: https://sourceforge.net/p/mingw-w64/mingw-w64/merge-requests/23/

I’ll wait a day or two and see if they respond before going any further on this.

dongcarl commented at 6:19 pm on April 29, 2022: contributor

I actually think it’s entirely possible that building a convenience library and then an actual library out of the convenience library could be the way to do it like in #24994. We shall see.

Another point of reference here is the libtool manual section about how to handle DLL_EXPORT: https://www.gnu.org/software/libtool/manual/libtool.html#Windows-DLLs

fanquake commented at 6:43 pm on April 29, 2022: member

I actually think it’s entirely possible that building a convenience library and then an actual library out of the convenience library could be the way to do it like in #24994. We shall see.

I don’t see why we should be refactoring our code, that works everywhere else, and adding more libraries, to work around a problem that occurs for a single OS, and in some part, only in certain build configurations. Especially if we know that it’s libtool that is “broken”, it’d seem better for us to just use a single, well documented, scoped to the Windows use of libtool workaround, as opposed to changing our source code to accommodate its quirkiness.

Note that #24994 alone seemingly doesn’t fix all the issues, i.e #24972, so even if we went ahead with either/both changes, we’re still going to continue to carry the windows-only libtool workarounds we’ve already got in our build system, which I’d say is the worst of both worlds (code refactoring + still having windows libtool workarounds). I’d rather add one more (well documented) libtool workaround to the pile, and hope that this may all become a moot point some time into the future (cmake 🦆s). Interested to see what appears on the mailing list upstream.

theuni commented at 6:52 pm on April 29, 2022: member

I’m not sure I understand the discussion about nested librarries, or avoiding libtool. This problem has nothing to do with libtool except that libtool is the one guilty of adding the problematic define.

Please note also that any library could be causing this problem. It just happens to be libpthread. It could equally be libevent or libzmq.

Also please note that DLL_EXPORT is not any kind of magic define or token. It’s a convention used by libtool, similar to autoconf’s HAVE_CONFIG_H. In this case though, there’s a clash of uses.

Here’s the snippet we’re dealing with in pthread.h:

0#if defined DLL_EXPORT
1#ifdef IN_WINPTHREAD
2#define WINPTHREAD_API __declspec(dllexport)
3#else
4#define WINPTHREAD_API __declspec(dllimport)
5#endif
6#else
7#define WINPTHREAD_API
8#endif

Here’s part of the explanation I provided in the patch to upstream:

When building a libtool lib, libtool automatically defines “DLL_EXPORT” for objects intended to be linked into a shared library.

Applications are then intended to also define “DLL_EXPORT”, which has the effect of importing the functions from the exported dll.

However, this goes bad when a downstream user is itself a libtool lib which is attempting to statically link in libpthread. libtool sets “DLL_EXPORT” for itself as expected, so that the downstream lib can export its own symbols. HOWEVER it also has the effect of trying to import the libpthread symbols, which doesn’t work because libpthread is being linked statically rather than as a dll.

So the problem is that we’re trying to import when building a dll. From the Microsoft docs:

You only want to use __declspec(dllimport) when calling DLL functions from outside the DLL itself. Don’t use __declspec(dllimport) on functions inside a DLL when building that DLL.

The fix is thus to avoid doing the dllimport when building our own dll. The nice way would be to have a define that could be set to avoid it. This is the fix I’ve proposed upstream.

Barring that, we just need to avoid having DLL_EXPORT defined in any file that ends up including pthread.h. Libtool appends this by default. The configure change I’ve proposed above prevents libtool from doing that, thereby fixing the problem.

Another solution might be to add -UDLL_EXPORT to AM_CPPFLAGS, overriding libtool’s define.

Edit: In case it isn’t clear/obvious, upstream winpthreads itself is build using libtool, which is why they use that define and why it’s colliding with ours.

dongcarl commented at 6:58 pm on April 29, 2022: contributor

Ah! The upstream explanation is very clear indeed. Makes sense!

ryanofsky commented at 7:25 pm on April 29, 2022: contributor

Another solution might be to add -UDLL_EXPORT to AM_CPPFLAGS, overriding libtool’s define.

Another option might be linking against the pthread library dynamically instead of statically when building a dynamic library, in which case mingw’s __declspec(dllimport) annotations would be correctly applied.

I think linking dependencies of a dynamic library into it dynamically is more typical if you are using dynamic libraries to begin with. Linking dependencies of a dynamic library into it statically can also make sense, but can cause problems because this can leave you with multiple instances of the same static library having to coexist inside the same process, each with their own separate global variables, and possibly inconsistent function definitions. This is ok if libraries are high level, and you aren’t trying to share pointers or handles between the different library instances. I don’t think we know if this applies to pthreads, but Cory’s upstream patch should let us find out.

theuni commented at 7:42 pm on April 29, 2022: member

ACK. I explored this a little bit today because I agree a single dll may or may not be how we want to ship in the end.

In fact, to your point, libstdc++ currently still gets linked dynamically. This can be fixed as usual (I did a quick check) by adding -static-libstdc++ to the dll link flags, but it definitely begs the question: do we want to ship a few dll’s instead?

I think at least in the beginning we should try to lump everything into a single dll. By controlling our exports (I think) we should be able to avoid leaking out symbols like pthread_create which would obviously be problematic.

But I agree it’s very possible we run into startup problems or handle collisions or something that forces us to switch to multiple dll’s. Something like an app interacting with thread_local vars on a thread spawned by the dll… I could see that going sideways.

hebasto commented at 10:23 am on April 30, 2022: member

I’d rather add one more (well documented) libtool workaround to the pile…

Just a reminder, that this will be the 4th Windows-related Libtool hack in our build system (sorry if it looks like or is off-topic).

First is: https://github.com/bitcoin/bitcoin/blob/23daa86ec132cf1208b8c1b13463e58e57a26a20/configure.ac#L696-L698

Second: https://github.com/bitcoin/bitcoin/blob/23daa86ec132cf1208b8c1b13463e58e57a26a20/configure.ac#L61-L68

Third: https://github.com/bitcoin/bitcoin/blob/23daa86ec132cf1208b8c1b13463e58e57a26a20/configure.ac#L712-L718

Btw, the third Libtool hack did actually cause the #19772 bug.

theuni commented at 5:22 pm on May 2, 2022: member

Upstream acknowledged the problem and committed a fix that goes even further than my proposed patch: https://sourceforge.net/p/mingw-w64/mingw-w64/ci/489520900fb6853c8be97e9d0214f39a77c846d9/

(Funny enough, they went back and forth a few times to arrive at that fix, so I suppose this is quite subtle indeed)

So the good news is: this is now fixed upstream and we don’t have to opt-in, it’ll just work when building against the next release. The bad news is: we’ll have to carry a work-around until this trickles down to the majority of mingw toolchains, which will take several years.

theuni commented at 9:15 pm on May 4, 2022: member

libsecp256k1 actually has the opposite problem: leaking exported symbols into static libraries.

I’ve PR’d a fix for that upstream: https://github.com/bitcoin-core/secp256k1/pull/1105

Without that fix, the windows dll links successfully, but exports only the libsecp symbols due to ld’s behavior (emphasis mine):

–export-all-symbols If given, all global symbols in the objects used to build a DLL will be exported by the DLL. Note that this is the default if there otherwise wouldn’t be any exported symbols.

So basically, because libsecp256k1’s symbols are exported, nothing else gets exported. By fixing that leak we get back to the default of all exports, which is what we’re after (for now).

maflcko referenced this in commit b91055ea55 on Jun 13, 2022

sidhujag referenced this in commit 3704ef4fcf on Jun 15, 2022

TheCharlatan commented at 9:22 am on February 21, 2023: contributor

I believe that https://github.com/theuni/bitcoin/commit/9612f5f0f1e3295065dbd8a0e0f472f8f020bbd9 is the clean/proper fix for us for the issue. We don’t want pthreads symbols exported, and that fixes the specific issue.

Also tested this. Has this been PR’ed?

theuni commented at 9:54 pm on February 21, 2023: member

@TheCharlatan I just rebased and pushed up a branch with a combo for dll’s that seems to work.

Mind testing https://github.com/theuni/bitcoin/commits/fix-dll-exports ? I’ll PR if it works for you.

Symbol visibility can be tested by marking a random function as dllexport. When dumped with objdump -p, you should see only your selected exported symbols.

TheCharlatan commented at 12:40 pm on February 22, 2023: contributor

Mind testing https://github.com/theuni/bitcoin/commits/fix-dll-exports ? I’ll PR if it works for you.

Mmh, I’m surprised that secp needs to be linked as well by the final binary and I’m not quite following why. What’s different between the visibility of say linking leveldb and linking libsecp in libbitcoinkernel so these symbols end up undefined in the bitcoin-chainstate binary?

theuni commented at 4:10 pm on February 22, 2023: member

@TheCharlatan symbol visibility is really complex, but it boils down to 2 main choices on our side, see here: #25008 (comment)

We can either ship a bunch of dll’s (kernel, libsecp256k1, libstdc++, libpthread, etc) or We can roll everything up into 1 dll and have it just work.

For now, I propose we do the latter. However, this means we need tight control over our symbol visibility. If you’re not familiar, see here for the canonical primer: https://gcc.gnu.org/wiki/Visibility

tl;dr: We set everything to “hidden”, then manually mark the functions that are meant to be visible.

I’ve pushed up a branch that defines our current api as used by bitcoin-chainstate here: https://github.com/theuni/bitcoin/commits/fix-dll-exports2 . This includes #27144 as a bugfix if you wouldn’t mind reviewing.

Note that this branch is just a test to see how we’re currently looking. At the moment we’re exporting random functions and globals all over the place. Obviously that’s not ideal, but at least this helps us to see where we are.

This can be checked by building a shared bitcoin-chainstate.exe. If any of the EXPORT_SYMBOLs are removed, bitcoin-chainstate.exe will fail to link.

To answer your question about secp.. It is not marked as exported by libkernel (we fixed that in https://github.com/bitcoin-core/secp256k1/pull/1105). This means that libkernel can use secp internally, but any exe that links against it can’t see those functions. Just like it can’t see pthreads/libstdc++/etc. If exe’s saw those functions, they’d collide with their own toolchain functions and things would get weird.

However, this introduces the possibility of having libkernel and bitcoin-chainstate using different secp versions. For this reason, we may decide to ship multiple dll’s instead. But either way, we’ll want to define our function visibility in order to build the smallest possible binaries.

Hope that thought-dump helps, I’d be happy discuss further in realtime.

fanquake closed this on Feb 27, 2023

bitcoin locked this on Feb 27, 2024

libbitcoinkernel: Building mingw-w64 dll’s broken #25008

`libbitcoinkernel`: Building `mingw-w64` dll’s broken #25008