This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Make _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS export members
ClosedPublic

Authored by smeenai on Jan 25 2017, 4:20 PM.

Details

Summary

When building libc++ with hidden visibility, we want explicit template
instantiations to export members. This is consistent with existing
Windows behavior, and is necessary for clients to be able to link
against a hidden visibility built libc++ without running into lots of
missing symbols.

An unfortunate side effect, however, is that any template methods of a
class with an explicit instantiation will get default visibility when
instantiated, unless the methods are explicitly marked inline or hidden
visibility. This is not desirable for clients of libc++ headers who wish
to control their visibility, and led to PR30642.

Annotate all problematic methods with an explicit visibility specifier
to avoid this. The problematic methods were found by running
https://github.com/smeenai/bad-visibility-finder against the libc++
headers after making the _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS change. The
methods were marked with the new _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
macro, which was created for this purpose.

It should be noted that _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS was originally
intended to expand to default visibility, and was changed to expanding
to default type visibility to fix PR30642. The visibility macro
documentation was not updated accordingly, however, so this change makes
the macro consistent with its documentation again, while explicitly
fixing the methods which resulted in that PR.

Event Timeline

smeenai created this revision.Jan 25 2017, 4:20 PM
smeenai updated this revision to Diff 87912.Feb 9 2017, 4:28 PM

Rebase and ping

EricWF added inline comments.Feb 16 2017, 5:54 PM
include/string
1094

Why inline _LIBCPP_INLINE_VISIBILITY here but _LIBCPP_HIDDEN everywhere else?

EricWF edited edge metadata.Feb 16 2017, 6:47 PM

I applied inline to most of the functions changed in basic_string, and updated the ABI lists accordingly.

Please update this patch so it merges with trunk. Note you can add _LIBCPP_HIDDEN to the functions in basic_string as well if you want, however the inlineis enough to make them hidden in the dylib.

EricWF added inline comments.Feb 16 2017, 7:04 PM
include/locale
626

After applying the changes to __config but not this header check-cxx-abilist didn't report any changes to these symbols. Why is this needed again?

Will do the rebase.

include/locale
626

It's to prevent https://llvm.org/bugs/show_bug.cgi?id=30642 from occurring again. If an extern template class is marked default visibility, and you have template methods of that class which get instantiated in other libraries, those template method instantiations also get marked default visibility, so now other libraries are exporting libc++ symbols. This change marks all such template methods as hidden to prevent the leakage.

That might not be the most clear explanation, so https://ghostbin.com/paste/29y9d demonstrates what I mean. (It's also the root cause of PR30642.)

https://reviews.llvm.org/D25208 addresses the same issue for non-template classes.

include/string
1094

This function is really small, so I figured marking it for inlining was more appropriate.

EricWF accepted this revision.Feb 28 2017, 4:25 PM

LGTM, but with a macro other than _LIBCPP_HIDDEN.

Also could you please add doc explaining the rational for this new macro in depth. I think the summary for this revision would be sufficient.

This revision is now accepted and ready to land.Feb 28 2017, 4:25 PM
EricWF added inline comments.Feb 28 2017, 4:33 PM
include/string
1094

This should probably have _LIBCPP_INLINE_VISIBILITY on it then.

smeenai added inline comments.Feb 28 2017, 5:39 PM
include/string
1094

I was assuming any client that cares about what they're exporting would build with -fvisibility-inlines-hidden, in which case just inline should be enough.

smeenai updated this revision to Diff 90108.Feb 28 2017, 5:45 PM
smeenai edited the summary of this revision. (Show Details)

Addressing comments

smeenai updated this revision to Diff 90264.Mar 1 2017, 5:35 PM

Changing more instances to new macro, per IRC discussion with Eric

smeenai updated this revision to Diff 90272.Mar 1 2017, 7:10 PM

Including inline in macro to work around PR32114

This revision was automatically updated to reflect the committed changes.