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

[David Holmes]

On 13/12/2012 6:53 PM, Erik Helin wrote:
> 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?

Looks fine to me. The empty destructors for the GangTasks seems 
superfluous anyway.

Sorry for the confusion.

David
-----


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