Possible bug in CEGUI 0.5.0 final release

[dteviot]

Hi Guys,
I believe theres a small error in your 0.5.0 final release code.
The short form is, your class RefCounted in the file CEGUIRefCounted.h has a CEGUIEXPORT modifier. i.e.

Code: Select all

template<typename T>
class CEGUIEXPORT RefCounted
{


CEGUIEXPORT is a macro that expands into a declspec(import) or declspec(export) modifier for Windows builds.
However, as RefCounted is a template, it will not be in the DLL. So, I recommend you remove the CEGUIEXPORT.

This link gives a detailed description of the problems resulting from the CEGUIEXPORT modifier being present.
http://www.xcomufo.com/forums/index.php ... 027300&hl=
Regards

[lindquist]

IIRC the reason this was added was to avoid warnings on MSVC8 (2005).
CEGUI has been tested and works properly in MSVC8. The link you have posted is broken, but I'm interested in hearing what (negative) effects this can have. Obviously the template will not be part of the DLL...

[dteviot]

Here's a copy of the problem.

[quote name='Pi Masta' date='Dec 17 2006, 11:42 AM' post='162376']
Ok, I mentioned this to reist, and probably confusingly as well. (Also doesn't help that this only affects Windows and he cannot test this himself) I think I found out why the dependencies are not compiling on my machine (and Xenocide with it). It is due to a little mundane addition to the 'CEGUIRefCounted.h' file, making its class 'export' to the DLL. They actually made this update in version 0.5 of CEGUI, however, it got overlooked when updating CEGUI in the dependencies until recently (revision 1499).

Let me explain what happens. The 'RefCounted' class is merely a template that seems to work as a shared pointer. Before it was just a normal class with the template argument. Our CEGUIPython uses this file and its template. (It is a useful utility class, but it should not specific to CEGUI or CEGUIPython). After the update it was given the 'CEGUIEXPORT' macro prefix which is defined in CEGUIBase.h:

Code: Select all

/*************************************************************************
   Dynamic Library import / export control conditional
   (Define CEGUIBASE_EXPORTS to export symbols, else they are imported)
*************************************************************************/
#if defined( __WIN32__ ) || defined( _WIN32 )
#   ifdef CEGUIBASE_EXPORTS
#      define CEGUIEXPORT __declspec(dllexport)
#   else
#      define CEGUIEXPORT __declspec(dllimport)
#   endif
#      define CEGUIPRIVATE
#else
#      define CEGUIEXPORT
#      define CEGUIPRIVATE
#endif


Nothing too big here, a lot of the classes in CEGUI use this macro to allow them to be imported when called outside of the project and exported when building CEGUI (CEGUIBase has CEGUIBASE_EXPORTS defined on the command line). I'm assuming that CEGUI added it to the RefCounted template so that we can access the other classes RefCounted methods.

However here is where the problem lies in. Take a look at CEGUIPythonFunctor.h (well the important parts for my point):

Code: Select all

#include "CEGUIRefCOunted.h"

namespace CEGUI
{

class PythonWrapper
{
// code here
};

class PythonFunctor
{
// code, methods and stuff
private:
   RefCounted<PythonCallbackWrapper> pythonCallback;
};

} // namespace


Now then, since CEGUIBASE_EXPORTS isn't defined in our project (which it shouldnt), the macro CEGUIEXPORT gets replaced with __declspec(dllimport). So when the compiler goes to link the pythonCallback with CEGUI's RefCounted, it goes to the CEGUIBase_d.dll and tries to look up the code for RefCounted<PythonCallbackWrapper> class (So that it can call it through the DLL).

See the problem? The DLL doesn't have this class, (RefCounted is in a header it's never compiled directly as it is a template). Furthermore the DLL has no idea what a PythonCallbackWrapper is, that is defined in our project.

Now, why hasn't this cropped up on anyone else yet?

• This affects windows only, you can see from the macro's definition that if it is not compiled on Windows, it gets defined to nothing
• I'm guessing that nobody using windows has rebuilt the Dependencies lately, they should get this error as well
• Everything still works for the Windows users (remember when they rebuilt the dependencies for CEGUI at 0.5 release, RefCounted wasn't updated), so nobody has bothered to rebuild the dependencies
• CEGUIPython is really the only Add-on for CEGUI AFAIK, I don't think anybody tries to use their RefCounted class like we are trying to.

[/quote]

[lindquist]

While this poses a problem for users of the RefCounted template it also ensures that it works properly for CEGUI classes.
Objects wrapped in this are returned from the DLL which is why MSVC complains if it's not there.

Should we make it generic for people to use in their own project or should we have CEGUI behave properly?

Not a hard question to answer...

I'd like to know if we can fix it without harming anything though!

[dteviot]

I think you are mistaken. I've tried compiling the 0.5.0 code with the CEGUIEXPORT removed from the RefCounted template, and I can not find any warnings that are related to the RefCounted class.
When I look at the CEGUI code some more, the only usage of RefCounted is the Connection class in Event.

Code: Select all

class CEGUIEXPORT Event
{
...
typedef RefCounted<BoundSlot> Connection;


And as the event class has a CEGUIEXPORT modifier, this gives Event::Connection a CEGUIEXPORT modifier.

[lindquist]

ok. first off, I may well be talking out of my arse here. I haven't tested...

IIRC the problem arises if you use Event::Connection in client code. And AFAIK being a member class does not mean you inherit dll export/import attributes.

If you can point me to some documentation on this it will be much appreciated.

[dteviot]

It looks like you're correct, nested classes do not get their enclosing class's declspec modifier. The relevant documentation appears to be:
http://msdn2.microsoft.com/en-us/library/81h27t8c.aspx

With the important lines being:
"When you declare a class dllexport, all its member functions and static data members are exported."
"All base classes of an exportable class must be exportable. If not, a compiler warning is generated. Moreover, all accessible members that are also classes must be exportable."

This one discusses templates and declspec:
http://msdn2.microsoft.com/en-us/library/twa2aw10.aspx
The relevant section appears to be:
Because of a change in behavior introduce in Visual C++ .NET to make the application of dllexport more consistent between regular classes and specializations of class templates, if you apply dllexport to a regular class that has a base class that is not marked as dllexport, the compiler will generate C4275.
The compiler generates the same warning if the base class is a specialization of a class template. To work around this, mark the base-class with dllexport. The problem with a specialization of a class template is where to place the __declspec(dllexport); you are not allowed to mark the class template. Instead, explicitly instantiate the class template and mark this explicit instantiation with dllexport. For example:

Code: Select all

template class __declspec(dllexport) B<int>;
class __declspec(dllexport) D : public B<int> {
// ...


[dteviot]

Actually, thinking about it some more, I suspect that trying to use Event::Connection in a client that calls the DLL would be problematic. The Event class is exported in the DLL, but Event::Connection is not.

[lindquist]

Thanx for these links.
So it seems the CEGUIEXPORT attribute should be removed from the RefCounted template class, and instead the RefCounted<BoundSlot> specialization should be explicitly instantiated with CEGUIEXPORT.

Code: Select all

template class CEGUIEXPORT RefCounted<BoundSlot>;
class Event {
    typedef RefCounted<BoundSlot> Connection;
};


or maybe this would work?

Code: Select all

class Event {
    typedef CEGUIEXPORT RefCounted<BoundSlot> Connection;
};


I should test this but I dont have time to fire up C++ right now.
Also the rest of the code will have to be looked as I'm pretty sure we're marking other template classes with CEGUIEXPORT.

[dteviot]

I believe the second option will work, but I haven't tried it.
I don't think there's any urgency in resolving the problem.