Building with -fvisibility=hidden fails on linking

[sopvop]

Description of Issue

If you try to build USD with gcc or clang with -fvisility=hidden it will fail when linking.

Steps to Reproduce

$ cmake -DCMAKE_CXX_VISIBILITY_PRESET=hidden ..
$ make
....
undefined reference to `typeinfo for pxrInternal_v0_18__pxrReserved__::somethingSomething

Why is this important.

Importance of builiding with hidden visibility is well described in GCC Wiki here.

tldr: It allows compiler and linker to do more optimizations, makes startup faster and binaries smaller.

Unlike MSVC, gcc does not export typeinfo and vtable of class by default. MSVC exports it if any member has attribute __declspec(dllexport) but gcc (and clang IIRC) require class to be declared with visibility attribute:

class __attribute__ ((visibility ("default"))) Foo {};
//or in pxr sources
class SOME_API Foo {};

In USD sources FOO_API macro expand to __attribute__ ((visibility ("default"))) on gcc or __declspec(dllexport) on windows, and those have different semantics when applied to types.
GCC will only export necessary rtti machinery (vtable, typeinfo and the like), but msvc will export all members as if all of them have __declspec(dllexport) attribute.

Fix

I propose to add FOO_CLASS_API or similarly named macro to api.h headers which will expand to __attribute__ ((visibility ("default"))) on gcc/clang and to nothing on windows. And add it to codegen templates and all public classes.

[c64kernal]

Hi @sopvop, thanks so much for the feedback. Building with hidden visibility is definitely something we'd like to do. Please give us a little time to review this proposal and get back to you. (For example, one thing that jumps out at me right away is that, while we try really hard not to, we sometimes use FOO_API to export whole classes on Windows (e.g., in the Houdini plugin code) -- so I'd be concerned about having to explain when to use FOO_CLASS_API and when to use FOO_API when exporting whole types).

[jtran56]

Filed as internal issue #USD-4842.

[c64kernal]

Hi again @sopvop,

Thanks so much for your patience with this, we had a chance to vet your proposal and overall we like it a lot. We have a few suggestions to run by you as well. Here's our current proposal (just small tweaks over your proposal, with a naming suggestion as well):

1. Add a new macro FOO_API_VAGUE that defines to __attribute__ ((visibility ("default"))) on gcc/clang and nothing on Windows.

2. Only use FOO_API_VAGUE on classes that require exports of vague objects on gcc/clang (in other words, not on all public classes, only those requiring vague objects to be exported).

3. For those classes, all methods must be either marked FOO_API or FOO_LOCAL.

4. Avoid FOO_API on classes, unless there is a clear and justified reason for it.

5. As part of the PR, we'd also love it if the entire repo was modified and tested to build with hidden visibility from now on as well.

For 1, this is exactly your proposal and we're additionally suggesting the use of the term VAGUE since we'd like for this macro to represent only exporting vague objects. Unfortunately, we've verified that on gcc (at least on our version of it, 4.8.x) adding default visibility to the class exports everything, not just the vague objects. Despite that, we still think it's a good idea to call it FOO_API_VAGUE since we can later define it to export only vague objects for compilers that already support this, or as compilers add support for it.

One of the things we're concerned about is that we don't know how much of the code needs to be tagged with FOO_API_VAGUE. We haven't done the experiment. If it turns out that a majority of classes need it, then we might want to reevaluate the proposal. Our gut feeling is that most classes will be okay with the current export policies that we've already tagged (which was initially done for MSVC). In other words, we think that most classes will not need to be tagged with FOO_API_VAGUE. Hopefully that holds up, but if it doesn't, please let us know.

The fact that gcc doesn't limit the symbols in this case is what leads us to 3. In order to make sure that we export as few symbols as possible, we're asking that any class tagged with FOO_API_VAGUE should have every symbol tagged with either FOO_API or FOO_LOCAL to express which symbols should and shouldn't be exported. (Note FOO_LOCAL for MSVC is a no-op, just like FOO_API_VAGUE, so the result on MSVC should be identical to current standard practices). This additional requirement of tagging everything is what makes us like to minimize the amount of places where FOO_API_VAGUE appears (item 2).

For 4, it's currently against our coding practices to tag FOO_API on classes. We've made a few exceptions where a reasonable argument was made, but in general this should still hold.

Finally, for 5, we'd actually love to have hidden visibility turned on by default everywhere -- once this work is done and we merge the PR, we'll apply it to our internal build system as well (which is a bit different) to make sure that we keep hidden visibility working by default.

Thank you so much again for the great suggestion, I think this will be a great addition.

[sopvop]

Unfortunately, we've verified that on gcc (at least on our version of it, 4.8.x) adding default visibility to the class exports everything, not just the vague objects.

This is true for gcc 6 as well, so I assume that's just how gcc works.

One of the things we're concerned about is that we don't know how much of the code needs to be tagged with FOO_API_VAGUE.

If we do the minimal changes required to build USD, then only classes exported to python via boost which have base classes also exported to boost need to be marked, since that feature uses dynamic_cast.

If usage of USD required usage of dynamic_cast, or exceptions, then those classes would also have to be marked. Though schema classes have their own type of RTTI and dynamic_cast only used in hydra renderer (I haven't researched why though), the usage of boost python would still require classes to be tagged. But otherwise I believe changes will be minimal.

The easiest first change, and the one which won't break any client code, is compiling python modules with "hidden" default visibility. In some cases it reduces size of the binary by more than half.

Building other libraries with hidden default visibility may break client code which depend on RTTI (and maybe ODR) and should be configurable via option.

I'll try to make PR this week.

[sopvop]

After a lot of research on how to do this right, and without too much meaningless work, I came to the conclusion that adding export declarations to class methods is not the best way.

Best way would be to only add FOO_API to a class only, and let all members be exported. This is how big c++ projects do it (like Qt), if they ever bother with visibility at all.

I see no reason to not export class method, if you export class. The benefits are tiny, and this selective export only works on windows, anyway.

My proposal for this is just add FOO_API to all exported classes, and remove it from all methods.

[dalboris]

Hi @sopvop , ex-Pixar intern here. For what it's worth, I came across this thread while trying to evaluate whether I should also in my own projects export whole classes or do the same as what USD was doing (only export public methods). I stumbled upon the following issue: pybind/pybind11#2132

I'm really very far from being an expert in this stuff, but I just wanted to note that Qt extensively uses the pimpl idiom, and does not use STL classes (e.g., std::string, std::vector, etc.), and therefore exporting whole classes is perhaps less of a concern in their cases (it's not going to bloat their dynamic tables with thousands of template classes). Exporting STL classes, especially in Windows DLLs, seems a little frowned upon according to my research.

That being said, maybe your conclusion is still the most sensible one, and exporting whole classes makes sense for USD too (and most libraries), not just Qt. Especially when writing Python bindings is important. I'm still a bit unclear about all this and just sharing my thoughts, in case it's useful. Sorry for the noise.

[meshula]

Exporting STL classes fails in subtle ways in Microsoft's libraries whenever there is shared state across classes. Things will often appear to work correctly, but subtle bugs occur when two different DLLs access the same supposedly shared state, but actually each DLL has its own copy, initialized independently. A second problem with exporting whole classes, is that when templates are involved, as @dalboris notes, symbol tables can become so large that linking fails.

We started the OpenEXR project with whole class exports. OpenEXR has a much simpler class structure than USD, and over time, subtle Windows specific issues piled up, including severe problems such as stack corruptions, until we finally bit the bullet and explicitly exported the API, as USD does. OpenEXR's cross platform behavior is now correct, and the Windows specific bugs resulting from class exports have all been resolved.

[dalboris]

Thanks @meshula for the added details and historical notes, very instructive and appreciated.