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

[David Holmes]

Hi Erik,

On 12/12/2012 11:13 PM, Erik Helin wrote:
> 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).

Agreed - but that is at odds with the use of the virtual destructor ... 
I also have to wonder why that wasn't done in the first place as it 
seems an "obvious" way to ensure they don't get called.

> 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.

I don't think these have to be mutually exclusive in a general sense. 
But it may be we don't need both capabilities in the current cases.

> 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).

Yes. If at any time you are likely to invoke delete on a derived class 
instance through a base class pointer then you want there to be a 
virtual destructor. Lippman's general rule of thumb (C++ Primer 3rd 
edition Section 17.5.5) is that any base class with any virtual 
functions should also have a virtual destructor.

I've had to fix a number of bugs in certain variants of hotspot caused 
by lack of a virtual destructor. And I doubt that the current codebase 
is particularly "clean" in this regard.

>  From what I understand, deriving from _ValueObj or StackObj means that
> you don't expect this to happen.
>
> What do you think of this?

I'm not versed enough in the use of all the different types that derive 
from ValueObj and StackObj to answer that definitively. It seems likely 
to be true to me - particularly for StackObj. But can we say it is never 
needed?

Given these classes must derive from one of the allocation types, the 
only option is to remove the virtual from the destructor, so at a 
minimum you have to establish for these types that a virtual destructor 
will never be needed.

Cheers,
David
-----


> 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
>>>
>