Clean up headers, fix symbol visibility changes

This changes the headers such that all the ImfForward-declared things
have ImfForward.h included first, such that ImfForward is truly ahead of
all of them, which allows us to tag the symbol visibility correctly and
avoid warnings. This also reduces header bloat throughout the library.

Further, this change documents a more proper path for controlling symbol
visibility. It switches things to using extern template instantiation
for the attribute types, which should lower code bloat overall. The
OpenEXR libraries are now compiled fully with hidden symbols by default
as well as hidden inlines by default per the generally recommended
behavior (-fvisibility=hidden -fvisibility-inlines-hidden via cmake).

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

cmake/LibraryDefine.cmake

@@ -10,7 +10,7 @@ function(OPENEXR_DEFINE_LIBRARY libname)
   cmake_parse_arguments(OPENEXR_CURLIB "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})
 
   if (MSVC)
-    set(_imath_extra_flags "/EHsc")
+    set(_openexr_extra_flags "/EHsc")
   endif()
   set(objlib ${libname})
   add_library(${objlib}
@@ -41,9 +41,10 @@ function(OPENEXR_DEFINE_LIBRARY libname)
     POSITION_INDEPENDENT_CODE ON
     C_VISIBILITY_PRESET hidden
     CXX_VISIBILITY_PRESET hidden
+    VISIBILITY_INLINES_HIDDEN ON
   )
-  if (_imath_extra_flags)
-    target_compile_options(${objlib} PUBLIC ${_imath_extra_flags})
+  if (_openexr_extra_flags)
+    target_compile_options(${objlib} PUBLIC ${_openexr_extra_flags})
   endif()
   set_property(TARGET ${objlib} PROPERTY PUBLIC_HEADER ${OPENEXR_CURLIB_HEADERS})
 

docs/SymbolVisibility.md

@@ -0,0 +1,103 @@
+# Symbol Visibility in OpenEXR
+
+## Overview
+
+Managing symbol visibility in a C++ library can reduce library sizes,
+and with the extra information, the optimizer may produce faster
+code. To take advantage of this, OpenEXR 3.0 is switching to
+explicitly manage symbols on all platforms, with a hidden-by-default
+behavior on unix-based platforms. Managing symbols has always been
+required for Windows DLLs, where one must explicitly tag functions for
+import and export as appropriate.
+
+For C, this is trivial: just tag public functions or global variable
+as default visibility and leave everything else defaulting to
+hidden. However, in C++, this is not exactly the same story. Functions
+and globals are of course the same. And class member functions are
+largely the same, and other than the attribute specification
+mechanics, follow the same rules between gcc, clang, and
+msvc. However, types have richer information than they do in C. So,
+unless extra work is done, concepts for RTTI like the typeinfo and the
+vtable for virtual classes will be hidden, and not visible. These are
+referred to as "vague" linkage objects in some discussions. 
+
+It is with the "vague" linkage objects where different properties
+arise. For example, if you have a template, it is happily instantiated
+in multiple compile units. If the typeinfo is hidden for one library,
+then this may cause things like dynamic_cast to fail because then the
+same typeinfo is not used, and even though one might think that
+ImfAttribute<Imath::Box2i> are the same in two places, because they
+are instantiated in separate places, they may be considered different
+types. To compound the issue, there are different rules for this in
+different implementations. For example, a default gcc under linux
+allows one to link against otherwise private "vague" linkage objects
+such that the typeinfo ends up as the same entity. clang, for MacOS
+anyway, follows a stricter approach and keeps those types separate,
+perhaps due to the two level namespace they maintain for symbols.
+
+Unfortunately, this is not clearly discussed as an overview of the
+differences between platforms, hence this document to add
+clarity. Each compiler / platform describes their own behavior, but
+not how that behaves relative to
+others. [libc++](https://libcxx.llvm.org/docs/DesignDocs/VisibilityMacros.html)
+from the llvm project is the closest to providing comparitive
+information, where by looking at how they define their macros and the
+comments surrounding, one can infer the behavior among at least
+windows DLL mode, then gcc vs. clang for unixen. Other compilers, for
+example, Intel's icc, tend to adopt the behavior of the predominant
+compiler for that platform (i.e. msvc under windows, gcc under linux),
+and so can generally adopt that behavior and are ignored here. If this
+is not true, the ifdef rules in the various library *Export.h headers
+within OpenEXR may need to be adjusted, and this table updated.
+
+As a summary, below is a table of the attribute or declspec that needs
+to be used for a particular C++ entity to be properly exported. This
+does not address weak symbols, ABI versioning, and only focusing on
+visibility. Under Windows DLL rules, if one exports the entire class,
+it also exports the types for the member types as well, which is not
+desired, so these are marked as N/A even though the compiler does
+allow that to happen.
+
+
+| C++ vs Compiler | MSVC                | mingw               | gcc                   | clang                      |
+|-----------------|---------------------|---------------------|-----------------------|----------------------------|
+| function        | dllexport/dllimport | dllexport/dllimport | visibility("default") | visibility("default")      |
+| hide a function | N/A                 | N/A                 | visibility("hidden")  | visibility("hidden")       |
+| class(typeinfo) | N/A                 | N/A                 | visibility("default") | visibility("default")      |
+| template class  | N/A                 | N/A                 | visibility("default") | type_visibility("default") |
+| template data   | N/A                 | N/A                 | visibility("default") | visibility("default")      |
+| class template<br>instantiation | dllexport/dllimport | N/A | N/A                   | N/A                        |
+| enum            | N/A                 | N/A                 | auto unhides (N/A)    | type_visibility("default") |
+| extern template | N/A                 | dllexport/dllimport | visibility("default") | visibility("default")      |
+
+With this matrix in mind, we can see the maximal set of macros we need to
+provide throughout the code. *NB*: This does not mean that we need to
+declare all of these, just that they might be needed. `XXX` should be
+substituted for the particular library name being compiled.
+
+| macro name                     | purpose                                                          |
+|--------------------------------|------------------------------------------------------------------|
+| `XXX_EXPORT`                   | one of export or import for windows, visibility for others       |
+| `XXX_EXPORT_TYPE`              | for declaring a class / struct as public (for typeinfo / vtable) |
+| `XXX_HIDDEN`                   | used to explicitly hide, especially members of types             |
+| `XXX_EXPORT_TEMPLATE_TYPE`     | stating the template type should be visible                      |
+| `XXX_EXPORT_EXTERN_TEMPLATE`   | exporting template types (i.e. extern side of extern template)   |
+| `XXX_EXPORT_TEMPLATE_INSTANCE` | exporting specific template instantiations (in cpp code)         |
+| `XXX_EXPORT_TEMPLATE_DATA`     | exporting templated data blocks                                  |
+| `XXX_EXPORT_ENUM`              | exporting enum types                                             |
+
+For a new library, the preference might be to call `XXX_EXPORT`
+something like `XXX_FUNC`, and rename things such as `XXX_EXPORT_TYPE`
+to `XXX_TYPE` for simplicity. However, historically, OpenEXR has used
+the `_EXPORT` tag, and so that is preserved for consistency.
+
+## References
+
+LLVM libc++ visibility macros:
+https://libcxx.llvm.org/docs/DesignDocs/VisibilityMacros.html
+
+GCC visibility wiki:
+https://gcc.gnu.org/wiki/Visibility
+
+Apple library design docs:
+https://developer.apple.com/library/archive/documentation/DeveloperTools/Conceptual/DynamicLibraries/100-Articles/DynamicLibraryDesignGuidelines.html

src/bin/exrheader/main.cpp

@@ -306,24 +306,24 @@ printInfo (const char fileName[])
             cout << i.name() << " (type " << a->typeName() << ")";
 
             if (const Box2iAttribute *ta =
-                            dynamic_cast_attr <Box2iAttribute> (a))
+                            dynamic_cast <const Box2iAttribute *> (a))
             {
                 cout << ": " << ta->value().min << " - " << ta->value().max;
             }
 
             else if (const Box2fAttribute *ta =
-                            dynamic_cast_attr <Box2fAttribute> (a))
+                            dynamic_cast <const Box2fAttribute *> (a))
             {
                 cout << ": " << ta->value().min << " - " << ta->value().max;
             }
             else if (const ChannelListAttribute *ta =
-                            dynamic_cast_attr <ChannelListAttribute> (a))
+                            dynamic_cast <const ChannelListAttribute *> (a))
             {
                 cout << ":";
                 printChannelList (ta->value());
             }
             else if (const ChromaticitiesAttribute *ta =
-                            dynamic_cast_attr <ChromaticitiesAttribute> (a))
+                            dynamic_cast <const ChromaticitiesAttribute *> (a))
             {
                 cout << ":\n"
                 "    red   " << ta->value().red << "\n"
@@ -332,34 +332,34 @@ printInfo (const char fileName[])
                 "    white " << ta->value().white;
             }
             else if (const CompressionAttribute *ta =
-                            dynamic_cast_attr <CompressionAttribute> (a))
+                            dynamic_cast <const CompressionAttribute *> (a))
             {
                 cout << ": ";
                 printCompression (ta->value());
             }
             else if (const DoubleAttribute *ta =
-                            dynamic_cast_attr <DoubleAttribute> (a))
+                            dynamic_cast <const DoubleAttribute *> (a))
             {
                 cout << ": " << ta->value();
             }
             else if (const EnvmapAttribute *ta =
-                            dynamic_cast_attr <EnvmapAttribute> (a))
+                            dynamic_cast <const EnvmapAttribute *> (a))
             {
                 cout << ": ";
                 printEnvmap (ta->value());
             }
             else if (const FloatAttribute *ta =
-                            dynamic_cast_attr <FloatAttribute> (a))
+                            dynamic_cast <const FloatAttribute *> (a))
             {
                 cout << ": " << ta->value();
             }
             else if (const IntAttribute *ta =
-                            dynamic_cast_attr <IntAttribute> (a))
+                            dynamic_cast <const IntAttribute *> (a))
             {
                 cout << ": " << ta->value();
             }
             else if (const KeyCodeAttribute *ta =
-                            dynamic_cast_attr <KeyCodeAttribute> (a))
+                            dynamic_cast <const KeyCodeAttribute *> (a))
             {
                 cout << ":\n"
                 "    film manufacturer code " <<
@@ -378,13 +378,13 @@ printInfo (const char fileName[])
                 ta->value().perfsPerCount();
             }
             else if (const LineOrderAttribute *ta =
-                            dynamic_cast_attr <LineOrderAttribute> (a))
+                            dynamic_cast <const LineOrderAttribute *> (a))
             {
                 cout << ": ";
                 printLineOrder (ta->value());
             }
             else if (const M33fAttribute *ta =
-                            dynamic_cast_attr <M33fAttribute> (a))
+                            dynamic_cast <const M33fAttribute *> (a))
             {
                 cout << ":\n"
                 "   (" <<
@@ -399,7 +399,7 @@ printInfo (const char fileName[])
                 ta->value()[2][2] << ")";
             }
             else if (const M44fAttribute *ta =
-                            dynamic_cast_attr <M44fAttribute> (a))
+                            dynamic_cast <const M44fAttribute *> (a))
             {
                 cout << ":\n"
                 "   (" <<
@@ -421,19 +421,19 @@ printInfo (const char fileName[])
                 ta->value()[3][3] << ")";
             }
             else if (const PreviewImageAttribute *ta =
-                            dynamic_cast_attr <PreviewImageAttribute> (a))
+                            dynamic_cast <const PreviewImageAttribute *> (a))
             {
                 cout << ": " <<
                 ta->value().width()  << " by " <<
                 ta->value().height() << " pixels";
             }
             else if (const StringAttribute *ta =
-                            dynamic_cast_attr <StringAttribute> (a))
+                            dynamic_cast <const StringAttribute *> (a))
             {
                 cout << ": \"" << ta->value() << "\"";
             }
             else if (const StringVectorAttribute * ta =
-                            dynamic_cast_attr <StringVectorAttribute> (a))
+                            dynamic_cast <const StringVectorAttribute *> (a))
             {
                 cout << ":";
 
@@ -445,13 +445,13 @@ printInfo (const char fileName[])
                 }
             }
             else if (const RationalAttribute *ta =
-                            dynamic_cast_attr <RationalAttribute> (a))
+                            dynamic_cast <const RationalAttribute *> (a))
             {
                 cout << ": " << ta->value().n << "/" << ta->value().d <<
                 " (" << double (ta->value()) << ")";
             }
             else if (const TileDescriptionAttribute *ta =
-                            dynamic_cast_attr <TileDescriptionAttribute> (a))
+                            dynamic_cast <const TileDescriptionAttribute *> (a))
             {
                 cout << ":\n    ";
 
@@ -468,28 +468,28 @@ printInfo (const char fileName[])
                 }
             }
             else if (const TimeCodeAttribute *ta =
-                            dynamic_cast_attr <TimeCodeAttribute> (a))
+                            dynamic_cast <const TimeCodeAttribute *> (a))
             {
                 cout << ":\n";
                 printTimeCode (ta->value());
             }
             else if (const V2iAttribute *ta =
-                            dynamic_cast_attr <V2iAttribute> (a))
+                            dynamic_cast <const V2iAttribute *> (a))
             {
                 cout << ": " << ta->value();
             }
             else if (const V2fAttribute *ta =
-                            dynamic_cast_attr <V2fAttribute> (a))
+                            dynamic_cast <const V2fAttribute *> (a))
             {
                 cout << ": " << ta->value();
             }
             else if (const V3iAttribute *ta =
-                            dynamic_cast_attr <V3iAttribute> (a))
+                            dynamic_cast <const V3iAttribute *> (a))
             {
                 cout << ": " << ta->value();
             }
             else if (const V3fAttribute *ta =
-                            dynamic_cast_attr <V3fAttribute> (a))
+                            dynamic_cast <const V3fAttribute *> (a))
             {
                 cout << ": " << ta->value();
             }

src/bin/exrmaketiled/main.cpp

@@ -13,6 +13,8 @@
 
 #include "makeTiled.h"
 
+#include <ImfHeader.h>
+
 #include <iostream>
 #include <exception>
 #include <string>

src/lib/Iex/IexExport.h

@@ -8,39 +8,11 @@
 
 /// \defgroup ExportMacros Macros to manage symbol visibility
 ///
-/// In order to produce tidy symbol tables in shared objects, one must
-/// manage symbol visibility. This is required under Windows for DLLs,
-/// and has been well documented in terms of export / import
-/// swapping. However, under Unixen or other similar platforms, there
-/// is a different mechanism of specifying the visibility. So to
-/// achieve nearly the same results, without requiring every single
-/// function be tagged, one can tell the compiler to mark all symbols
-/// as hidden by default, then only export the specific symbols in
-/// question.
-///
-/// However, this is not so easy for C++. There are what are called
-/// 'vague' linkage objects, which are often the typeinfo / vtable
-/// type objects, although templates and a few other items fall into
-/// this category. In order to enable dynamic_cast and similar
-/// behavior, we must export the typeinfo and such. This is done at
-/// the class level, but if we were to do this under Windows, then the
-/// class member data types also end up being exported, which leaks
-/// STL details. So, to differentiate this, we use
-/// IMF_EXPORT_TYPE at the class level, and continue to use
-/// IMF_EXPORT on the public function symbols. Unfortunately, by
-/// putting the export at the class level, that means that private
-/// functions are then exported as well. Hence, we must force any of
-/// these back to local using IMF_EXPORT_LOCAL. To avoid pollution of
-/// the code, we should only apply this to classes which are used for
-/// dynamic_cast, or otherwise require it.
-///
-/// There may be more needed than have been tagged so far, if you
-/// start receiving link errors about typeinfo, please look if it is
-/// the symbol exports, and whether additional tagging is
-/// needed. Having a goal to hide symbols should increase symbol
-/// loading / resolution performance in aggregate, so is a desired end
-/// goal.
+/// See docs/SymbolVisibility.md for more discussion
 ///
+/// Iex is simple and does not need to do more than expose class types
+/// and functions, and does not have any private members to hide
+/// 
 /// @{
 #if defined(OPENEXR_DLL)
 #    if defined(IEX_EXPORTS)
@@ -49,16 +21,13 @@
 #        define IEX_EXPORT __declspec(dllimport)
 #    endif
 #    define IEX_EXPORT_TYPE
-#    define IEX_EXPORT_LOCAL
 #else
 #    ifndef _MSC_VER
-#        define IEX_EXPORT __attribute__ ((visibility ("default")))
-#        define IEX_EXPORT_TYPE __attribute__ ((visibility ("default")))
-#        define IEX_EXPORT_LOCAL __attribute__ ((visibility ("hidden")))
+#        define IEX_EXPORT __attribute__ ((__visibility__ ("default")))
+#        define IEX_EXPORT_TYPE __attribute__ ((__visibility__ ("default")))
 #    else
 #        define IEX_EXPORT
 #        define IEX_EXPORT_TYPE
-#        define IEX_EXPORT_LOCAL
 #    endif
 #endif
 /// @}