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

[Erik Helin]

Thanks for taking your time and reviewing this change!

Erik

On 12/13/2012 11:30 AM, David Holmes wrote:
> 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
>>>>>>>
>>>>>
>>