This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Default to DLL semantics on Windows
AbandonedPublic

Authored by smeenai on Sep 23 2016, 12:37 PM.

Details

Summary

On Windows builds, we currently require _LIBCPP_DLL to be specified in
order for dllexport/dllimport annotations to be generated. I believe
it's better to do the opposite, i.e. require a macro to be present in
order to *not* include the annotations, since building libc++ shared is
more common.

The problem with having the DLL annotations be opt-in is that any
project using the libc++ headers and linking against a libc++ DLL must
specify _LIBCPP_DLL in its build system, otherwise the libc++ headers
will not be annotated with dllimport. This lack of annotation will then
cause inefficient code generation, since the linker will generate import
thunks. No compile-time errors will be reported (except in the case of
data symbols, for which import thunks are not possible) about this
inefficient code generation.

In contrast, having the DLL annotations be opt-out means that any
project using the libc++ headers and linking against a static libc++
will fail to link with missing import table entries if the project
forgets to specify the macro to suppress DLL annotations. Link errors
seem far preferable to silent inefficient code generation.

Diff Detail

Event Timeline

smeenai updated this revision to Diff 72326.Sep 23 2016, 12:37 PM
smeenai retitled this revision from to [libc++] Default to DLL semantics on Windows.
smeenai updated this object.
smeenai added reviewers: compnerd, EricWF, mclow.lists.
smeenai added a subscriber: cfe-commits.
EricWF resigned from this revision.Sep 26 2016, 3:30 PM
EricWF removed a reviewer: EricWF.

I fixed this with a slightly different patch in r282449. I hope you don't mind that I didn't use this patch, I just wanted to add support for generating custom __config headers for static builds on Windows. Let me know if you have any issues with my patch.

Resigning as a reviewer since this issue is fixed.

smeenai abandoned this revision.Sep 26 2016, 3:45 PM

Your patch looks good to me and is a nicer way of accomplishing this. Thanks for taking care of it!