Crystal Space
Welcome, Guest. Please login or register.
October 24, 2014, 10:35:52 pm

Login with username, password and session length
Search:     Advanced search
9011 Posts in 2044 Topics by 8817 Members
Latest Member: Natsaw1610
* Home Help Search Login Register
+  Crystal Space
|-+  Crystal Space Project Development
| |-+  Development Discussion
| | |-+  non-virtual destructors in pure-virtual base classes, TIME-BOMB
« previous next »
Pages: [1] Print
Author Topic: non-virtual destructors in pure-virtual base classes, TIME-BOMB  (Read 9258 times)
evought
Newbie
*
Posts: 9


View Profile Email
« on: November 14, 2005, 07:06:27 am »

I am looking at the spew of warnings in CEL about non-virtual destructors. The CS psuedo-stable has the same warnings, but it has been turned off via a compiler option in the recent cvs versions. In the code, (e.g.: cs/include/csutil/scf_interface.h, I see:

  // Jorrit: removed the code below as it causes 'pure virtual' method
  // calls to happen upon destruction.
  //protected:
  //// Needed for GCC4. Otherwise emits a flood of "virtual functions but
  //// non-virtual destructor" warnings.
  //virtual ~iBase() {}

and this seems typical of about twenty gazillion files in the various codebases. What is going on here, is there a problem which needs solving? This is not a GCC 4.x problem. GCC gives the warning because the code is explosively dangerous without the virtual destructor. It can result in severe memory/resource leaks and segfaults.

What happens is that without the declared virtual destructor, the compiler is required (by the standard) to generate a destructor for you. The generated destructor is empty and non-virtual. Now, you have a pointer to a descendent of iBase held in an iBase pointer. The child class has its own destructor, which may, for instance, release critical OpenGL resources. When the pointer is deleted and the object is destructed, because the destructor is non-virtual, the reference is clipped: the auto-generated iBase destructor gets called instead of the subclass destructor. Not having a virtual destructor only makes sense if the class will *never* be subclassed, and since iBase (and many of these classes) have pure-virtual functions, this is obviously not the case. Aside from leakage, you can end up with really hairy problems because of objects not cleaning up state and lead to a core-dump several bounces down the road. If you use multiple inheritance anywhere, you can end up with nasty memory corruption.

I assume the calls were commented out for a reason. Jorrit refers to a 'pure virtual' call happening (presumably with subsequent core dump). This is usually caused by bad casts and is often fixable (or at least diagnosable) by using appropriate checked cast templates (static_cast<>(), dynamic_cast<>(), reinterpret_cast<>() and friends) and a bit of code reorg. Is this something you would like tracked down and fixed?
Logged
jorrit
Administrator
Hero Member
*****
Posts: 1706


View Profile
« Reply #1 on: November 14, 2005, 08:17:07 am »

I am looking at the spew of warnings in CEL about non-virtual destructors. The CS psuedo-stable has the same warnings, but it has been turned off via a compiler option in the recent cvs versions. In the code, (e.g.: cs/include/csutil/scf_interface.h, I see:

  // Jorrit: removed the code below as it causes 'pure virtual' method
  // calls to happen upon destruction.
  //protected:
  //// Needed for GCC4. Otherwise emits a flood of "virtual functions but
  //// non-virtual destructor" warnings.
  //virtual ~iBase() {}

and this seems typical of about twenty gazillion files in the various codebases. What is going on here, is there a problem which needs solving? This is not a GCC 4.x problem. GCC gives the warning because the code is explosively dangerous without the virtual destructor. It can result in severe memory/resource leaks and segfaults.

Actually in this particular case this is not true. The interfaces don't have any members so the warning is not really harmful. It is annoying though and we are looking into trying to fix that warning. Eric Sunshine recently told us that he had a solution in the making for this. We can't just add the virtual destructor as that causes a part of CS to break. Eric has a fix for that.

Quote
What happens is that without the declared virtual destructor, the compiler is required (by the standard) to generate a destructor for you. The generated destructor is empty and non-virtual. Now, you have a pointer to a descendent of iBase held in an iBase pointer. The child class has its own destructor, which may, for instance, release critical OpenGL resources.

We are talking about SCF interfaces here. These don't have any resources to free. So for this particular situation it is a non-issue (but an annoying warning).

Greetings,
Logged
sunshine
Administrator
Sr. Member
*****
Posts: 294


View Profile
« Reply #2 on: November 14, 2005, 02:27:43 pm »

I hope, in the near, future to have this resolved by adding the virtual destructors and eliminating the compilation switch. Unfortunately, there is a serious design flaw in some of the meshes (at least in Thing) which causes the application to crash when virtual destructors are present, so the design flaw needs to be re-worked before this issue can be resolved. I spent one weekend debugging the problem (or at least one of the design problems), and another weekend implementing a partial solution, but have not yet had time for implementhing the full solution.
Logged
jorrit
Administrator
Hero Member
*****
Posts: 1706


View Profile
« Reply #3 on: November 14, 2005, 02:32:43 pm »

I hope, in the near, future to have this resolved by adding the virtual destructors and eliminating the compilation switch. Unfortunately, there is a serious design flaw in some of the meshes (at least in Thing) which causes the application to crash when virtual destructors are present, so the design flaw needs to be re-worked before this issue can be resolved. I spent one weekend debugging the problem (or at least one of the design problems), and another weekend implementing a partial solution, but have not yet had time for implementhing the full solution.


I'm curious. What is the nature of this design problem?

Greetings,
Logged
sunshine
Administrator
Sr. Member
*****
Posts: 294


View Profile
« Reply #4 on: November 14, 2005, 02:47:32 pm »

There are a couple related issues:

(1) Many of the meshes have iPolygonMesh implementations embedded within them, yet the iPolygonMesh implementations are not marked as SCF embedded objects. This means that the reference counts on these embedded objects are independent of the reference counts of the objects in which they are embedded. This can lead to really awful situations where either the embedded and/or the embedding object are destroyed independently of the other. In fact, this is what happens with Thing. The Thing mesh object is destroyed while its csObjectModel is still holding references to the iPolygonMesh embedded in the Thing object. Later, when the csObjectModel releases those references, the embedded iPolygonMeshes are destroyed a second time. This bug occurs *all* the time so it is not caused by having virtual destructors; virtual destructors merely allow the bug (which is constantly occurring) to manifest as an actual crash. This situation *must* be fixed in all meshes. Having embedded objects with independent reference counts is about the biggest NO-NO one can imagine. Also, this design flaw isn't limited just to iPolygonMesh implementations. The same problem is repeated in the meshes for some of the implementations of the shader-related interfaces.

(2) When/if the iPolygonMeshes are properly embedded (which is not currently the case), there is a circular reference between the mesh object and its csObjectModel (from which the mesh either inherits or which it embeds). In particular, the mesh holds the object model, which holds the iPolygonMesh implementation(s) embedded within the mesh. The iPolygonMesh implementation, because it is embedded, then holds the mesh, which holds the object model, which holds the iPolygonMesh(es), which hold the mesh, which hold the object model, and so on. Because of the circularity, the mesh is never destroyed.

If you want to fix these problems, please go ahead and do so. I simply haven't had the necessary time to work on it.
Logged
jorrit
Administrator
Hero Member
*****
Posts: 1706


View Profile
« Reply #5 on: November 14, 2005, 02:50:26 pm »

There are a couple related issues:

(1) Many of the meshes have iPolygonMesh implementations embedded within them, yet the iPolygonMesh implementations are not marked as SCF embedded objects. This means that the reference counts on these embedded objects are independent of the reference counts of the objects in which they are embedded. This can lead to really awful situations where either the embedded and/or the embedding object are destroyed independently of the other. In fact, this is what happens with Thing. The Thing mesh object is destroyed while its csObjectModel is still holding references to the iPolygonMesh embedded in the Thing object. Later, when the csObjectModel releases those references, the embedded iPolygonMeshes are destroyed a second time. This bug occurs *all* the time so it is not caused by having virtual destructors; virtual destructors merely allow the bug (which is constantly occurring) to manifest as an actual crash. This situation *must* be fixed in all meshes. Having embedded objects with independent reference counts is about the biggest NO-NO one can imagine. Also, this design flaw isn't limited just to iPolygonMesh implementations. The same problem is repeated in the meshes for some of the implementations of the shader-related interfaces.

Hmm... note that iPolygonMesh no longer has to be embedded. i.e. that kind of usage is deprecated (I mean SCF_QUERY_INTERFACE of iPolygonMesh against the iMeshObject). This has been deprecated a LOOONG time ago so I don't think it would be problematic to remove support for this now. The proper way to get the polygon mesh is to use mesh->GetObjectModel()->GetPolygonMeshXxx().

Greetings,
Logged
sunshine
Administrator
Sr. Member
*****
Posts: 294


View Profile
« Reply #6 on: November 14, 2005, 02:52:54 pm »

The simplest solution is to simply allocate the iPolygonMesh implementations from the heap. That should resolve the issue nicely.

On the other hand, I was looking for a solution which would avoid these extra heap allocations (which might get expensive when a lot of meshes are in play), but had difficulty discovering a workable solution.

(I did, by the way, in my partial solution remove the capability to perform that long-deprecated SCF query.)
Logged
evought
Newbie
*
Posts: 9


View Profile Email
« Reply #7 on: November 14, 2005, 07:24:12 pm »

There are a couple related issues:

(1) Many of the meshes have iPolygonMesh implementations embedded within them, yet the iPolygonMesh implementations are not marked as SCF embedded objects. This means that the reference counts on these embedded objects are independent of the reference counts of the objects in which they are embedded. This can lead to really awful situations where either the embedded and/or the embedding object are destroyed independently of the other. In fact, this is what happens with Thing. The Thing mesh object is destroyed while its csObjectModel is still holding references to the iPolygonMesh embedded in the Thing object. Later, when the csObjectModel releases those references, the embedded iPolygonMeshes are destroyed a second time. This bug occurs *all* the time so it is not caused by having virtual destructors; virtual destructors merely allow the bug (which is constantly occurring) to manifest as an actual crash. This situation *must* be fixed in all meshes. Having embedded objects with independent reference counts is about the biggest NO-NO one can imagine. Also, this design flaw isn't limited just to iPolygonMesh implementations. The same problem is repeated in the meshes for some of the implementations of the shader-related interfaces.
<snip>

So what is happening is that when you destruct an already destructed object, its memory may have already been reused and the D/VMT pointer is bad (or arbitrary) which leads to calling random virtual methods and a messy crash. I can look at some of this, but, not understanding the history and design, it will take me a bit (though this discussion is helping). Two possibilities spring to mind, one is to reorganize the references. It sounds like you are already deprecating/ removing some old methods which will make this easier; the second is to add indirection to avoid the double-death. Basically, bury the iPolygonMesh implementation deeper so that 'destroying it' just destroys a wrapper (which decrements a reference) and the second destruction actually does the job. Double-indirection may be a good immediate solution with the reorg happening  a little slower. The double-indirection can be modified to add some assertions to help check that the solution works.

One thing I have found to greatly help in other projects is gratuitous mutilation: visibly damage an object when it is destructed in a way which can be tested by an assertion. Thus the program will fail on the first illegal access after destruction rather than when the memory is reclaimed. It cannot protect every instance but detects 99% of them. Have an inlinable 'checkvalid()' method in the base class which can be called from major operations. It will not generally affect performance and assertions can be macroed out.
Logged
evought
Newbie
*
Posts: 9


View Profile Email
« Reply #8 on: November 14, 2005, 07:25:57 pm »

The simplest solution is to simply allocate the iPolygonMesh implementations from the heap. That should resolve the issue nicely.

On the other hand, I was looking for a solution which would avoid these extra heap allocations (which might get expensive when a lot of meshes are in play), but had difficulty discovering a workable solution.

(I did, by the way, in my partial solution remove the capability to perform that long-deprecated SCF query.)


I would recommend the heap allocation. If it becomes a performance issue, we can implement a custom allocator (not fun, but effective).
Logged
sunshine
Administrator
Sr. Member
*****
Posts: 294


View Profile
« Reply #9 on: November 14, 2005, 11:26:40 pm »

I'm inclined to go with heap allocation also because it is a simple solution and easy to understand.

As for the other solutions I was investigating, which would keep the iPolygonMesh implementations embedded, I was more concerned about heap size than performance since it is common for quite a lot of mesh objects to exist at one time, and experience has shown that it is possible to shave a good deal of memory (sometimes megabytes) from a large game's footprint by being a bit more careful about mesh sizes. (At least this is my recollection from some of Jorrit's changes.)
Logged
res
Develazyoper
CS Developer
Full Member
*****
Posts: 206


View Profile Email
« Reply #10 on: December 01, 2005, 08:50:06 pm »

(2) When/if the iPolygonMeshes are properly embedded (which is not currently the case), there is a circular reference between the mesh object and its csObjectModel (from which the mesh either inherits or which it embeds). In particular, the mesh holds the object model, which holds the iPolygonMesh implementation(s) embedded within the mesh. The iPolygonMesh implementation, because it is embedded, then holds the mesh, which holds the object model, which holds the iPolygonMesh(es), which hold the mesh, which hold the object model, and so on. Because of the circularity, the mesh is never destroyed.

Maybe change csObjectModel to not keep references to the poly meshes?... That would break up the circularity above. The impracticality is that existing code that relies on csObjectModel keeping a ref may break. However, since csObjectModel's stated intention is to be somehow embedded into the mesh class and the mesh is also a practical place to implement iPolygonMesh, this idea seems somewhat sensible.
Logged
sunshine
Administrator
Sr. Member
*****
Posts: 294


View Profile
« Reply #11 on: December 02, 2005, 02:52:53 am »

I had actually implemented that approach and, although it fixed that problem, the program crashed later on. I didn't have time to debug that new crash, but had reasoned out why the new crash occurred. Unfortunately, I can no longer recall the reason. I do seem to remember that, with that approach, the original mesh and its iPolygonMeshes were getting destroyed while something was still pointing at (but not holding a reference) to the iPolygonMeshes or more likely the object model. When that something tried invoking a method of the iPolygonMesh (which had already been destroyed), it crashed.
Logged
Pages: [1] Print 
« previous next »
Jump to:  

Powered by MySQL Powered by PHP Powered by SMF 1.1.2 | SMF © 2006-2007, Simple Machines LLC Valid XHTML 1.0! Valid CSS!
Page created in 9.482 seconds with 16 queries.