RFR (S): 8004845: Catch incorrect usage of new and delete during compile time for value objects and stack objects

[Erik Helin]

David,

On 12/12/2012 11:50 AM, David Holmes wrote:
>> class A {
>> private:
>> void operator delete(void *) { }
>> };
>>
>> class B : public A {
>> public:
>> B() { }
>> virtual ~B() { }
>> };
>>
>>  > g++ -c foo.cpp
>> foo.cpp: In destructor ‘virtual B::~B()’:
>> foo.cpp:3:10: error: ‘static void A::operator delete(void*)’ is private
>> foo.cpp:9:18: error: within this context
>> foo.cpp:3:10: error: ‘static void A::operator delete(void*)’ is private
>> foo.cpp:9:18: error: within this context
>> foo.cpp: In destructor ‘virtual B::~B()’:
>> foo.cpp:3:10: error: ‘static void A::operator delete(void*)’ is private
>> foo.cpp:9:18: error: within this context
>> foo.cpp:3:10: error: ‘static void A::operator delete(void*)’ is private
>> foo.cpp:9:18: error: within this context
>
> Okay that kind of makes sense. I was thinking more about them being in
> the same class.
>
> But this just says to me "don't make new/delete private". (Think of them
> like virtual methods on Object - you can redefine them but they are
> always public ;-) )

I believe that the benefit of making new/delete private is that we get 
compile time checking instead of maybe finding out via tests in JPRT (if 
the tests does not traverse a code path with incorrect usage, then we 
won't find it).

In my opinion, if a class has a virtual destructor and inherits from 
_ValueObj or StackObj, then either the destructor shouldn't be virtual 
or the class should not inherit from _ValueObj or StackObj.

As far as I can see, the only use for a _virtual_ destructor in a class 
is if you want to delete a pointer to that class (and make sure that the 
destructor of any class inheriting from that class also gets called). 
 From what I understand, deriving from _ValueObj or StackObj means that 
you don't expect this to happen.

What do you think of this?

Thanks for taking your time,
Erik

> David
>
>> Thanks,
>> Erik
>>
>>> Cheers,
>>> David
>>>
>>> On 12/12/2012 7:34 PM, Erik Helin wrote:
>>>> Hi all,
>>>>
>>>> the webrev is located at:
>>>>
>>>> http://cr.openjdk.java.net/~ehelin/8004845/webrev.00/
>>>>
>>>> Summary:
>>>> When I browsed the code during a code review, I noticed the macro
>>>> VALUE_OBJ_CLASS_SPEC which is defined as ": public _ValueObj" and is
>>>> used to describe that all instances of a given C++ class can never be
>>>> placed on (or removed from) the heap with the operations new or delete.
>>>>
>>>> This is enforced by the class _ValueObj which implements the operators
>>>> new and delete by calling ShouldNotCallThis which will crash the VM.
>>>>
>>>> The operators new and delete in _ValueObj are declared public
>>>> and this change instead declares them as private. This has the effect
>>>> of enforcing the above described restrictions at compile time.
>>>>
>>>> The following three classes inherited from _ValueObj even though they
>>>> made use of either the new or delete operator:
>>>> - RegMask:
>>>> The functions Node::out_regMask and Node::in_regMask are defined as:
>>>> void RegMask &Node::out_regMask() const {
>>>> ShouldNotCallThis();
>>>> return *(new RegMask());
>>>> }
>>>> Both out_regMask and in_regMask are overridden by subclasses and they
>>>> are not meant to be called on a Node object (as can be seen by the
>>>> call ShouldNotCallThis). This is the only place where a RegMask
>>>> object is allocated with new. There are two solutions to this problem:
>>>> - Removed the VALUE_OBJ_CLASS_SPEC for RegMask
>>>> - Implement a work around to be able to keep VALUE_OBJ_CLASS_SPEC for
>>>> RegMask
>>>> This CR presents a work around to the problem instead of removing
>>>> VALUE_OBJ_CLASS_SPEC.
>>>>
>>>> - MemBaseLine
>>>> MemBaseLine has a virtual destructor which is not possible together
>>>> with a private delete operator. Once again, the VALUE_OBJ_CLASS_SPEC
>>>> can be removed, but I chose to make the destructor non-virtual since
>>>> no class currently inherits from MemBaseline.
>>>>
>>>> - AbstractGangTask
>>>> AbstactGangTask has a virtual destructor _and_ there are classes
>>>> inheriting from AbstractGangTask. The problem is that a class that
>>>> defines a virtual destructor in C++ must also have a (working) delete
>>>> operator in scope. The current delete operator is not working (it only
>>>> calls ShouldNotReachHere), therefore my suggestion is to remove
>>>> VALUE_OBJ_CLASS_SPEC from AbstractGangTask.
>>>>
>>>> How could this work before? I haven't checked that thoroughly, but I
>>>> believe that no code calls delete on an AbstractGangTask pointer. If
>>>> this is true, then the destructor in AbstractGangTask could be
>>>> declared non-virtual, but I don't want to take that risk.
>>>>
>>>> This change also changes the new and delete operations of "StackObj"
>>>> to be private (for the same reasons). This did not cause changes in any
>>>> other code.
>>>>
>>>> Testing:
>>>> JPRT
>>>>
>>>> Thanks,
>>>> Erik
>>