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

[David Holmes]

On 12/12/2012 8:16 PM, Erik Helin wrote:
> Hi David,
>
> thanks for the review! See comments inline.
>
> On 12/12/2012 11:01 AM, David Holmes wrote:
>> Doesn't the change to AbstractGangTask break the rule that all VM types
>> must inherit from one of the allocation classes ?
>
> I wasn't aware of this rule, thanks for bringing it up. Is the class
> CheapObj the one that other classes should derive from if it is possible
> to put instances of the class on the heap?

Yes. See the comments in allocation.hpp.

> On 12/12/2012 11:01 AM, David Holmes wrote:
>> I'm not sure that your reasoning about virtual destructors and operator
>> delete is correct. It is possible to call destructors directly without
>> deleting the object. Need to think more about this ... and see what
>> others (Hi Coleen!) have to say.
>
> I'm not completely sure why you can't have virtual destructor and
> private delete operator. The following small sample shows that you can't
> have private delete operator and a virtual destructor:
>
> 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 ;-) )

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
>