Make the default symbol visibility hidden for unixen builds

[kdt3rd]

This only applies when building shared libraries, but switches to a
symbol management strategy which is more in line with Windows, where
symbols must be explicitly exported (marked as default visibilty). This
allows us to be much more explicit and careful about what symbols are
being exposed, making our .so files tidier

Signed-off-by: Kimball Thurston kdt3rd@gmail.com

[lgritz]

I think any method fully defined inside a struct or class declaration is implicitly inline.

[meshula]

yes

[lgritz]

There are some failures on the Mac CI test. Not sure if it's related to this PR.

[meshula]

A few years ago we painstakingly went from this form to the individual export form, because classes contained things, such as std::string that are not adorned for export. This led to subtle corruption style bugs when using OpenEXR as a DLL. One of the root causes of the nightmare was that BaseExc was sublcassed from std::string. I'm extremely nervous about undoing this change! Are any warnings about "some members are not decorated for export" emitted during compilation? One of the many symptoms was that exception handling never worked on Windows; OpenEXR exceptions segfaulted.

[meshula]

Another issue that resulted was that symbol tables become ungodly huge, base exporting the entire class exports way more than you would expect, including symbols for any std objects within the class. Perhaps Microsoft has refined things and it's no longer the can of worms that it was? ... nervous,,,, it took a long time to figure out why Windows was so unstable and correct it....

[lgritz]

Well, you should never believe me about Windows things, but I'm quite certain that both OIIO and OSL use this method of tagging the whole class as exportable, and not the individual methods therein, and I have not had any reported problems. Maybe this was an issue that was fixed in MSVS long ago?

[kdt3rd]

no warnings I've seen on any of the platform compiles about things not adorned for export on any of the CI builds, but I think the failure on the mac is due to me missing something about that for that platform - exceptions are notoriously difficult in this realm, so I think one / some of the Imf exceptions are not being exported correctly.

[meshula]

We recently did a pass on USD to do it the pain-staking member-by-member way, rather than class by class, because we ran into, in particular, that since we have vector, vector was being re-exported as somehow being owned by USD libs. The root problem boils down to STL classes being re-exported by client libraries, and the fact that historically the Microsoft STL relied on static member initialization which breaks down fiendishly in a DLL based world. A quick scan of https://github.com/microsoft/STL suggests that they no longer rely on static initializers. Still, there may be a symbol table explosion due to undesired STL exports that the painstaking method avoids. At ILM, the historic motivation to do it the nasty way was that between boost and the STL the Zeno symbol table became too huge and we had to do fairly frequent refactorings to get templated symbols below the MSVC crash threshold.

All this is to say that I'm inclined to the simpler suggestion you're proposing, if the symbol table sizes don't increase by integer multiples of the current size, and if exceptions thrown under Windows don't go back to segfaulting.

[meshula]

Setting the visibility preset to hidden is necessary for the export attributes to mean anything under gcc/clang, but I want to point out that the existing set up of exporting individual members as we did is compatible with that approach, and should work in and of itself without changing the export mechanism on all the classes.

[kdt3rd]

yes, my original goal was to just change to turn on the hidden flag (and change the export defines, similar to what USD does) to avoid symbol leakage - we've had some issues with this recently in other libraries, but seemed an appropriate opportunity to become tidier about what symbols we expose to the world.

However, once I set the default visibility to hidden, I was having issues with vtables not being exported consistently, even though the destructor was exported - I think something is/was off with default provided functions (i.e. rule of 3/5) and visibility. I do see what you mean by it exporting contained types as weak dynamic symbols, I did not expect that.

I will explore more why I was having issues with the vtables, to be able to clean that up, and maybe take a different stab at this.

[meshula]

The problem is that gcc doesn't export the vtable. There's a proposal in the USD Issues which contains a lot more analysis of the horrible state of shared libraries in general. It also has me relating some of the OpenEXR experience...

PixarAnimationStudios/OpenUSD#665

#if HAVE_GCC_OR_CLANG
#define FOO_EXPORT_API the visibility thing
#else
#define FOO_EXPORT_API
#endif

class FOO_EXPORT_API Foo
{
     int foo(); // not exported
     FOO_EXPORT int bar(); // exported
};

In general, this stuff is a world of hurt. More "fascinating" investigation here, wrt pybind11: pybind/pybind11#2132

[kdt3rd]

After reading the USD issue and

https://gcc.gnu.org/onlinedocs/gcc/Vague-Linkage.html

I think it makes the most sense to follow the basic guidelines outlined in the USD issue, and have implemented similar constructs here. I also put a brief doxygen description of the issue in IexExports.h, and put all the other export headers into the same export macros group

[kdt3rd]

hrm, I am not sure what github has done in running the CI, it seems to have merged in master because it detected a conflict. Warning to be careful during merge...

[meshula]

@kdt3rd This is looking good. Seeing your solution withe "vague" linkage, reminds me of a pattern I used in some code at ILM where something similar came up ago in aeons past --- I used, equivalently to IEX_EXPORT_VAGUELINKAGE, IEX_EXPORT_CLASS, to capture that it was an export macro, and where it was intended to be used. I wonder if using CLASS might help down the line in years to come, if we are thinking about where VAGUELINKAGE might come into play, and if it's safe to use it in other contexts. EXPORT_CLASS would preemptively answer that hypothetical question.

[kdt3rd]

@meshula - so you'll notice the CI is failing under Mac. It is unfortunately at runtime, and basically, the types of the "known" attribute types are being hidden inside the dylibs, and the dynamic RTTI comparison is failing when attributes are being set into the exr header because they are created in a separate module, and so result in a separate type. Where g++ does something different. This is the best explanation of what is happening I've been able to find:

https://stackoverflow.com/questions/19496643/using-clang-fvisibility-hidden-and-typeinfo-and-type-erasure

So I think there is going to have to be a multi-stage response to this in order to get to the full solution. The first I'll put in will be to use something similar to the dynamic cast work around suggested in the above. This can be encapsulated in the (existing) cast function of TypedAttribute. This ends up comparing the names after a few other checks (which is apparently what libstdc++ does, which I agree w/ the llvm/libc++ guys, is a bit dodgy, but hey, represents some definition of truth). Then, once this change is propagated to Imath as well, where we can tag the template classes with an export properly, then we can expose the other types inside OpenEXR that are put into TypedAttribute, but passed in from external (i.e. TypedAttributeImath::vec2i and you have to even expose the enums such as enum EnvMap). Then it will all work under libc++ (it's not really an issue w/ clang, clang under linux works just fine without libc++) - I've been able to make incremental changes, and move the problem. Which is making me re-examine what's really wrong.

I should note that this problem is contained and not unbounded, in that we are dynamic casting the attributes to the type we think we know (but are actually ending up as different typeid values, which is pedantically correct). A similar change will be needed in OpenEXRUtil for the image / channel sub types, but that is self-contained there as well.

I do agree that EXPORT_VAGUELINKAGE is a bit ... vague? but it gives an obvious search term to start the journey of discovery. However, I agree that something like an EXPORT_TYPE might be better (or EXPORT_CLASS, but given I'll need to add this eventually to the enums, thought TYPE was clearer). So I think I'll switch it to TYPE.

[cary-ilm]

These #defines don't exactly mirror the IEX ones, is that intentional? There's no OPENEXR_USE_DEFAULT_VISIBILITY here. Should the two libraries follow the same structure?

[cary-ilm]

indentation?

[kdt3rd]

yeah, the old files somehow had tabs in the spaces (my editor is spaces only)

[cary-ilm]

Just curious, if the default is hidden, why do we have to mark this explicitly as HIDDEN?

[kdt3rd]

the outer class has been marked as public (default) such that it's typeinfo is exported, so I am forcing this one back to hidden. It may not be necessary on all platforms, but seemed safer.

[cary-ilm]

Nice explanation!

[cary-ilm]

This idiom occurs for every attribute type, it's worth explaining exactly what's going on here. Why do you need to avoid these declarations in the .cpp?

[cary-ilm]

Remove commented-out lines.

[cary-ilm]

Remove commented-out lines.

[cary-ilm]

Per earlier comments, is there any reason to not just use OPENEXR_EXPORTS? Does OpenEXRUtil really need its own distinct macros?

[cary-ilm]

I've looked it over and it seems reasonable, although I hope there's a way of consolidating the various sets of export macros, for Imath, Iex, IlmThread, OpenEXR, since they're all doing the same basic thing but with slight variations. Are the variations intentional, or oversights, or misinterpretations on my part?