Virtual destructor for Obj life Counter

[raakella1]

make the destructor of obj life counter virtual for its destructor to be called when the derived class is destructed

[szmyd]

lgtm

[hkadayam]

cc @raakella1 @szmyd @yamingk

My 2 cents: I don't think this is a right fix - at least that is not the intent of the Obj Life Counter. The justification for this change is that you have base class destructor not called, but real reason for not getting called is not because it not virtual class. We need virtual destructor on base class if you assign a derived class pointer to base class and call delete from base class, something like

Base* ptr = new Derived()
delete ptr

in this case only base class destructor is called, but there is no case where base class destructor is not called. It will always be called.

If you are worried that destructor of derived class of ObjLifeCounter is not called, then also it does not make sense, because there is no code that should do the following, it just doesn't make sense.

ObjLifeCounter< T >* ptr = new T();
delete ptr

In fact I would argue if we indeed have a code like that, its better to catch this by having a memory leak.

Another reason why its not good, is now we are forcing all derived class of this class to be virtual, otherwise we end up getting "size mismatch" during asan. Moreover, it generates a vtable which has cost penalty, so if I wanted a structure with only PODs and need quick access, like below, this virtual keyword prevents from doing it.

int i;
uint64_t ul;
};

[szmyd]

@hkadayam thanks for the response!

I think it's reasonable that any class that has a non-default destructor like ObjLifeCounter does should have either:

1. A final decorator preventing inheritence.
2. The destructor declared virtual

This is not a case of memory leak; this is a case of logic inconsistency. The counter is only decremented when the base class' destructor is called. s_... are static so will be freed during process termination. But the COUNTS are wrong making them useless.

    virtual ~ObjLifeCounter() {
        assert(s_alive.load() > 0);
        s_alive.fetch_sub(1, std::memory_order_relaxed);
    }

In many places we had a band-aid to do:

class MyObject : public ObjectLifeCounter {
  ~MyObject() { ::~ObjectLifeCounter(); }

So; the solution is to either take the cost-penalty or enforce that all users of this code either "remember" this or wrap all interaction with MACROS that handle this non-conformant behaviour.