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

[Coleen Phillimore]

This looks good, thank you for making this change.
Coleen

On 12/14/2012 1:08 AM, Erik Helin wrote:
> Coleen,
>
> On 12/13/2012 04:15 AM, Coleen Phillimore wrote:
>> I think this looks reasonable, although someone from GC should comment
>> on the AbstractGangTask's real allocation type.
>
> what do you think of the latest webrev (which keeps the allocation 
> type for AbstractGangTask):
>
> 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.
>
> This was changed because I got some great feedback from David:
>
> On 12/13/2012 05:51 AM, David Holmes wrote:
> > 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.
>
> Thanks,
> Erik
>
>> 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
>>>>>>
>>>>
>>
>