This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Omit dllimport in public headers in MinGW mode
ClosedPublic

Authored by mstorsjo on May 18 2022, 1:06 PM.

Details

Summary

In MinGW environments, thanks to slightly different code generation
and linker tricks, it's possible to link against a DLL C++ standard
library without dllimport attributes.

This allows using one single set of headers for linking against
either the DLL or a static library, leaving the decision entirely
up to the linking stage (where it can be switched with options like
-static-libstdc++).

This matches how libstdc++ headers work; there's no dllimport attributes
by default (unless the user has defined _GLIBCXX_DLL when including
headers).

This allows using one single set of headers while linking against
either a DLL or a static library, just like on Unix platforms.

This matches how libc++ has been used in MinGW configurations for
years (by first building the DLL, then configuring a static-only
build and installing on top, overwriting the libc++ config file
with one for static linking) by multiple MinGW toolchains, making
the dllimport-less use the official, tested configuration. This
also allows building all of libc++ in one single CMake configuration,
instead of having to do two separate builds on top of each other.

(Linking against a DLL without dllimport can break if e.g. templates
use inconsistent visibility attributes - in cases where it still
works when using explicit dllimport; such a case was fixed in
948dd664c3ed30dd853df03cb931436f280bad4a / D99932. With this as the
default configuration, we can catch such issues in CI.)

This patch allows opting in back to dllimports by defining
_LIBCPP_DLLIMPORT. (Alternatively, one could consider a CMake build
time option.)

Diff Detail

Event Timeline

mstorsjo created this revision.May 18 2022, 1:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2022, 1:06 PM
mstorsjo requested review of this revision.May 18 2022, 1:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2022, 1:06 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mati865 accepted this revision.May 18 2022, 3:04 PM

Thank you! It will surely make building easier.

thieta accepted this revision.May 20 2022, 1:31 AM

Nice!

ldionne requested changes to this revision.Jun 2 2022, 8:01 AM

Looks OK, but I'd like to understand the need for _LIBCPP_DLLIMPORT before we go forward.

libcxx/include/__config
542

Aw, I don't like how that gets messy and this condition overlaps with #elif defined(_LIBCPP_BUILDING_LIBRARY) below, but I tried making it better and I don't see how. I suggest we go with this and then I can refactor the whole family of visibility macros to be per-platform, which would make everything much easier to understand IMO.

Also, why do we need to introduce _LIBCPP_DLLIMPORT? Can't we use the new behavior on MinGW unconditionally?

This revision now requires changes to proceed.Jun 2 2022, 8:01 AM
mstorsjo requested review of this revision.Jun 2 2022, 9:28 AM
mstorsjo added inline comments.
libcxx/include/__config
542

Yep, I don’t like it either, but the alternative would probably require even more reshuffling.

We don’t strictly need _LIBCPP_DLLIMPORT - I wanted to keep it as a convenient way to opt back in to the old dllimport style, which mostly would be useful for “in the field debugging”. But now when this configuration would be tested by CI, there should be less risk/need for such debugging - so if you don’t want that extra backdoor, I can leave it out.

ldionne accepted this revision.Jun 6 2022, 7:31 AM

LGTM with green CI and leaving _LIBCPP_DLLIMPORT out. Please ping me on Discord once this is landed, I want to see if we can untangle the visibility macros by shuffling stuff around. Thanks!

This revision is now accepted and ready to land.Jun 6 2022, 7:31 AM
mstorsjo updated this revision to Diff 434506.Jun 6 2022, 9:24 AM

Remove _LIBCPP_DLLIMPORT.

This revision was automatically updated to reflect the committed changes.