Page MenuHomePhabricator

[libcxx] Special visibility macros for the experimental library
AcceptedPublic

Authored by hamzasood on Aug 26 2017, 9:00 AM.

Details

Summary

This patch fixes an issue that I encountered while porting the filesystem library to Windows (which I'll submit a patch for in a few days).

libc++experimental is always a static library, but on Windows the experimental APIs get marked with dllimport/dllexport.
This patch changes that by adding experimental visibility macros that don't use DLL declspecs on Windows.

Diff Detail

Event Timeline

hamzasood created this revision.Aug 26 2017, 9:00 AM
smeenai resigned from this revision.Aug 29 2017, 3:52 AM
smeenai added reviewers: mclow.lists, compnerd.

It's really cool that you're getting the filesystem library to work on Windows :)

This looks reasonable to me; it's the only way I can think of to get the experimental library working on Windows if we're keeping it static. It's a large change, but most of it is mechanical. Given its size, however, I also want to let at least one of Eric and Marshall take a look.

The other question, of course, is why the experimental library needs to be static. If it were built shared, the annotations would just work on Windows in theory (though I'm sure there are other issues there).

include/experimental/__config
57

I think it would make more sense to make these macros empty on all platforms, not just Windows. It's true that they'll only cause link errors on Windows (since you'll attempt to dllimport functions from a static library), but on ELF and Mach-O, the visibility annotations would cause these functions to be exported from whatever library c++experimental gets linked into, which is probably not desirable either.

The exception (if you'll pardon the pun) is _LIBCPPX_EXCEPTION_ABI, which will still need to expand to at least __type_visibility__((default)) for non-COFF in order for throwing and catching those types to work correctly.

The other question, of course, is why the experimental library needs to be static. If it were built shared, the annotations would just work on Windows in theory (though I'm sure there are other issues there).

Even if libc++experimental is no longer forced to be static, it'd probably be too restrictive to force libc++ and libc++experimental to be the same library type. E.g. someone might want a dynamic libc++ but a static libc++experimental. If that's possible (even if not by default), different visibility macros will be needed.

include/experimental/__config
57

I might be mistaken, but I think the regular libc++ library still uses visibility annotations when it's being built as a static library on non-Windows platforms. So I figured it'd be best to match that behaviour.

mclow.lists edited edge metadata.Aug 30 2017, 8:36 AM

The reason for building the filesystem library as a statically linked lib (instead of dynamic) is that for quite a while it was changing significantly. Having people link statically means that we can make changes w/o worrying (as much) about people using the library - they can pick up new features /changes when they re-link their programs. Once we change to a dylib (and we will, but not yet), then we have to worry about ABI changes affecting programs in the field.

hamzasood updated this revision to Diff 113399.Aug 31 2017, 7:30 AM

I've added a couple more Windows macro-related fixes to the experimental library.

Most importantly is the change to _LIBCPP_BUILDING_LIBRARY. Previously _LIBCPP_BUILDING_LIBRARY was defined for every source file, including files destined for the experimental library. As a result of this, non-experimental APIs were marked as dllexport when compiling the experimental library (which led to link errors whenever experimental code uses symbols from the regular libc++ library).
I've changed it so that _LIBCPP_BUILDING_LIBRARY is only defined for non-experimental source files, and _LIBCPPX_BUILDING_LIBRARY is defined for experimental source files (that macro isn't currently used for anything, it's mainly there for consistency).

I've also added the needed MSVCRT defines to the experimental code too, so that there won't be a bunch of warnings about using "deprecated" functions etc.

compnerd edited edge metadata.Aug 31 2017, 10:55 PM

I think that splitting this up in a series of patches would be much better. The first patch should be to do the entirely mechanical change of the visibility attribute. It is a separate library and needs its own visibility attribute. That would significantly slim down this subsequent change. A follow-up change could fix the build, and a third change adds the test harness/support the tests.

hamzasood updated this revision to Diff 113527.Sep 1 2017, 3:14 AM

Okay, I've split the patch up as requested.

This patch is now just the mechanical attribute change and documentation change.
The test case is now D37375.
The other Windows build fixes are D37376.

compnerd accepted this revision.Sep 2 2017, 10:21 AM
compnerd added inline comments.
include/experimental/__config
57

It's not about matching that behaviour, libc++ is built shared, this is built static. The static link marked default visibility would make the functions be re-exported. However, if I'm not mistaken, this already happens today, so fixing that in a follow-up is fine as the status quo is maintained.

This revision is now accepted and ready to land.Sep 2 2017, 10:21 AM

Thanks. Could someone commit this for me please?

include/experimental/__config
57

But isn't it possible to build libc++ as static instead of shared? (LIBCXX_ENABLE_STATIC=YES and LIBCXX_ENABLE_SHARED=NO).
I'm pretty sure that in that case, the visibility annotations are left active.

I have access now, so I'm able to commit this myself.
However it's been a while since it was approved, so I'd be grateful if someone could take another look to make sure nothing has changed in the meantime (besides potentially needing to re-tag some new APIs).

Sorry, this has been on my queue for a long time, but I haven't gotten the chance to get to it yet. I'll try to take a look (at this and your other patches) in the next few days.

This is an old patch; is this still needed/desired?