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

[Erik Helin]

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?

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

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