This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use exclude_from_explicit_instantiation instead of always_inline
ClosedPublic

Authored by ldionne on Sep 23 2018, 2:22 PM.

Details

Summary

This commit adopts the exclude_from_explicit_instantiation attribute discussed
at [1] and reviewed in [2] in libc++ to supplant the use of always_inline
for visibility purposes.

This change means that users wanting to link together translation units built
with different versions of libc++’s headers into the same final linked image
MUST define the _LIBCPP_HIDE_FROM_ABI_PER_TU macro to 1 when building those
TUs. Doing otherwise will lead to ODR violations and ABI issues.

[1]: http://lists.llvm.org/pipermail/cfe-dev/2018-August/059024.html
[2]: https://reviews.llvm.org/D51789

Diff Detail

Repository
rCXX libc++
Build Status
Buildable 22971

Event Timeline

ldionne created this revision.Sep 23 2018, 2:22 PM
rsmith added a comment.Oct 3 2018, 5:14 PM

Makes sense to me. Is there some way you can write a test for this? (Maybe applying nm to a linked program?)

Makes sense to me. Is there some way you can write a test for this? (Maybe applying nm to a linked program?)

What I had in mind is that we should have a build bot that runs the test suite with/without _LIBCPP_HIDE_FROM_ABI_PER_TU. If all the current libc++ tests link, this seems to give us the most confidence that nothing is broken. Did you have something more specific in mind that you're concerned about? If so, I'm happy to write a test for it, I'd just like to understand what that is.

Also, are we good to go with https://reviews.llvm.org/D51789, which this patch requires?

rsmith added a comment.Oct 3 2018, 5:57 PM

Makes sense to me. Is there some way you can write a test for this? (Maybe applying nm to a linked program?)

What I had in mind is that we should have a build bot that runs the test suite with/without _LIBCPP_HIDE_FROM_ABI_PER_TU. If all the current libc++ tests link, this seems to give us the most confidence that nothing is broken. Did you have something more specific in mind that you're concerned about? If so, I'm happy to write a test for it, I'd just like to understand what that is.

I'm happy with any testing that works :)

Makes sense to me. Is there some way you can write a test for this? (Maybe applying nm to a linked program?)

What I had in mind is that we should have a build bot that runs the test suite with/without _LIBCPP_HIDE_FROM_ABI_PER_TU. If all the current libc++ tests link, this seems to give us the most confidence that nothing is broken. Did you have something more specific in mind that you're concerned about? If so, I'm happy to write a test for it, I'd just like to understand what that is.

I'm happy with any testing that works :)

Ok. Well, if I remove __always_inline__ without using exclude_from_explicit_instantiation, I get many link errors in the libc++ test suite. So I'm confident that the fact that libc++ isn't broken is already tested, at least to some extent. Also, the well-functioning of the attribute itself is tested in the Clang patch.

Of course, it's possible that some unexpected thing is going to show up when I merge this in, as is always the case, but I don't easily see what tests I could add to reduce this risk at the moment.

I'd like to merge this early in the LLVM 8 release cycle to give us plenty of time to catch potential problems with this change. @EricWF, can you take a look?

LGTM other than the inline comments.

The chrome binary size goes from 810 MB to 756 MB after this patch.

libcxx/include/__config
802

We should use the reserved spelling __exclude_from_explicit_instantiation__.

EricWF accepted this revision.Oct 26 2018, 2:18 PM
This revision is now accepted and ready to land.Oct 26 2018, 2:18 PM
ldionne updated this revision to Diff 171369.Oct 26 2018, 4:02 PM

Use exclude_from_explicit_instantiation instead of non-reserved variant, per Eric's comment

ldionne marked an inline comment as done.Oct 26 2018, 4:03 PM

[...]

The chrome binary size goes from 810 MB to 756 MB after this patch.

This is great!

This revision was automatically updated to reflect the committed changes.