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

[Coleen Phillimore]

Erik,
I think this looks reasonable, although someone from GC should comment 
on the AbstractGangTask's real allocation type.
Thanks,
Coleen

On 12/12/2012 8:44 PM, David Holmes wrote:
> 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
>>>>
>>