Hide PugiXML symbols via cmake

[ld-kerley]

In response to #1941. Updating cmake to hide the PugiXML symbols.

Creates separate static library for PugiXML and hide the symbols using cmake, so it should be portable.

[DarkDefender]

I'm still getting some pugi symbols in the shared library I think:

nm -D lib/libMaterialXFormat.so | grep pugi
000000000005119e W _ZNK4pugi16xml_object_rangeINS_17xml_node_iteratorEE3endEv
0000000000051184 W _ZNK4pugi16xml_object_rangeINS_17xml_node_iteratorEE5beginEv
000000000005113e W _ZNK4pugi16xml_object_rangeINS_22xml_attribute_iteratorEE3endEv
0000000000051124 W _ZNK4pugi16xml_object_rangeINS_22xml_attribute_iteratorEE5beginEv

[ld-kerley]

Thats interesting, when I check the same on both my linux and macOS machines I don't see those symbols visible.

I'm also a little suspicious about all the CI failures above too, they appear to fail during cmake generation, claiming that its necessary to export the privately linked static library, which I don't think should be necessary. I don't see that complaint when I build locally - and @DarkDefender if you were able to build with this patch, I'm assuming you didn't see that either.

@jstone-lucasfilm - any idea what's going on with the CI here?

[jstone-lucasfilm]

@ld-kerley That's a new error message to me, so I'd need to investigate further to understand what CMake objects to. Usually this category of error is triggered by a validation step that only exists in the very latest version of a package, e.g. CMake, where GitHub Actions tends to be more cutting edge than my local setup.

[DarkDefender]

@DarkDefender if you were able to build with this patch, I'm assuming you didn't see that either.

I didn't see any cmake failures, no.
Weird that you don't get the symbols in your builds.
It exposes way less pugi symbols than it used to but those four still remains for some reason on my end.
(I also tried in a clean checkout and build directory as well. Same result)

[ld-kerley]

I just tried with latest cmake 3.30 - and still no errors for me - so I'm certainly confused about the CI failures.

The main reason I was trying to go the cmake route to resolve this was to be portable - but it doesn't really look like this is portable at all :/.

[jstone-lucasfilm]

@ld-kerley You may have noticed this pattern as well, but in GitHub Actions, it seems as if builds with MATERIALX_BUILD_SHARED_LIBS are succeeding, and builds without it are failing. I haven't yet taken a close look, but perhaps that can be helpful in diagnosing the cause.

[ld-kerley]

Great clue @jstone-lucasfilm - I was able to reproduce locally with MATERIALX_BUILD_SHARED_LIBS=OFF. Feels a bit like a cmake bug to me, but I'm no expert. Using the BUILD_INTERFACE generator expression does the upstream use of the static library from the export(). I think that neither the shared library or static library build should need the PugiXML library installed because it's privately statically linked.

[jstone-lucasfilm]

This looks good to me, thanks @ld-kerley!

@DarkDefender Does this address the issue with PugiXML symbols that you originally reported?

[DarkDefender]

@jstone-lucasfilm sadly the four symbols from pugi that I posted above still shows up for me. Both in the shared and static libs.
I'm using gcc 13.2.1 and cmake 3.29.3.
I've tried both the gnu ld linker and the lld linker from llvm 17, both have the same four pugi symbols exported.

Edit:
These symbols do not show up in the lib/libpugixml.a file.
So I'm guessing these gets exposed as these are defined in the pugixml.hpp file that is included in the XmlIo.cpp file.

There are of course xml_object_range symbols defined in the lib/libpugixml.a file. But they are a bit different from the ones in lib/libMaterialXFormat.so for some reason.
lib/libMaterialXFormat.so:

000000000005119e W _ZNK4pugi16xml_object_rangeINS_17xml_node_iteratorEE3endEv
0000000000051184 W _ZNK4pugi16xml_object_rangeINS_17xml_node_iteratorEE5beginEv
000000000005113e W _ZNK4pugi16xml_object_rangeINS_22xml_attribute_iteratorEE3endEv
0000000000051124 W _ZNK4pugi16xml_object_rangeINS_22xml_attribute_iteratorEE5beginEv

nm --defined-only -g lib/libpugixml.a | grep xml_object_range:

0000000000000000 W _ZN4pugi16xml_object_rangeINS_17xml_node_iteratorEEC1ES1_S1_
0000000000000000 W _ZN4pugi16xml_object_rangeINS_17xml_node_iteratorEEC2ES1_S1_
0000000000000000 W _ZN4pugi16xml_object_rangeINS_22xml_attribute_iteratorEEC1ES1_S1_
0000000000000000 W _ZN4pugi16xml_object_rangeINS_22xml_attribute_iteratorEEC2ES1_S1_
0000000000000000 W _ZN4pugi16xml_object_rangeINS_23xml_named_node_iteratorEEC1ES1_S1_
0000000000000000 W _ZN4pugi16xml_object_rangeINS_23xml_named_node_iteratorEEC2ES1_S1_

[DarkDefender]

Note that all pugi symbols are hidden if I just do:

diff --git a/source/MaterialXFormat/External/PugiXML/pugiconfig.hpp b/source/MaterialXFormat/External/PugiXML/pugiconfig.hpp
index f739e062..c0bdadb1 100644
--- a/source/MaterialXFormat/External/PugiXML/pugiconfig.hpp
+++ b/source/MaterialXFormat/External/PugiXML/pugiconfig.hpp
@@ -30,6 +30,7 @@
 // #define PUGIXML_NO_EXCEPTIONS
 
 // Set this to control attributes for public classes/functions, i.e.:
+#define PUGIXML_API __attribute__((visibility("hidden")))
 // #define PUGIXML_API __declspec(dllexport) // to export all public symbols from DLL
 // #define PUGIXML_CLASS __declspec(dllimport) // to import all classes from DLL
 // #define PUGIXML_FUNCTION __fastcall // to set calling conventions to all public functions to fastcall
diff --git a/source/MaterialXFormat/External/PugiXML/pugixml.hpp b/source/MaterialXFormat/External/PugiXML/pugixml.hpp
index 53922596..fa883db8 100644
--- a/source/MaterialXFormat/External/PugiXML/pugixml.hpp
+++ b/source/MaterialXFormat/External/PugiXML/pugixml.hpp
@@ -284,7 +284,7 @@ namespace pugi
        #endif
 
        // Range-based for loop support
-       template <typename It> class xml_object_range
+       template <typename It> class PUGIXML_CLASS xml_object_range
        {
        public:
                typedef It const_iterator;

The added PUGIXML_CLASS is to fix some warnings. Note that adding the PUGIXML_CLASS definition does not fix the unhidden symbols in the PR.

Seems like doing what Ray suggested in the issue with simply adding set(CMAKE_CXX_VISIBILITY_PRESET hidden) to the main CMakeLists.txt also works. (But I don't know if that breaks something)

[ld-kerley]

I tried a number of different build variations, and I can't get it to build with the PugiXML symbols visible.

That said I like the idea of hiding all the symbols in the libraries by default, that can lead to smaller binaries and even faster code in some instances.

I don't think we can set set(CMAKE_CXX_VISIBILITY_PRESET hidden) at the top level CMakeLists.txt, but I've pushed a PR that sets this (and also CMAKE_VISIBILITY_INLINES_HIDDEN=1) for each of the library targets, and then only makes visible the symbols that have been explicitly tagged for export.

@DarkDefender - as I can't replicate your pugiXml symbol visibility, could you pull this update down and try it on your side.

@jstone-lucasfilm - I'm pretty sure this is a safe change to make, but I think we should sit on this until after Siggraph, and give people a chance to kick the tires. I'm gonna try building USD against this, but other MateiralX integrators might want to make sure there aren't any symbols that are now hidden that shouldn't be.

[DarkDefender]

@ld-kerley That seems to do the trick!
nm -D lib/*.so | grep pugi returns nothing for me now :)

I just have one nitpick that is not related to the symbols:
Perhaps adding a README.materialx in the pugixml folder that notes or explains the modifications done to the library?
We do this in Blender with our bundled libraries to make it easier for people to see what has changed from the upstream version or if there are no changes.

[jstone-lucasfilm]

This change looks great to me, @ld-kerley, and I had just one question about the inclusion of a new file.

[jstone-lucasfilm]

This looks like a new file that has been accidentally included in the PugiXML folder?

[ld-kerley]

Actually, I added that file intentionally. It captures the differences between the local pugiXML code, and the v1.9 release that was used as a base.

I'm not sure if the patch would ever cleanly merge on top of a newer release - but it was intended to make clearer the differences that the MaterialX project has made.

I did leave a note at the bottom of the readme.txt file describing the addition

[jstone-lucasfilm]

Ah, I can understand the motivation for including the difference file directly, but I wonder if this actually increases our maintenance burden over time, as the difference file would need to be kept in sync with the actual delta between our code and the off-the-shelf version.

Maybe your text description at the bottom of the README is sufficient here, as a developer can always perform a difference between our code and the off-the-shelf code on their own?

[ld-kerley]

If you don't think it adds clarity - I'm happy to remove it - hopefully we can get back to a non-modified library at some point somewhat soon...

[jstone-lucasfilm]

Sounds good, and thanks! Moving away from a modified PugiXML library seems like the right approach, and we'll just need to think through whether it should be accomplished through specification changes (i.e. requiring escape codes for angle brackets) or through refactoring of the codebase (i.e. moving all exceptional behavior from PugiXML to MaterialXFormat).

[jstone-lucasfilm]

This looks like a good improvement to me, thanks @ld-kerley!

[sergeyvfx]

@ld-kerley @jstone-lucasfilm Hey. Is it something you've considered for 1.38 series? Surely, there is some work to be done to make it work (unfortunately, is not as clean as just cherry pick).
If you've considered it, it would simplify fixing some issues for on on Blender side (as there are issues caused by the symbol conflict, and update to the upcoming MaterialX might not be as easy either since USD does not support it yet to my knowledge).