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

[Erik Helin]

Thanks!

Erik

On 12/17/2012 02:40 PM, Coleen Phillimore wrote:
>
> 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
>>>>>>>
>>>>>
>>>
>>
>