This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add the _LIBCPP_HIDE_FROM_ABI_AFTER_V1 macro
ClosedPublic

Authored by ldionne on Jul 27 2018, 6:41 AM.

Details

Summary

This macro allows hiding symbols from the ABI when the library is built
with an ABI version after ABI v1, which is currently the only stable ABI.
This commit defines _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY to be
_LIBCPP_HIDE_FROM_ABI_AFTER_V1, meaning that symbols that were only
exported by the library for historical reasons are not exported anymore
in the unstable ABI.

Because of that, this commit is an ABI break for ABI v2. This ABI version
is not stable, however, so this should not be a problem.

Diff Detail

Repository
rL LLVM

Event Timeline

ldionne created this revision.Jul 27 2018, 6:41 AM

This is the second step of the proposal aiming to remove uses of __always_inline__ in the visibility macros. This proposal is documented on the list here: http://lists.llvm.org/pipermail/cfe-dev/2018-July/058633.html

ldionne updated this revision to Diff 157696.Jul 27 2018, 7:55 AM

Document the macro in the official documentation instead of the header, per
Eric's comment on a previous patch of mine.

This looks correct to me, but I wouldn't mind having someone else take a look too.

libcxx/include/__config
798 ↗(On Diff #157696)

It looks like if you switch this to #if !defined(...) you can use #elif instead of a nested #if. I think that would make the logic a bit more clear for me, but if you disagree feel free to leave it as is.

ldionne added inline comments.Jul 27 2018, 11:28 AM
libcxx/include/__config
798 ↗(On Diff #157696)

The reason I did it that way is that this structure will make it easier to add new versions in the future:

#ifdef _LIBCPP_BUILDING_LIBRARY
#  if _LIBCPP_ABI_VERSION > 1
#    define _LIBCPP_HIDE_FROM_ABI_AFTER_V1 _LIBCPP_HIDE_FROM_ABI
#  else
#    define _LIBCPP_HIDE_FROM_ABI_AFTER_V1
#  endif

#  if _LIBCPP_ABI_VERSION > 2
#    define _LIBCPP_HIDE_FROM_ABI_AFTER_V2 _LIBCPP_HIDE_FROM_ABI
#  else
#    define _LIBCPP_HIDE_FROM_ABI_AFTER_V2
#  endif

#else
#  define _LIBCPP_HIDE_FROM_ABI_AFTER_V1 _LIBCPP_HIDE_FROM_ABI
#  define _LIBCPP_HIDE_FROM_ABI_AFTER_V2 _LIBCPP_HIDE_FROM_ABI
#endif

With your suggestion, the case where we have only one ABI version is indeed clearer, but it's not as easy to add a new version (remember than if the version is 2, both _LIBCPP_HIDE_FROM_ABI_AFTER_V1 and _LIBCPP_HIDE_FROM_ABI_AFTER_V2 would need to be defined, so an elif chain does not cut it):

// Hide symbols when we're not building the dylib
#if !defined(_LIBCPP_BUILDING_LIBRARY)
#  define _LIBCPP_HIDE_FROM_ABI_AFTER_V1 _LIBCPP_HIDE_FROM_ABI
// Otherwise, hide symbols depending on the ABI version used to build the dylib
#elif _LIBCPP_ABI_VERSION > 1
#  define _LIBCPP_HIDE_FROM_ABI_AFTER_V1 _LIBCPP_HIDE_FROM_ABI
#else
#  define _LIBCPP_HIDE_FROM_ABI_AFTER_V1
#endif

This basically needs to turn into what I've put above if we are to add a new ABI version.

mclow.lists accepted this revision.Jul 30 2018, 8:14 AM

This LGTM, but suggests another cleanup.

We have a bunch of _LIBCPP_BUILDING_XXX macros defined in libcxx/src - do we need them any more now that we have _LIBCPP_BUILDING_LIBRARY?

src/bind.cpp:10: #define _LIBCPP_BUILDING_BIND
src/memory.cpp:10: #define _LIBCPP_BUILDING_MEMORY
src/mutex.cpp:10: #define _LIBCPP_BUILDING_MUTEX
src/new.cpp:10: #define _LIBCPP_BUILDING_NEW
src/shared_mutex.cpp:13: #define _LIBCPP_BUILDING_SHARED_MUTEX
src/system_error.cpp:12: #define _LIBCPP_BUILDING_SYSTEM_ERROR
src/utility.cpp:10: #define _LIBCPP_BUILDING_UTILITY

I can take care of this post-branch for release.

This revision is now accepted and ready to land.Jul 30 2018, 8:14 AM
This revision was automatically updated to reflect the committed changes.