temisu / ancient

Decompression routines for ancient formats
BSD 2-Clause "Simplified" License
209 stars 14 forks source link

Registry does not work when built as a static library #31

Closed manxorist closed 3 years ago

manxorist commented 3 years ago

Ancient works fine when compiled directly into the resulting executable or when compiled as a shared library (dylib/so/dll).

However, it fails when built as a static library that gets linked into an executable or shared library. The sympton is ancient::internal::decompressors being nullptr due to ancient::internal::Decompressor::registerDecompressor never getting called.

The reason is due to how linkers handle static libraries. They see static libraries as a collection of individual contained object files. They only include any such object file if any symbol it defines was required by any other object file the linker has seen before. The linker makes this decision on the individual object file level contained in a static library. While each individual Decompressor in ancient references the registry registration functions in Decompressor.cpp, it is itself never actually referenced by any other object file inside the library, and thus will not be linked in, which then obviously never invokes the global static initializer for the Registry.

This behavior is documented for example here:

Now, I do consider this behavior (which happens on all major platforms, as far as I know (however, I did not try to reproduce on non-Windows yet)) an outright bug and in violation of the intentions of the C++ standard. However, sadly, reality disagrees with my opinion.

I am not aware of a simple clean solution to this problem. One thing that will work, is making all Decompressor::Registry<> instantiations inline in their respective header files (a C++17 feature) and then including all individual Decompressor header files in 1 single cpp file (which somewhat defeats the purpose of the Registry) that gets referenced when using the library (obvious candidates would be API.cpp or Decompressor.cpp).

This bug actually bit us when integrating ancient into OpenMPT (which is Windows-only, and for simplicity reasons links as much as possible statically, and splits individual libraries into their own respective static library in the build system). While we can work-around the issue by using /WHOLEARCHIVE, I think ancient still needs a solution so that this issue does not bite any other future users.

temisu commented 3 years ago

I thought I knew most of the silly magic things in the linker, but I learned something new today. Hmm, I'll think about a workaround

temisu commented 3 years ago

Should be fixed now. It is not pretty. But neither was registry in the first place

manxorist commented 3 years ago

Thanks, works fine in OpenMPT now.