Export Data3DPointsData_t class in shared library.

[marxin]

We depend on the class in our code, hope it's fine to export it.

[asmaloney]

This change removes the whole reason for having explicit template instantiation in the cpp file.

What error is the original code giving you? What compiler are you using?

According to everything I looked at w.r.t. Windows said that the way I did it is correct and the CI tests run properly with the shared lib. Maybe there's some other issue that's showing up as a problem with this declaration?

[asmaloney]

Quick follow-up. I just tried it locally with this compiler:

C:/Program Files (x86)/Microsoft Visual Studio/2019/Community/VC/Tools/MSVC/14.29.30133/bin/HostX64/x64/cl.exe

The test executable builds and runs without warnings/errors when using E57Format as a shared lib. It uses both Data3DPointsFloat and Data3DPointsDouble.

[marxin]

Ok, so my original problem is that Data3DPointsData_t<double> and Data3DPointsData_t<false> are hidden in Linux shared library. If I added E57_DLL at line 747 I got the following Windows build errors:

D:\a\libE57Format\libE57Format\include\E57SimpleData.h(747): error C2220: the following warning is treated as an error
D:\a\libE57Format\libE57Format\include\E57SimpleData.h(747): warning C4910: 'e57::Data3DPointsData_t<float>': '__declspec(dllexport)' and 'extern' are incompatible on an explicit instantiation
D:\a\libE57Format\libE57Format\include\E57SimpleData.h(748): warning C4910: 'e57::Data3DPointsData_t<double>': '__declspec(dllexport)' and 'extern' are incompatible on an explicit instantiation

which is about one can't combine extern and __declspec(dllexport) at one line. And so I suggested to use E57_DLL in the cpp file.

[marxin]

I will verify that the symbol is hidden on Linux tomorrow.

[asmaloney]

Aha! I thought you were on Windows for some reason.

which is about one can't combine extern and __declspec(dllexport) at one line

Yeah - I think the Windows stuff is correct (based on CI & my very limited local testing) - it was confusing to get working because it seems backwards - and I know the macOS version works because that's what I use.

I don't have Linux set up so I can't test it other than through CI. That said - the Linux shared lib CI seems to compile cleanly and work with the original code. Are you using gcc? Maybe there's a compiler switch that might be causing this that I need to account for?

Can you quote the full error when trying to use the original code?

Data3DPointsData_t<false>

Typo when writing on GitHub or is that in your code?

[asmaloney]

Oh - it would also be interesting to see the contents of E57Export.h which is a generated file and will be at the top-level of your build directory.

[marxin]

All right, so the situation is a bit more complicated as I'm using the conan package: conan-io/conan-center-index#19135 where they use hidden default visibility (-fvisiblity=hidden).

With conan create . libe57format/3.0.2@ --build=missing -o libe57format:shared=True I get:

nm -C /home/marxin/.conan/data/libe57format/3.0.2/_/_/package/752ae3741ec7065181ee66ffca66ad784a951d0f/lib/libE57Format.so | grep ::Data3DPointsData_t
...
000000000021ba70 t e57::Data3DPointsData_t<double>::Data3DPointsData_t(e57::Data3D&)
000000000021ba70 t e57::Data3DPointsData_t<double>::Data3DPointsData_t(e57::Data3D&)
000000000021b1a0 t e57::Data3DPointsData_t<double>::~Data3DPointsData_t()
000000000021b1a0 t e57::Data3DPointsData_t<double>::~Data3DPointsData_t()
000000000021b360 t e57::Data3DPointsData_t<float>::Data3DPointsData_t(e57::Data3D&)
000000000021b360 t e57::Data3DPointsData_t<float>::Data3DPointsData_t(e57::Data3D&)
000000000021afe0 t e57::Data3DPointsData_t<float>::~Data3DPointsData_t()
000000000021afe0 t e57::Data3DPointsData_t<float>::~Data3DPointsData_t()

so as seen, t means hidden. However, if I include the patch from this pull request I get to:

000000000021ca90 W e57::Data3DPointsData_t<double>::Data3DPointsData_t(e57::Data3D&)
000000000021ca90 W e57::Data3DPointsData_t<double>::Data3DPointsData_t(e57::Data3D&)
000000000021c1c0 W e57::Data3DPointsData_t<double>::~Data3DPointsData_t()
000000000021c1c0 W e57::Data3DPointsData_t<double>::~Data3DPointsData_t()
000000000021c380 W e57::Data3DPointsData_t<float>::Data3DPointsData_t(e57::Data3D&)
000000000021c380 W e57::Data3DPointsData_t<float>::Data3DPointsData_t(e57::Data3D&)
000000000021c000 W e57::Data3DPointsData_t<float>::~Data3DPointsData_t()
000000000021c000 W e57::Data3DPointsData_t<float>::~Data3DPointsData_t()

which seems correct to me. And the content of E57Export.h with this pull request:

#ifndef E57_DLL_H
#define E57_DLL_H

#ifdef E57FORMAT_STATIC_DEFINE
#  define E57_DLL
#  define E57FORMAT_NO_EXPORT
#else
#  ifndef E57_DLL
#    ifdef E57Format_EXPORTS
        /* We are building this library */
#      define E57_DLL __attribute__((visibility("default")))
#    else
        /* We are using this library */
#      define E57_DLL __attribute__((visibility("default")))
#    endif
#  endif

#  ifndef E57FORMAT_NO_EXPORT
#    define E57FORMAT_NO_EXPORT __attribute__((visibility("hidden")))
#  endif
#endif

#ifndef E57FORMAT_DEPRECATED
#  define E57FORMAT_DEPRECATED __attribute__ ((__deprecated__))
#endif

#ifndef E57FORMAT_DEPRECATED_EXPORT
#  define E57FORMAT_DEPRECATED_EXPORT E57_DLL E57FORMAT_DEPRECATED
#endif

#ifndef E57FORMAT_DEPRECATED_NO_EXPORT
#  define E57FORMAT_DEPRECATED_NO_EXPORT E57FORMAT_NO_EXPORT E57FORMAT_DEPRECATED
#endif

#if 0 /* DEFINE_NO_DEPRECATED */
#  ifndef E57FORMAT_NO_DEPRECATED
#    define E57FORMAT_NO_DEPRECATED
#  endif
#endif

// Windows DLLs can't include the extern keyword when declaring templates 
#ifdef _MSC_VER                 
#define TEMPLATE_EXTERN         
#else                           
#define TEMPLATE_EXTERN extern  
#endif                          
                                
// NOTE: This is a generated file. Any changes will be overwritten.
#endif /* E57_DLL_H */

[asmaloney]

Thanks Martin.

The reason I'm pushing on this is that I'm seeing an inconsistency between what the CI seems to say and what you're experiencing, so I'd like to figure out exactly what's going on before making changes.

1. Hidden visibility should be on by default - it is controlled by E57_VISIBILITY_HIDDEN. So that should be the same between CI and your conan build.
2. What version of gcc are you using? The CI is using 11.3.0.
3. Would you be able to compile libE57Format as an so without conan and see what nm tells you?

...if I include the patch from this pull request...

Removing the extern template declarations from the header defeats the purpose - namely instantiating our (only) two cases once so they only appear in one translation unit.

[marxin]

The reason I'm pushing on this is that I'm seeing an inconsistency between what the CI seems to say and what you're experiencing, so I'd like to figure out exactly what's going on before making changes.

Sure, that's super fine and I think I found the root cause. It's the following thing:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index ca34630..844652d 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -227,7 +227,7 @@ if ( E57_BUILD_TEST )
     # If we are building tests, we need access to all the symbols, so override visibility
     set_target_properties( E57Format
         PROPERTIES
-            CXX_VISIBILITY_PRESET default
+            CMAKE_CXX_VISIBILITY_PRESET default
             CMAKE_VISIBILITY_INLINES_HIDDEN OFF
         )

which the change I can't see the symbols exported in the library:

marxin@marxinbox:~/Programming/libE57Format/build> nm -C libE57Format.so | grep 'Data3DPointsData_t()'
marxin@marxinbox:~/Programming/libE57Format/build> 

Does it make sense now?

[asmaloney]

Ahh! Not testing what I think I'm testing! 🤦

Changing it from CXX_VISIBILITY_PRESET to CMAKE_CXX_VISIBILITY_PRESET makes the visibility hidden again. (CMAKE_CXX_VISIBILITY_PRESET can only be set when a target is created, so setting it here has no effect.)

OK good. So can you try one last thing please?

Could you please revert the changes to the header so the the only change is adding E57_DLL in the cpp? If that works for you, then that's our solution.

[marxin]

Ahh! Not testing what I think I'm testing! facepalm

Heh, that happens ;)

OK good. So can you try one last thing please?

Sure.

Could you please revert the changes to the header so the the only change is adding E57_DLL in the cpp? If that works for you, then that's our solution.

Yep. If I do that I end up with:

/home/marxin/Programming/libE57Format/src/E57SimpleData.cpp:250:28: error: type attributes ignored after type is already defined [-Werror=attributes]
  250 |    template struct E57_DLL Data3DPointsData_t<float>;
      |                            ^~~~~~~~~~~~~~~~~~~~~~~~~
/home/marxin/Programming/libE57Format/src/E57SimpleData.cpp:251:28: error: type attributes ignored after type is already defined [-Werror=attributes]
  251 |    template struct E57_DLL Data3DPointsData_t<double>;
      |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~

[asmaloney]

So I think I can fix this, however now I get UBSan failures related to xercesc_3_2::SAX2XMLReader vs. xercesc_3_2::SAX2XMLReaderImpl when running tests.

Not sure what's going on, but not surprised it's an issue with Xerces.

I will check it in on a branch to see if other warnings/errors show up on other compilers.

[marxin]

Thank you very much!

[asmaloney]

So I think I can fix this

Actually I'm not sure I can fix this 😆

Seems like conflicts between extern templates and hidden symbol visibility. There are too many combinations of compiler, OS, visibility, and shared/static. Fix one combination and it breaks another.

I may remove the E57_VISIBILITY_HIDDEN option altogether to see if that helps.