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

[Erik Helin]

David,

On 12/13/2012 05:51 AM, David Holmes wrote:
> Erik,
>
> Sorry, I've confused myself on a key detail here. I said:
>
> "you have to establish for these types that a virtual destructor will
> never be needed."
>
> but I also said:
>
> "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."
>
> But given that operator delete can NOT be called on these classes you
> can NOT need a virtual destructor. QED. Hence the correct fixes for
> MemBaseline and AbstractGangtask (and subclasses) would be to remove the
> "virtual" from the destructor.

I agree with you. I've uploaded a new webrev at:

http://cr.openjdk.java.net/~ehelin/8004845/webrev.01/

This webrev keeps VALUE_OBJ_CLASS_SPEC and instead makes the destructor 
for the classes AbstractGangTask and YieldingFlexibleGangTask 
non-virtual. The reason for making the destructor of 
YieldingFlexibleGanskTask non-virtual is because it inherits from 
AbstractGangTask.

What do you think of the change now?

Thanks,
Erik

> Thanks,
> David
>
> On 13/12/2012 11:44 AM, 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
>>>>>
>>>