C++ delete operator undefined behavior

[Kim Barrett]

> On Feb 9, 2019, at 1:36 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> 
> On 2/9/19 10:16 AM, Kim Barrett wrote:
>>> On Feb 9, 2019, at 3:42 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>> 
>>> I am getting a "ud2" instruction on the Mac for a 'delete' expression. According to this page, it's clang flagging an undefined behavior:
>>> 
>>> https://stackoverflow.com/questions/21529308/why-does-clang-generate-ud2-opcode-on-osx
>>> 
>>> My code looks like this:
>>> 
>>> class MetaspaceClosure {
>>>   class Ref : public ResourceObj {
>>>   ...
>>>   };
>> You have changed Ref to derived from ResourceObj; in current mainline it has
>> no baseclass.  Ref is abstract, but in mainline doesn’t have a virtual destructor
>> (which is a bug; it probably should also be non-copyable).  delete through a
>> pointer to a base class (which Ref* certainly is, since Ref is abstract) that
>> doesn’t have a virtual destructor is guaranteed slicing, e.g. UB.  So there you
>> go.  Add the missing (empty) public virtual destructor for Ref (and for extra
>> credit, poison copy and assign).
> 
> Hi Kim,
> 
> Thanks for the explanation. It starts to make sense now :-)
> 
> I am not familiar with "poisoning". Are there any examples in the hotspot code?

One of the ways to make a class "non-copyable" in C++98 is to declare
its copy constructor and assignment operator private, and not provide
a definition for either.  That way, external attempts to invoke them
will fail at cmpiletime because they are inaccessible (because they
are private), and internal or friend attempts will (usually) fail at
linktime because they aren't defined.  That is, those operations are
"poisoned"; attempts to call them will error (hopefully before runtime).

So in the case of Ref, just add in the private part of the class

  // Noncopyable.
  Ref(const Ref&);
  Ref& operator=(const Ref&);

An existing (recent) example is TaskTerminator (in gc/shared/taskqueue.hpp).

In C++11, giving them deleted definitions (either public or private)
is better.

  Ref(const Ref&) = delete;
  Ref& operator=(const Ref&) = delete;

We should consider having a macro to make this simpler and more
obvious, e.g. something like

#define DECLARE_NONCOPYABLE(Name) \
  Name(const Name&); Name& operator=(const Name&);

which could be updated someday to delete the definitions.

> Also, why would clang stop putting in the 'ud2' instruction after I switched to subclassing from CHeapObj?

No immediately obvious (to me) reason.  One difference between the two
cases is that ResourceObj has a (debug-only) user-defined destructor,
whereas in the CHeapObj case Ref would have a trivial destructor.  But
that doesn't change the fact that deleting through Ref* is slicing off
the derived class's (unknown) destructor.  So, no, I don't know why
there is a difference.  It may be that clang is being conservative (or
buggy) about when to make that transformation.