This is an archive of the discontinued LLVM Phabricator instance.

Build with -fvisibility=hidden
ClosedPublic

Authored by EricWF on Oct 30 2018, 7:49 AM.

Details

Summary

This change changes the build to use -fvisibility=hidden

The exports this patch removes are symbols that should have never been exported
by the dylib in the first place, and should all be symbols which the linker
won't de-duplicate across SO boundaries, making them safe to remove.

After this change, we should be able to apply _LIBCPP_HIDDEN to the versioning namespace without changing the export lists.

Diff Detail

Repository
rCXX libc++

Event Timeline

EricWF created this revision.Oct 30 2018, 7:49 AM
mclow.lists added inline comments.Oct 30 2018, 8:27 AM
lib/abi/CHANGELOG.TXT
49

Are you sure about the string one? That's operator+(string, const char *)
Will that break anyone?

EricWF added inline comments.Oct 30 2018, 8:40 AM
lib/abi/CHANGELOG.TXT
49

Yeah, I'm sure about that one. We don't externally instantiate it unlike operator+(const char*, string), so it's just like any other template we define in the headers.

ldionne requested changes to this revision.Oct 30 2018, 9:13 AM

I think changing the default breaks _LIBCPP_HIDE_FROM_ABI_AFTER_V1:

#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
#else
#  define _LIBCPP_HIDE_FROM_ABI_AFTER_V1 _LIBCPP_HIDE_FROM_ABI
#endif

I think this would now need to be

#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 __attribute__((__visibility__(("default"))))
#  endif
#else
#  define _LIBCPP_HIDE_FROM_ABI_AFTER_V1 _LIBCPP_HIDE_FROM_ABI
#endif
This revision now requires changes to proceed.Oct 30 2018, 9:13 AM

I think changing the default breaks _LIBCPP_HIDE_FROM_ABI_AFTER_V1:

#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
#else
#  define _LIBCPP_HIDE_FROM_ABI_AFTER_V1 _LIBCPP_HIDE_FROM_ABI
#endif

I think this would now need to be

#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 __attribute__((__visibility__(("default"))))
#  endif
#else
#  define _LIBCPP_HIDE_FROM_ABI_AFTER_V1 _LIBCPP_HIDE_FROM_ABI
#endif

Making the change seems to have no effect on the export list.

I think what I would prefer is a patch that fixes the build with -fvisiblity=hidden on Linux using source-level annotations without actually building with -fvisibility=hidden. This can be done by using either _LIBCPP_EXPORTED_FROM_ABI if the thing should be visible, or mark it as hidden if it shouldn't.

Once this is done and libc++ works properly with inline visibility on Linux and MacOS, we can switch the namespace-scope attribute to make everything hidden _and_ fixup the macros that need to account for the change in default.

lib/abi/CHANGELOG.TXT
27

Why are we removing any symbols from the dylib? With the previous change I made, I already fixed the ABI lists and I assumed it would work just the same on Linux. Did the check-cxx-abilist fail on Linux when applying -fvisibility=hidden even after my patch?

I think what I would prefer is a patch that fixes the build with -fvisiblity=hidden on Linux using source-level annotations without actually building with -fvisibility=hidden. This can be done by using either _LIBCPP_EXPORTED_FROM_ABI if the thing should be visible, or mark it as hidden if it shouldn't.

I think we should be moving away from per-function source-level annotations.

Once this is done and libc++ works properly with inline visibility on Linux and MacOS, we can switch the namespace-scope attribute to make everything hidden _and_ fixup the macros that need to account for the change in default.

After this patch, we can switch to a namespace-scope attribute without anything changing (in terms of export lists). That was part of the goal of this patch: To update the export lists so that they matched what they'll be after the namespace change.

mclow.lists added inline comments.Oct 30 2018, 11:03 AM
lib/abi/CHANGELOG.TXT
49

You're kidding me, right? We externally instantiate _some_ of the operator+, but not others?
/me heads off to check this lunacy....

ldionne accepted this revision.Oct 30 2018, 2:14 PM

After talking to Eric, the plan is:

  1. Turn on -fvisibility=hidden when building
  2. Apply __attribute__((visibility("hidden"))) at namespace scope and remove the compiler flags.
  3. Start moving classes to use explicit lists of exported methods with default visibility attributes instead of the huge mess we have today.

I'm fine with this, but (3) still needs some preliminary work before it can happen fully (visibility attributes are not supported perfectly even on Clang trunk).

This revision is now accepted and ready to land.Oct 30 2018, 2:14 PM
This revision was automatically updated to reflect the committed changes.
lib/abi/CHANGELOG.TXT