RFR: 8012902: remove use of global operator new - take 2

[Coleen Phillimore]

David,

I think Yumin has some additional operator new[] calls that he hasn't 
fixed yet so I'm expecting another webrev.   I agree it's a more 
complicated change than originally thought, but it's progressing so I 
don't think quitting now is a good idea.  The main change helps resolve 
a fundamental problem that has recently broken the VM.  Yumin's change 
makes it harder to do this.

On 04/29/2013 09:00 AM, David Holmes wrote:
> Hi Yumin,
>
> I think we need to pull back on this until we can address the broader 
> issues:
>
> a) there are a number of classes that don't obey the rules about 
> extending one of the allocation types

This will always be the case.   This shouldn't be a blocker.
>
> b) adding additional operator new/new[] for explicit C-Heap usage 
> conflicts with the use of the existing macros/functions documented in 
> allocation.hpp (I still think I prefer NEW_C_HEAP_OBJ + global 
> placement new to invoke the correct constructor). If you stick with 
> your approach then the documentation in allocation.hpp needs rewriting.

I think the documentation in allocation.hpp describes how we want this 
to work.   The exceptional cases should be documented where they exist. 
   I don't really have an opinion whether NEW_C_HEAP_OBJ vs. adding new 
and new[] to the exceptional classes is better.  Both have their pros 
and cons.

>
> c) there seem to be other global array allocations still lurking
>

Yes, the ones we know about should be fixed.   And do due diligence to 
find them all.  The purpose of the assert is to find any that might leak 
in after this exercise.

> d) the effect of the hotspot global operator new on the other 
> libraries needs to be better understood and dealt with. If I 
> understand your fix as it stands you will abort in product mode, and 
> warn in debug - yet we know this problem exists so this will simply 
> force an abort. I would not expect to see the ShouldNotReachHere() 
> variants.
>

I think the logic is reversed in the new code.  #ifndef 
CATCH_OPERATOR_NEW_USAGE should just revert to the global operators 
new/new[]/delete/delete[], ie be empty.   The code under CATCH_* should 
assert and return AllocateHeap() in product quietly. 
ShouldNotReachHere() gives a fatal error in product mode too, so it 
should be avoided.

> This is without a doubt quite a mess, so I think we need to proceed 
> very cautiously.

Definitely.

Coleen

>
> David
>
> PS. I'm going to be on vacation from about 18 hours from now. Back 
> next week - no email.
>
>
> On 26/04/2013 4:48 PM, Yumin Qi wrote:
>> HI, all
>>
>>    This bug is filed after this bug
>>    8010992: Remove calls to global ::operator new[] and new
>>    was anti-delta'ed due to testing failure on JVMTI. Now it takes both.
>>
>>    http://cr.openjdk.java.net/~minqi/8012902/webrev/
>>
>>    ObjectMonitor is an class which should not inherit from any class,
>> but is has subclass in Jvmti, JvmtiRawMonitor. In previous changeset,
>> ObjectMonitor is created through NEW_C_HEAP_OBJ/ARRAY macros, but
>> JvmtiRawMonitor was missed. This is why when it is 'new'ed failed on
>> disabled global operator new in debug mode. To prevent using more
>> complex form of marco NEW_C_HEAP_OBJ/ARRAY operator new and new[] added
>> to ObjectMonitor so JvmtiRawMonitor will inherit for them.
>>
>>    HandleMark should be a StackObj but we create the first one in Thread
>> ctor in heap, so it did not have StackObj as parent. Added oerator new
>> and new [] to it though in fact it is  used as StackObj. Same treatment
>> for MemRegion too, it is an _ValueObj (in Linux, it did not inherit from
>> any). Adding operator new and new[] is mainly avoid using macro
>> involving alloc/dealloc and ctor/dtor complexity.
>>
>>    The make files, vm.make and fastdebug.make  changed under BSD is for
>> one problem which is not clear: jdk code called operator new in hotspot
>> library. This only happens on Darwin. Exposure of
>> CATCH_OPERATOR_NEW_USAGE giving an warning when first time global
>> operator new and new [] called.
>>
>>    Tested with JPRT, vm.quick, jtreg.
>>
>>    Thanks
>>    Yumin