This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Make sure we can build libc++ with -fvisibility=hidden
ClosedPublic

Authored by ldionne on Sep 28 2018, 10:40 AM.

Details

Summary

When building with -fvisibility=hidden, some symbols do not get exported from
libc++.dylib. This means that some entities are not explicitly given default
visibility in the source code, and that we rely on the fact -fvisibility=default
is the default. This commit explicitly gives default visibility to those
symbols to avoid being dependent on the command line flags used.

Also, in the future, we may want to mark the whole std:: namespace as having
hidden visibility (to switch from opt-out to opt-in), in which case the
changes done in this commit will be required.

Event Timeline

ldionne created this revision.Sep 28 2018, 10:40 AM

It's awesome that you're looking into hidden visibility!

I'm guessing you figured out the missing symbols by running check-cxx or trying to link some applications or similar?

It's awesome that you're looking into hidden visibility!

The gray hair I'm growing begs to disagree :-).

I'm guessing you figured out the missing symbols by running check-cxx or trying to link some applications or similar?

I added -fvisibility=hidden to the build flags, built libc++.dylib and ran check-cxx-abilist. There were several missing symbols, and I added the annotations in this commit until there were no missing symbols anymore.

It's awesome that you're looking into hidden visibility!

The gray hair I'm growing begs to disagree :-).

Hah, I'm sure I've lost a good amount of hair to visibility issues as well :)

I'm guessing you figured out the missing symbols by running check-cxx or trying to link some applications or similar?

I added -fvisibility=hidden to the build flags, built libc++.dylib and ran check-cxx-abilist. There were several missing symbols, and I added the annotations in this commit until there were no missing symbols anymore.

That makes sense. Back when I was looking into this (which is over a year ago now, and sadly I got pulled away into other things before I had a chance to upstream all of it), I found that only some of the missing exports when building with hidden visibility were actually required for check-cxx to pass. The rest could be added if you want to preserve ABI compatibility for v1, but hidden visibility very much seems like an ABI v2 thing, so at that point you could also fix the over-exporting.

Internally, we've been building libc++ with hidden visibility for a long time now, and it's been working well. I'm linking some of the issues from when I was working on that; I've lost all context on these by now, but I figured it might save you some trouble if you ran into similar issues:

https://bugs.llvm.org/show_bug.cgi?id=32114
https://bugs.llvm.org/show_bug.cgi?id=34614
https://github.com/smeenai/bad-visibility-finder

That makes sense. Back when I was looking into this (which is over a year ago now, and sadly I got pulled away into other things before I had a chance to upstream all of it), I found that only some of the missing exports when building with hidden visibility were actually required for check-cxx to pass. The rest could be added if you want to preserve ABI compatibility for v1, but hidden visibility very much seems like an ABI v2 thing, so at that point you could also fix the over-exporting.

I think this is applicable for ABI v1: those symbols are currently exported by the ABI, and it is just a coincidence that we export them from the dylib.

Internally, we've been building libc++ with hidden visibility for a long time now, and it's been working well. I'm linking some of the issues from when I was working on that; I've lost all context on these by now, but I figured it might save you some trouble if you ran into similar issues:

Why have you been building libc++ with hidden visibility? What is the advantage of doing that?

https://bugs.llvm.org/show_bug.cgi?id=32114
https://bugs.llvm.org/show_bug.cgi?id=34614
https://github.com/smeenai/bad-visibility-finder

Thanks for the links!

That makes sense. Back when I was looking into this (which is over a year ago now, and sadly I got pulled away into other things before I had a chance to upstream all of it), I found that only some of the missing exports when building with hidden visibility were actually required for check-cxx to pass. The rest could be added if you want to preserve ABI compatibility for v1, but hidden visibility very much seems like an ABI v2 thing, so at that point you could also fix the over-exporting.

I think this is applicable for ABI v1: those symbols are currently exported by the ABI, and it is just a coincidence that we export them from the dylib.

Fair point. I think it's worth distinguishing between symbols that we actually need to export (for check-cxx or applications or similar) and symbols that just happen to be exported because of the default visibility but don't actually need to be exported. If you're already doing that in this patch, or if there aren't actually any symbols in the first category anymore, then just ignore me :)

Internally, we've been building libc++ with hidden visibility for a long time now, and it's been working well. I'm linking some of the issues from when I was working on that; I've lost all context on these by now, but I figured it might save you some trouble if you ran into similar issues:

Why have you been building libc++ with hidden visibility? What is the advantage of doing that?

For libc++, there's not too much advantage, since (a) it already controls its ABI pretty thoroughly, and (b) it already builds with -fvisibility-inlines-hidden, which takes care of a lot of cases. We did find a few symbols that were over-exported, but not enough to make any significant reductions in dynamic symbol table size. Mostly it was a cleanliness thing, where we wanted all our libraries having controlled ABIs and building with hidden visibility. It also helped us with cross-platform compatibility, since Windows always has the equivalent of hidden visibility and you have to explicitly declare your exports (which is a much nicer default IMO).

Fair point. I think it's worth distinguishing between symbols that we actually need to export (for check-cxx or applications or similar) and symbols that just happen to be exported because of the default visibility but don't actually need to be exported. If you're already doing that in this patch, or if there aren't actually any symbols in the first category anymore, then just ignore me :)

I'm not sure how we can ever check that a symbol is exported but not actually needed. Wouldn't this require analyzing the set of all programs linking to libc++.dylib in the wild?

Internally, we've been building libc++ with hidden visibility for a long time now, and it's been working well. I'm linking some of the issues from when I was working on that; I've lost all context on these by now, but I figured it might save you some trouble if you ran into similar issues:

Why have you been building libc++ with hidden visibility? What is the advantage of doing that?

For libc++, there's not too much advantage, since (a) it already controls its ABI pretty thoroughly, and (b) it already builds with -fvisibility-inlines-hidden, which takes care of a lot of cases. We did find a few symbols that were over-exported, but not enough to make any significant reductions in dynamic symbol table size. Mostly it was a cleanliness thing, where we wanted all our libraries having controlled ABIs and building with hidden visibility. It also helped us with cross-platform compatibility, since Windows always has the equivalent of hidden visibility and you have to explicitly declare your exports (which is a much nicer default IMO).

Got it. I agree that Windows is much saner in that respect. In the future, we could actually apply hidden visibility to all of namespace std and then only cherry-pick entities that we want to export (and apply a default-visibility attribute to those). However, that requires the ability to explicitly instantiate vtables and RTTI, which we don't have right now (but we could add as an extension to Clang). This would allow us to drop almost all of the visibility annotations we currently have in libc++.

Fair point. I think it's worth distinguishing between symbols that we actually need to export (for check-cxx or applications or similar) and symbols that just happen to be exported because of the default visibility but don't actually need to be exported. If you're already doing that in this patch, or if there aren't actually any symbols in the first category anymore, then just ignore me :)

I'm not sure how we can ever check that a symbol is exported but not actually needed. Wouldn't this require analyzing the set of all programs linking to libc++.dylib in the wild?

Sorry, you're right. Internally I'd based my analysis on the set of symbols needed by check-clang + the set of symbols needed by all the binaries we were building (which is a pretty extensive test case), but of course that wouldn't cover everyone's use cases. There were some symbols which seemed fairly obviously internal/intended to be hidden, but since they've been part of the ABI it's entirely possible someone is using them anyway.

Internally, we've been building libc++ with hidden visibility for a long time now, and it's been working well. I'm linking some of the issues from when I was working on that; I've lost all context on these by now, but I figured it might save you some trouble if you ran into similar issues:

Why have you been building libc++ with hidden visibility? What is the advantage of doing that?

For libc++, there's not too much advantage, since (a) it already controls its ABI pretty thoroughly, and (b) it already builds with -fvisibility-inlines-hidden, which takes care of a lot of cases. We did find a few symbols that were over-exported, but not enough to make any significant reductions in dynamic symbol table size. Mostly it was a cleanliness thing, where we wanted all our libraries having controlled ABIs and building with hidden visibility. It also helped us with cross-platform compatibility, since Windows always has the equivalent of hidden visibility and you have to explicitly declare your exports (which is a much nicer default IMO).

Got it. I agree that Windows is much saner in that respect. In the future, we could actually apply hidden visibility to all of namespace std and then only cherry-pick entities that we want to export (and apply a default-visibility attribute to those). However, that requires the ability to explicitly instantiate vtables and RTTI, which we don't have right now (but we could add as an extension to Clang). This would allow us to drop almost all of the visibility annotations we currently have in libc++.

Hmm, why do we need to be able to explicitly instantiate vtables and RTTI ... doesn't the implied emission of those in the object file containing the key function work? I've been aware of a few -dev threads flying back and forth about various visibility issues for libc++, and I admit I haven't been keeping up with them fully, so I apologize if this has already been covered in one of those.

Hmm, why do we need to be able to explicitly instantiate vtables and RTTI ... doesn't the implied emission of those in the object file containing the key function work? I've been aware of a few -dev threads flying back and forth about various visibility issues for libc++, and I admit I haven't been keeping up with them fully, so I apologize if this has already been covered in one of those.

We'd need to give default visibility to the vtable and the RTTI, which requires using an attribute.

Hmm, why do we need to be able to explicitly instantiate vtables and RTTI ... doesn't the implied emission of those in the object file containing the key function work? I've been aware of a few -dev threads flying back and forth about various visibility issues for libc++, and I admit I haven't been keeping up with them fully, so I apologize if this has already been covered in one of those.

We'd need to give default visibility to the vtable and the RTTI, which requires using an attribute.

Isn't that what __attribute__((type_visibility("default"))) accomplishes?

Hmm, why do we need to be able to explicitly instantiate vtables and RTTI ... doesn't the implied emission of those in the object file containing the key function work? I've been aware of a few -dev threads flying back and forth about various visibility issues for libc++, and I admit I haven't been keeping up with them fully, so I apologize if this has already been covered in one of those.

We'd need to give default visibility to the vtable and the RTTI, which requires using an attribute.

Isn't that what __attribute__((type_visibility("default"))) accomplishes?

The problem is that it requires applying the attribute to the class template itself as opposed to a single instantiation of it. Unless I am wrong?

Sent from iphone

Hmm, why do we need to be able to explicitly instantiate vtables and RTTI ... doesn't the implied emission of those in the object file containing the key function work? I've been aware of a few -dev threads flying back and forth about various visibility issues for libc++, and I admit I haven't been keeping up with them fully, so I apologize if this has already been covered in one of those.

We'd need to give default visibility to the vtable and the RTTI, which requires using an attribute.

Isn't that what __attribute__((type_visibility("default"))) accomplishes?

The problem is that it requires applying the attribute to the class template itself as opposed to a single instantiation of it. Unless I am wrong?

Thinking about this more, I guess you can't really have a key function for a class template anyway, since you need to define all your methods inline for clients to be able to use your class.

We'd need to give default visibility to the vtable and the RTTI, which requires using an attribute.

Isn't that what __attribute__((type_visibility("default"))) accomplishes?

The problem is that it requires applying the attribute to the class template itself as opposed to a single instantiation of it. Unless I am wrong?

Thinking about this more, I guess you can't really have a key function for a class template anyway, since you need to define all your methods inline for clients to be able to use your class.

You're right. I guess we _might_ be able to come up with a twisted way to achieve this with type_visibility, for example: declare (but not define) the key function in the template, then define it out-of-line, but add an explicit instantiation declaration of the key function for the specific instantiation of the class template we're interested in. And then apply the type_visibility("default") attribute to an explicit instantiation declaration of the instantiation of the class template we're interested in. IDK whether this would work, and I don't care. It's messy and too complicated, and this area of the language is already crazy enough.

We'd need to give default visibility to the vtable and the RTTI, which requires using an attribute.

Isn't that what __attribute__((type_visibility("default"))) accomplishes?

The problem is that it requires applying the attribute to the class template itself as opposed to a single instantiation of it. Unless I am wrong?

Thinking about this more, I guess you can't really have a key function for a class template anyway, since you need to define all your methods inline for clients to be able to use your class.

You're right. I guess we _might_ be able to come up with a twisted way to achieve this with type_visibility, for example: declare (but not define) the key function in the template, then define it out-of-line, but add an explicit instantiation declaration of the key function for the specific instantiation of the class template we're interested in. And then apply the type_visibility("default") attribute to an explicit instantiation declaration of the instantiation of the class template we're interested in. IDK whether this would work, and I don't care. It's messy and too complicated, and this area of the language is already crazy enough.

Yeah, having a direct way to instantiate the vague linkage types does seem like a much better solution :)

EricWF requested changes to this revision.Oct 12 2018, 9:45 PM

If we're adding a new "Visibility" macro, then the VisibilityMacros.rst doc should be updated to include it.

Otherwise, I don't see any other problems in this patch. I'll leave it to other reviewers.

libcxx/include/__debug
77

Is this the only usage of _LIBCPP_EXTERN_VIS?

If so, please go ahead and purge all definitions of the macro as well as it's documentation.

This revision now requires changes to proceed.Oct 12 2018, 9:45 PM
ldionne marked an inline comment as done.Oct 14 2018, 11:41 AM

If we're adding a new "Visibility" macro, then the VisibilityMacros.rst doc should be updated to include it.

Otherwise, I don't see any other problems in this patch. I'll leave it to other reviewers.

Done.

libcxx/include/__debug
77

Good catch, thanks!

ldionne marked an inline comment as done.Oct 15 2018, 9:29 AM
ldionne updated this revision to Diff 169722.Oct 15 2018, 10:08 AM

Purge _LIBCPP_EXTERN_VIS

EricWF added inline comments.Oct 18 2018, 12:25 PM
libcxx/include/locale
2426

Undef the macro after we're done using it.

libcxx/include/thread
170

So we likely only got this symbol in the dylib as a result of an implicit instantiation. Changing this to an explicit instantiation might be ABI breaking.

libcxx/src/iostream.cpp
80

Does this actually need to be exported to serve it's function of initializing the streams?

ldionne updated this revision to Diff 170126.Oct 18 2018, 2:35 PM
ldionne marked 3 inline comments as done.

Implement Eric's feedback

libcxx/include/thread
170

We agreed we could just remove the symbol. I removed the instantiation, marked the function as hidden and documented that in the ABI list CHANGELOG.

libcxx/src/iostream.cpp
80

I don't think so -- I marked it as hidden.

@EricWF I think all of your concerns with this patch have been addressed -- do you see anything else or is this good to go?

EricWF accepted this revision.Oct 24 2018, 3:39 PM

Yeah, this LGTM. Let's see if we break anyone!

This revision is now accepted and ready to land.Oct 24 2018, 3:39 PM
This revision was automatically updated to reflect the committed changes.