This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Make _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS export members

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



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 against the libc++
headers after making the _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS change. The
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

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

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.


It's to prevent 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 demonstrates what I mean. (It's also the root cause of PR30642.) addresses the same issue for non-template classes.


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

This should probably have _LIBCPP_INLINE_VISIBILITY on it then.

smeenai added inline comments.Feb 28 2017, 5:39 PM

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.