(no longer active) Reporter
	Description • 13 years ago

It is almost always an error to delete a base class pointer which does not have a virtual destructor:

class Base {
  virtual foo(); // in order for Base to have a vtable
};

class Derived : public Base {
public:
  ~Derived(); // will never be called in f below
};

void f() {
  Base * obj = new Derived();
  delete obj; // oops
}

Having a static analysis to catch this is potentially going to catch things like bug 605732.

	Benjamin Smedberg
	Comment 1 • 13 years ago

This is going to be very hard to do without a whole-program analysis: you aren't guaranteed in any particular translation unit to know both parts of the question:

* I'm deleting a class without a virtual dtor
* That class has subclasses

And even within a translation unit this can be tricky depending on the order you receive type/method information.

	Josh Matthews [:jdm]
	Comment 2 • 13 years ago

Doesn't GCC warn about this? Can we just turn that warning into an error?

	Joshua Cranmer [:jcranmer]
	Comment 3 • 13 years ago

One thing we can do more easily is warn/error if:
1. Base has a virtual function,
2. Base does not have a virtual destructor,
3. delete <expr of type Base> is called from a function f,
4. and f is not a virtual member of Base marked NS_MUST_OVERRIDE.

Note that XPCOM relies on having a vtable for methods but not having a virtual destructor, so we would have to mark all methods in NS_DECL_ISUPPORTS NS_MUST_OVERRIDE to guarantee this.

[NS_MUST_OVERRIDE methods must be overridden in all subclasses, and this is already statically enforced, or would be, if we had a static-checking buildbot already].

	Joshua Cranmer [:jcranmer]
	Comment 4 • 13 years ago

(In reply to Josh Matthews [:jdm] from comment #2)
> Doesn't GCC warn about this? Can we just turn that warning into an error?

There is -Wnon-virtual-dtor, but enabling that as an error would cause all of XPCOM to fail to compile.

	Brian R. Bondy [:bbondy]
	Comment 5 • 13 years ago

If we find a way to do this, something to look out for is that sometimes in IDL (such as nsIXPCScriptable.idl) you will have something like:

> %{ C++
> ...

> class NS_NO_VTABLE nsXPCClassInfo : public nsIClassInfo,
> public nsIXPCScriptable
> {

	(no longer active) Reporter
	Comment 6 • 13 years ago

I think comment 3 provides a perfect solution here.  It will result in some false positives, but I think we might want to fix those anyways, so that they wouldn't be real problems in the future.