🔒 Locked

Virtual object lifetimes

Started by mr_jrt Jun 4, 2006 at 12:02 PM 15 replies 8.4k views

Original Post

Jun 4, 2006 at 12:02 PM

Here's an interesting one for you lot. I'm having trouble with the lifetimes of virtual base classes. I think. Here's a rough simplified analagy of what I'm working with. Interfaces:

iReferenceCountedObject - Guess, I dare you.
iModel - A generic model. Has a virtual iReferenceCountedObject base class, as all iModels have to support reference counting sematics.

Classes:

cReferenceCountedObject - Implementes common reference counting functionality. Has a virtual iReferenceCountedObject base class.
cModelBase - Implements common functionality for models. Has virtual iReferenceCountedObject, iModel, and cReferenceCountedObject base classes.
cModel - Implements all the functionality of a model. Has a cModelBase base class (and thus supports both the iReferencecountedObject and iModel interfaces).
cManager - Keeps track of children. Has a createChild() that returns a new iModel instance, as well as register(iModel*), and unRegister(iModel*).

Right, them's the classes. Here be thar problem. In cManager::createChild(), a cModel is created, and the iModel pointer is then registered to the list of children. In the destructor of cModelBase, the this pointer is passed to cManager::unRegister(). This works correctly. However, here's the strange bit. In unRegister(), I release() the iModel when I remove it from the register. If I examine the object in the debugger, it's all there (especially the vtable). When the release function is called however, we're in undefined territory. The release() code has the 'this' pointer in some random memory location with an invalid instance of the class. When the function returns, the instance is there again. A bad thunk it would seem. In the course of my debugging, I found that this problem only manifests when unRegister() is called from ~cModelBase(). My initial conclusion is that this is a lifetime issue, akin to the reason we don't call virtual functions in constructors/destructors. My understanding is that when unRegister() is called, the 'this' pointer is a cModelBase, not a cModel, the cModel having already been destroyed. It was my understanding that the virtual cReferencecountedObject still existed at this point (this seemingly being confirmed by the fact I can see the whole object clear as day except whilst in release() ), being part of the cModelBase and not the cModel, not to mention that as virtual base classes are constructed first, they should be destroyed last. I'm sure some of you might be tempted to tell me why doing things this way is wrong, but please resist. I want to know why *this* is behaving so strangely, not why it shouldn't be an issue :)

Xai

Jun 4, 2006 at 1:00 PM

I can't quite see the whole picture clearly in my head, but here's what it seems like to me.

You are mixing responsibilities.

There are 2 management issues here, ref count (object lifetime) and management (presence in the manager list). Don't confuse them.

For this to work you have to pick ONE WAY in which objects will be taken out of scope and deleted, which in a ref counted system would usually be when ref-count goes to 0.

Problem is, you are adding a seperate system with the manager that also has first level-status.

If the manager class is just an optional set of code, like any other client, the base class should NOT be unregistering itself from the list, because the base class shouldn't be getting a ref-count 0 until AFTER it has been released from the list (since the list should be making the ref non-zero.

If, instead, the manager is not like other classes, and you are defining a system in which all live object's are known to the manager, then the register / unregister functions should be internal (friend I guess) details strongly coupled to the ModelBase system, and the Register should be being invoked in the constructor ONLY and the UNREGISTER should be being invoked in the base classes destructor ONLY - a find-in-files should reveal only this 1 call each for your entire program.

mr_jrt

Jun 4, 2006 at 7:28 PM

I appreciate you taking the time to reply. However, you're doing what I asked people not to do, comment on the method. I agree things could be better design-wise, but I want to understand the actual technical reason this strange event occurs. Prehaps lifetime was a bad title for the thread, it's possibly more accurately about the order of destruction of virtual base classes.

Why is the object fully present from the outside, but not when inside a virtual function of a virtual base class when the object is being destroyed. The vtable must be intact, as the function is called correctly, but the virtual base object is not behaving as expected whilst inside the actual function.

Enigma

Jun 4, 2006 at 7:33 PM

I think we're going to need to see some real code. It would also be helpful to know what compiler you're using. I tried to replicate your problem from the description you gave but was unable to.

Σnigma

Palidine

Jun 4, 2006 at 7:34 PM

wild guess: are you calling virtual methods in the base class constructor? the vtable is not set up yet for the derived class so that won't work.

-me

mr_jrt

Jun 5, 2006 at 5:39 AM

Quote:
Original post by Palidine
wild guess: are you calling virtual methods in the base class constructor? the vtable is not set up yet for the derived class so that won't work.

Nope, I am well aware of said situations. here's the code that is problematic ('Parent' is a reference to the manager object that created the model). My guess is that the use of the 'this' pointer in this context is the root of the problem.

My guess is that I've created such a rare monstrosity that I've possibly hit a compiler bug. It wouldn't be the first time, I have a knack for finding these things it would seem.

Quote:
Original post by Enigma
I think we're going to need to see some real code. It would also be helpful to know what compiler you're using. I tried to replicate your problem from the description you gave but was unable to.

I'm using VC++.NET 2003.

I don't see how the code will help any more than my description (bar of course, any coding mistakes by me ;)), but I'm game, so here are the relevant bits, edited for brevity...

// From cModelBase.h...class cModelBase :  public virtual cReferenceCountedObject, // Each object should only have one reference count  public virtual iModel // Only one iModel interface should be present{public:  virtual ~cModelBase()  {    Parent.UnregisterModelChild(this);  }protected:  cModelBase(    iGraphics &NewParent  ) :    Parent(NewParent)  {}private:  iGraphics& Parent;	// Reference to graphics parent};

// From cReferenceCountedObject.h...class cReferenceCountedObject   : public virtual iReferenceCountedObject // Only one iReferenceCounted Interface should be present{public:  virtual fastuint addReference(fastuint NewReferences);  virtual const fastuint getReferenceCount() const;  virtual fastuint releaseReference();protected:  cReferenceCountedObject(  ) :    References(1)  {}  virtual ~cReferenceCountedObject()  {}private:  fastuint References;};

// From cModel.h...class cModel :  public cModelBase, // Base functionality  virtual public iModel // Only one iModel interface should be present{  // ...};

// From cGraphicsBase.cpp... (the base functionality of a iGraphics-compatible manager)bool cGraphicsBase::UnregisterModelChild(iModel * Model){  std::vector<iModel*>::iterator Result = std::find(ModelChildren.begin(), ModelChildren.end(), Model);  if (Result != ModelChildren.end())  {    std::cerr << "Model unregistered" << std::endl;    (*Result)->releaseReference(); // <-- Here's where it all goes wrong!    // Model->ReleaseReference(); // Functionally the same as above, but same result, just in case :)    ModelChildren.erase(Result);    return true;  }  else  {    std::cerr << "Model unregistration failed" << std::endl;    return false;  }}

For what it's worth, I can post screendumps of the debugger illustrating this weirdness if need be.

NotAYakk

Jun 5, 2006 at 1:03 PM

cModelBase -> cReferenceCountedObject & iModel
cReferenceCountedObject -> iReferenceCountedObject
iModel -> ??? (you didn't specify, but I'm guessing iReferenceCountedObject)
cModel -> non-virtual cModelBase & iModel

Quote:
In the destructor of cModelBase, the this pointer is passed to cManager::unRegister(). This works correctly.

At this point, you are dealing with a partially destroyed object.

You should never call virtual functions during Construction or Destruction, or cause them to be called.

Doing so does screwed up things. Expecting any sensible result from calling, or causing to be called, a virtual method during construction or destruction is not reasonable.

To fix your problem, stop calling virtual functions during either construction or destruction. If you want to see what is going wrong, exactly, place printf statements in each of the destructors of every class in the heiarchy, saying "class X was destroyed\n".

I'm betting that a class whose interface you are using is being destroyed on you before you call release(). The printf debugging will reveal this.

Now, if you want to be able to do destruction-like activities and call virtual functions, you should use a pre-destroy pattern, like the one sketched below:

class PreDestructable {	public:		virtual ~PreDestructable() {};		virtual void DoPreDestructor() = 0;};class PreDestructionImpl:		private virtual PreDestructable{	public:		virtual ~PreDestructionImpl() {}	public:		class InternalPreDestructor:			private virtual PreDestructionImpl		{			public:				virtual ~InternalPreDestructor() {}  		private:				virtual void InternalDestroy() = 0;				InternalPreDestructor() { RegisterInternalPreDestructor(this); }		};	private:		std::vector<InternalPreDestructor*> destroy_data;		void RegisterInternalPreDestructor( InternalPreDestructor* ipd ) {			destroy_data.push_back(ipd);		}};// This is the class you inherit from when you want a pre-destructor// You pass a pointer to your own class-type as the template parameter// then override PreDestructor( your_own_class*template<typename tag>class PreDestruct:	private PreDestruction::InternalPreDestructor,	public virtual PreDestructable{	public:		virtual ~PreDestruct() {}	private:		virtual void PreDestructor( tag t = 0 ) = 0;		virtual void InternalDestroy() { PreDestructor( (tag) 0 ); } };// example:// struct A: public PreDestruct<A*> {//   // ...//   void PreDestructor(A* a = 0) {//     printf("A has been pre destroyed\n");//   }// };// struct B: public A, public PreDestruct<B*> {//    void PreDestructor(B* b = 0) {//      printf("B has been pre destroyed\n");//    }// };// void test() {//   A* a = new A();//   B* b = new B();//   a->DoPreDestructor();//   b->DoPreDestructor();// }

Excors

Jun 5, 2006 at 2:07 PM

I see the same error in VC2003, using the example code you gave and adding some bits to make it compile - it works fine if cModel is an empty class, but adding a data member causes the 'release' call to have the wrong 'this' (though the order of destruction doesn't change - it does ~cModel, ~cModelBase, ~iModel, ~cRefCntObj, ~iRefCntObj, as expected, which looks like it's well defined and ought to work).

In VC2005, it does work correctly. VC2003 thinks 'this' (in releaseReference during ~cModelBase) is sizeof(cModel)-sizeof(cModelBase) bytes earlier than it should be. So I guess that either it's a compiler bug, or it's undefined behaviour and the implementation has changed between VC2003/2005.

VC2005's undocumented (unknown? at least Google doesn't know about it) reportSingleClassLayout / reportAllClassLayout feature (e.g. compile with "cl /d1 reportSingleClassLayoutcModel example.cpp") gives some interesting information as to how the classes are actually laid out - with the example, I get something like

class cModel    size(32):        +---        | +--- (base class cModelBase) 0      | | {vbptr} 4      | | Parent        | +---        +---8       | (vtordisp for vbase iReferenceCountedObject)        +--- (virtual base iReferenceCountedObject)12      | {vfptr}        +---        +--- (virtual base cReferenceCountedObject)16      | {vbptr}20      | References        +---        +--- (virtual base iModel)24      | {vfptr}28      | {vbptr}        +---cModel::$vbtable@cModelBase@: 0      | 0 1      | 12 (cModeld(cModelBase+0)iReferenceCountedObject) 2      | 16 (cModeld(cModelBase+0)cReferenceCountedObject) 3      | 24 (cModeld(cModelBase+0)iModel)cModel::$vftable@:        | -12 0      | &(vtordisp) cModel::{dtor} 1      | &(vtordispex) thunk: this+=16; goto cReferenceCountedObject::addReference 2      | &(vtordispex) thunk: this+=16; goto cReferenceCountedObject::getReferenceCount 3      | &(vtordispex) thunk: this+=16; goto cReferenceCountedObject::releaseReferencecModel::$vbtable@cReferenceCountedObject@: 0      | 0 1      | -4 (cModeld(cReferenceCountedObject+0)iReferenceCountedObject)cModel::$vftable@iModel@:        | -24 0      | &iModel::TestcModel::$vbtable@iModel@: 0      | -4 1      | -16 (cModeld(iModel+4)iReferenceCountedObject)cModel::{dtor} this adjustor: 12cModel::__delDtor this adjustor: 12cModel::__vecDelDtor this adjustor: 12vbi:              class  offset o.vbptr  o.vbte fVtorDispiReferenceCountedObject      12       0       4 1cReferenceCountedObject      16       0       8 0                 iModel      24       0      12 0

But VC2003 doesn't appear to have any such feature, so I'm not sure how to compare the way it interprets the class. At least you can fix it easily by upgrading your compiler [smile]

hplus0603

Jun 5, 2006 at 2:22 PM

To actually implement reference counted objects, you should probably use a template, OR derive all reference counted interfaces from the same concrete implementation.

Here's an example:

// in referenced.hclass I_Referenced {  public:    virtual void addRef() = 0;    virtual void release() = 0;    virtual void * queryInterface(type_info const & type) = 0;};template<class Base>class ReferencedImpl {  public:    ReferencedImpl() : refCount_(0) {}    ~ReferencedImpl() { assert(refCount_ == 0); }    void addRef() { assert(refCount_ < 100); ++refCount_; }    void release() { assert(refCount_ > 0); if (!--refCount_) delete this; }    void * queryInterface(type_info const & type) { return 0; }};// in modelinstance.hclass I_ModelInstance : public virtual I_Referenced {  public:    virtual void whateverModelInstanceFunction() = 0;};// in texturesource.hclass I_TextureSource : public virtual I_Referenced {  public:    virtual void whateverTextureSourceFunction() = 0;};// in model.cppclass C_Model : public ReferencedImpl<I_ModelInstance>, public I_TextureSource{  public:    C_Model() {}    void * queryInterface(type_info const & type) {      if (type == typeid(I_ModelInstance))        return static_cast<I_ModelInstance *>(this);      if (type == typeid(I_TextureSource))        return static_cast<I_TextureSource *>(this);      return 0;    }    virtual void whateverModelInstanceFunction() { }    virtual void whateverTextureSourceFunction() { }};

Fruny

Jun 5, 2006 at 2:43 PM

Quote:
Original post by mr_jrt
Prehaps lifetime was a bad title for the thread, it's possibly more accurately about the order of destruction of virtual base classes.

C++ FAQ Lite 25.15

Quote:
Why is the object fully present from the outside, but not when inside a virtual function of a virtual base class when the object is being destroyed.

Virtual base classes are destroyed last, after the whole non-virtual inheritance tree.

Quote:
The vtable must be intact, as the function is called correctly, but the virtual base object is not behaving as expected whilst inside the actual function.

Once a derived class destructor has run, the object is no longer of that derived type. The vtable at that point will be that of the class currently being destroyed.

If C derives from B which derives from A, and C override one of A's member functions, then in C's destructor, C's version will be called, while in B's destructor, A's version will be called.

Excors

Jun 5, 2006 at 3:25 PM

Quote:
Original post by Fruny
Once a derived class destructor has run, the object is no longer of that derived type. The vtable at that point will be that of the class currently being destroyed.

I think the issue here is slightly different, and it happens before the virtual base classes' destructors are called:
When destructing a cModel object, it first calls ~cModel, during which 'this' is a valid cModel*. Then it calls ~cModelBase, during which 'this' is a valid cModelBase* (but not a cModel*, so it'll have cModelBase's vtable). That means 'this' should still be inheriting from iModel, and it can be validly (?) cast to a iModel* (e.g. in the call to cGraphicsBase::UnregisterModelChild).

Since iModel inherits from iReferenceCountedObject, you can validly (?) cast it to a iReferenceCountedObject*, e.g. in the call to model->releaseReference(). Since virtual base classes are destructed last, the cReferenceCountedObject base class object still exists at this point, and so cReferenceCountedObject::releaseReference should (?) be called with a cReferenceCountedObject* for 'this'.

But in VC2003, it passes an invalid pointer to cReferenceCountedObject::releaseReference as 'this'. (VC2005 and GCC 3.4.4 both pass the correct cReferenceCountedObject* pointer.)

Should there be a problem in any of those steps?

(There is, of course, the problem of whether you should do those steps, but that was not the original question...)

Excors

Jun 5, 2006 at 3:49 PM

And a more precise example:

#include <iostream>class iReferenceCountedObject{public:    virtual void PrintThis() = 0;};class iModel : public virtual iReferenceCountedObject{};class cReferenceCountedObject :    public virtual iReferenceCountedObject{public:    cReferenceCountedObject() { } // this is required, else it doesn't break (?!)    virtual void PrintThis()    {        std::cout << this << "\n";    };};class cModelBase :    public virtual cReferenceCountedObject,    public virtual iModel{public:    virtual ~cModelBase()    {        this->PrintThis();        ((iModel*)this)->PrintThis();    }};class cModel :    public cModelBase,    public virtual iModel{    int irrelevant_member;};int main(){    cModel* model = new cModel;    model->PrintThis();    ((iModel*)model)->PrintThis();        std::cout << "\n";    delete model;}

That should print the same variable twice with a fully-constructed cModel, and then twice during ~cModelBase (which is after ~cModel, but before any other destructor), with the theory being that cReferenceCountedObject::PrintThis is called with 'this' still being the cReferenceCountedObject object contained within the original cModel.

On VC2003:
002F2F24
002F2F24
002F2F24
002F2F20 // oops - that's not pointing to a cReferenceCountedObject

On VC2005:
002F4F74
002F4F74
002F4F74
002F4F74

On GCC:
0x452de0
0x452de0
0x452de0
0x452de0

mr_jrt

Jun 5, 2006 at 5:59 PM

Firstly, all of you thanks. Special mention to the mighty Excors. All hail Excors! Thanks for proving I'm not going mad, or at the very least my installation isn't screwed up :)

So, seemingly it could actually be a bug. Hmm. I wonder if in Microsoft towers someones cursing that they would've gotten away with it if it weren't for those damn kids....;)

I may post to one of the C++ newsgroups and see what they say as to whether this is specified behavior or simply coincidental amoung the various compilers...if I can survive the burning sensation they'll induce from my mis-use of language features :)

Quote:
Original post by NotAYakk
cModelBase -> cReferenceCountedObject & iModel
cReferenceCountedObject -> iReferenceCountedObject
iModel -> ??? (you didn't specify, but I'm guessing iReferenceCountedObject)
cModel -> non-virtual cModelBase & iModel

Quote:
Original post by mr_jrt
In the destructor of cModelBase, the this pointer is passed to cManager::unRegister(). This works correctly.

At this point, you are dealing with a partially destroyed object.

Yup.

Quote:
Original post by NotAYakk
You should never call virtual functions during Construction or Destruction, or cause them to be called.

I'm betting that a class whose interface you are using is being destroyed on you before you call release(). The printf debugging will reveal this.

Nope. As I said, it's still there in all it's reference-counted glory outside the function. As later proved by Excors.

Quote:
Original post by Fruny
Quote:
Original post by mr_jrt
Prehaps lifetime was a bad title for the thread, it's possibly more accurately about the order of destruction of virtual base classes.

C++ FAQ Lite 25.15

I was waiting for someone to bring up the Cpp FAQ entry :) Yes, I am fully aware of said facts (and was all but ready to admit defeat until Excors's VC2005 discoveries), but took the fact that the cModelBase was a) complete and b) the function being called was in a virtual base class and thus still present, to mean that it should work.

Quote:
Original post by Fruny
Quote:
Original post by mr_jrt
Why is the object fully present from the outside, but not when inside a virtual function of a virtual base class when the object is being destroyed.

Virtual base classes are destroyed last, after the whole non-virtual inheritance tree.

Quote:
Original post by mr_jrt
The vtable must be intact, as the function is called correctly, but the virtual base object is not behaving as expected whilst inside the actual function.

Once a derived class destructor has run, the object is no longer of that derived type. The vtable at that point will be that of the class currently being destroyed.

Fruny, you seem to be trying to counter my code using an explanation that proves my points??? At the point where the cModel is only a cModelBase, the virtual base class is still in existance, as is cModelBase's vtable, this is why I took it to mean that things should work.

Fruny

Jun 6, 2006 at 9:33 AM

Ok, never mind me then. [embarrass]

NotAYakk

Jun 6, 2006 at 10:47 AM

Ayep -- it does look like VC2003 screwed up. :)

However, the pre-destruction code dance I spewed above will avoid the VC2003 bug.

Wierd things happen during destruction. Doing complex things during destruction tends to lead to brittle code. So I avoid doing things in the destruction phase.

Zahlman

Jun 6, 2006 at 12:22 PM

Quote:
Original post by mr_jrt
However, you're doing what I asked people not to do, comment on the method.

I don't like making absolute statements like this, but for the sake of simplification: seemingly everyone makes this request, and it is almost universally wrong to do so.

mr_jrt

Jun 6, 2006 at 1:59 PM

Quote:
Original post by Zahlman
Quote:
Original post by mr_jrt
However, you're doing what I asked people not to do, comment on the method.

I don't like making absolute statements like this, but for the sake of simplification: seemingly everyone makes this request, and it is almost universally wrong to do so.

I can't comment for other cases, but as I explained above, I wasn't interested in why this code was bad (I know it probably is, it's an old hack-job that's slap-bang in the middle of a refactoring jobbie I'm doing on my code. In this case, the intent for the list is a prime candidate for some variety of observer pattern malarky). I just wanted to know the real technical reason why it occurs so I could be aware of this next time it occurs and/or avoid it occurring again. I'm not the kind of guy that likes to have the foundations of his understanding as "because thats the way it is".

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.