vsg/utils/Builder.h isn't self-sufficient

[AnyOldName3]

When including vsg/utils/Builder.h in a file that doesn't also include vsg/io/Options.h, (at least when consuming static VSG when building a DLL with MSVC 2022) it will generate an error as the compiler can't generate the implicit assignment operator for vsg::Builder because it's only got a forward declaration for vsg::Options, which isn't enough for vsg::ref_ptr<vsg::Options>::operator= as it's got to call ref and unref, so Builder::options breaks.

I've not hit this error when building a static library with the same file and compiler, and my theory for that is that the compiler knows anyone calling it who's consuming a static library has to have the header that it can be implicitly generated from, so doesn't generate it if it's not called, whereas as I've got a class that inherits from vsg::Builder which is declspec(dllexport) when it's built as a DLL, it's mandatory that the compiler generates the implicit assignment operator.

The fix would be to include Options.h in Builder.h if it's meant to be copyable, or delete the assignment operators if it's not.

[robertosfield]

I have just had a look at vsg::Builder and I don't even see a default constructor, let alone a copy operator or copy constructor.

I don't think there is particular value in copy constructor or assignment operator. It would be better users just created it on the heap and then shared that instance.

[robertosfield]

I have added a default constructor to see if it affects your build and need to include Options.h d599ee2.

This change is checked into VSG master. Could you try it out to see if you still need the include of Options.h?

[AnyOldName3]

It's the implicitly generated copy assignment operator that causes the problem. That commit neither provides an alternative copy assignment operator nor explicitly deletes the implicit one, so the implicit one will still get generated, and still need to call ref and unref, so still need vsg::Options to be fully declared. I tried it anyway, and got the same error.

I investigated further.

First I tried building VSG with BUILD_SHARED_LIBS enabled, and that worked fine, so the trigger isn't as simple as the declspec(dllexport).

Next, I tried explicitly defaulting the copy assignment operator in Builder.cpp, which got rid of the errors I was seeing, but triggered new equivalent errors about the destructor and copy constructor, so I treated them the same way, and then got a new more sensible error about the default constructor (as adding an explicit copy constructor disabled the implicit default constructor), so I ended up with an explicit default constructor in the end, too. That chased away the errors. As a reminder, all these functions were already implicitly created inline by the compiler, so all I've done is moved the place that vsg::Options has to be fully declared out of the header rather than creating new pointless functions.

That change was basically

// Builder.h
        Builder();
        Builder(const Builder& rhs);
        Builder& operator=(const Builder&);
    protected:
        virtual ~Builder();
// Builder.cpp
Builder::Builder() = default;
Builder::Builder(const Builder& rhs) = default;
Builder& Builder::operator=(const Builder&) = default;
Builder::~Builder() = default;

Something weird was that it was different files complaining about different functions. I was using the default constructor and the destructor in one of these, but that one wasn't the only one complaining about the destructor, and none of the files should have been copying anything as they were just using a reference to a single instance. If every file complained about everything, or files only complained about the functions they used, it'd make more sense, but what I'm seeing makes me suspicious a compiler bug might be involved.

As it only became a problem when I started consuming VSG in a DLL, I took a look at the documentation for declspec(dllexport), and it says that a declspec(dllexport) or declspec(dllimport) class' parent classes must all have one or the other of the two attributes, otherwise a compiler warning (which I'm not getting) will be emitted and presumably something doesn't work. When consuming a VSG static library in a DLL, this rule's violated, so I thought this might be the cause of the problem, then remembered that I'd originally noticed it when consuming a DLL build of VSG in a DLL, and checked and the attributes are set up correctly in that situation, so it can't be the cause.

The last thing I tried was simply adding a .cpp file to the VSG that just contains

#include <vsg/utils/Builder.h>

with the idea that if it builds, the header's self-sufficient, and if it doesn't, then it isn't.

When I build VSG as a static library, this file is fine. However, when I build it as a dynamic library, it emits the following:

1>BuilderTest.cpp
1>C:\C++Libraries\VulkanSceneGraph\VulkanSceneGraph\include\vsg\core\ref_ptr.h(102,23): error C2027: use of undefined type 'vsg::Options'
1>(compiling source file '../../../src/vsg/utils/BuilderTest.cpp')
1>C:\C++Libraries\VulkanSceneGraph\VulkanSceneGraph\include\vsg\io\FileSystem.h(25,11):
1>see declaration of 'vsg::Options'
1>C:\C++Libraries\VulkanSceneGraph\VulkanSceneGraph\include\vsg\core\ref_ptr.h(102,23):
1>the template instantiation context (the oldest one first) is
1>	C:\C++Libraries\VulkanSceneGraph\VulkanSceneGraph\include\vsg\utils\SharedObjects.h(89,26):
1>	see reference to class template instantiation 'vsg::ref_ptr<vsg::Options>' being compiled
1>	C:\C++Libraries\VulkanSceneGraph\VulkanSceneGraph\include\vsg\core\ref_ptr.h(94,18):
1>	while compiling class template member function 'vsg::ref_ptr<vsg::Options> &vsg::ref_ptr<vsg::Options>::operator =(const vsg::ref_ptr<vsg::Options> &)'
1>		C:\C++Libraries\VulkanSceneGraph\VulkanSceneGraph\include\vsg\utils\SharedObjects.h(99,5):
1>		see the first reference to 'vsg::ref_ptr<vsg::Options>::operator =' in 'vsg::LoadedObject::operator ='
1>C:\C++Libraries\VulkanSceneGraph\VulkanSceneGraph\include\vsg\core\ref_ptr.h(105,27): error C2027: use of undefined type 'vsg::Options'
1>(compiling source file '../../../src/vsg/utils/BuilderTest.cpp')
1>C:\C++Libraries\VulkanSceneGraph\VulkanSceneGraph\include\vsg\io\FileSystem.h(25,11):
1>see declaration of 'vsg::Options'
1>Done building project "vsg.vcxproj" -- FAILED.

and that's even with the explicitly defaulted functions for vsg::Builder. If I get rid of them, the output is exactly the same. That's at least enough to show that there's a problem in the VSG itself rather than just how vsgPhysX is subclassing things, but it's odd that it's dying before it's even finished with the includes with an equivalent problem and yet I managed to skip over it.

I suspect there are quite a few other places where headers only have forward declarations, and while the explicitly declared functions are all fine, the implicit functions aren't, but it's not caused symptoms because no one's tried using those headers on their own. This can be detected with a self-sufficient header check in CI or by manually doing what I did, and then if the implicit functions are supposed to be there, they can be fixed by including the necessary headers or making them explicit and having the implementation in a file with the header included, and if they're not supposed to exist, then they can be explicitly deleted.

So after spending a couple of hours looking into this I've come full circle back to my original theory that it's just some headers in the VSG not being self-sufficient, and it not being noticed as it only affects compilation units that don't have extra includes that hide the problem and might need to export a definition.

As an aside, this shouldn't be a weird Windows problem - GCC isn't used with -fvisibility=hidden, so every symbol is exported by default for Linux builds. It actually might be a good idea to add that flag and make VSG_DECLSPEC expand to __attribute__ ((visibility ("default"))) when GCC is used as it can improve binary size and speed.

[robertosfield]

Forward declaration is typically used to avoid circular references in the headers or to keep the include count down. The need to include <vsg/Options.h> is something that has been present with many of the VSG's .cpp's. The Windows' DLL build is what highlights the problem and I have to go in and add the vsg/Options.h include.

One option might be to delete the copy and assignment operators, that way there isn't any need to implement them, and as they wouldn't typically be needed this shouldn't be an issue. There is the vsg::Object::clone() method though, which might need a null implementation if it's an issue.

It would be an interesting test to try attribute ((visibility ("default"))) if only to use it as an early warnings with the Windows DLL build. I am curious if there would be any memory and performance delta, I suspect little difference.

[AnyOldName3]

For Windows, it's only a potential problem for symbols explicitly marked __declspec(dllexport). For GCC and Clang, it's a potential problem for literally everything, unless you pass -fvisibility=hidden, which makes it work like Windows, then it's only a potential problem for things marked __attribute__ ((visibility ("default"))). By default, Windows is less vulnerable to hitting this kind of problem than Unix.

The reason things haven't been on fire all along is that nothing's compiling the affected headers without also including the headers they implicitly depend on, until a user of VSG tries including a header and hits this. You need to compile headers on their own if you want to proactively detect this kind of problem. I can take a look at adding a self-sufficient header check to CI if you think that would be worthwhile.

[robertosfield]

I tried an experiment on my Kubuntu 22.04 systems with g++ 11.4.0 and a modified include/vsg/core/Export.h with:

#    define VSG_DECLSPEC __attribute__ ((visibility ("default")))

But the library and vsgviewer application sizes are identical for with/without this change, and this applies to static and shared library builds.

However, it's not entirely clear to me what changes you are expecting to be needed, I've just taken my best guess based on how I've interpreted what you mean. This is the problem having to guess rather than have an actual commit with all the required changes in place.

[robertosfield]

To help resolve the immediate issue I have put in explicit deletion of the copy constructor and assignment operation: e0cc165

Could you try this out with vsgPhysX in the various build combinations you've been testing?

[AnyOldName3]

I tried an experiment on my Kubuntu 22.04 systems with g++ 11.4.0 and a modified include/vsg/core/Export.h with:

#    define VSG_DECLSPEC __attribute__ ((visibility ("default")))

But the library and vsgviewer application sizes are identical for with/without this change, and this applies to static and shared library builds.

However, it's not entirely clear to me what changes you are expecting to be needed, I've just taken my best guess based on how I've interpreted what you mean. This is the problem having to guess rather than have an actual commit with all the required changes in place.

It's that, plus adding -fvisibility=hidden to the compiler flags. Otherwise, everything defaults to default visibility, so explicitly labelling things as default doesn't change anything.

MinGW flavours of GCC don't work the same as native Unix ones with this feature, so I'd have to drop what I was doing and reboot into Linux to switch to this tangent. I only brought it up because I realised I'd mentioned a lot of Windows-specific things and wanted to make it clear that it's the least likely platform to run into this problem rather than the only place it can happen.

To help resolve the immediate issue I have put in explicit deletion of the copy constructor and assignment operation: e0cc165

Could you try this out with vsgPhysX in the various build combinations you've been testing?

It stops it complaining about the copy constructor and assignment operator, but the destructor still depends on vsg::Options being fully declared, so has to be moved to the .cpp file. I've opened #1193 to do that.