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

[David Holmes]

Hi Yumin,

First thanks for persevering here. :)

I still have a few queries/concerns:

Why doesn't KlassHandle extend one of the allocation types?

I also thought we could now have HandleMark extend StackObj as desired. 
But note that by adding the operator new overrides to the class, rather 
than using NEW_C_HEAP+placement-new at the call site, people can 
incorrectly define additional C-heap instances beyond the one special 
case that we had to handle. :(

I don't understand why these overrides:

! #ifndef ALLOW_OPERATOR_NEW_USAGE
   void* operator new(size_t size){
!   assert(false, "Should not call global operator new");
!   return 0;
   }

use assert instead of guarantee? On product builds we will simply return 
0 and have a null pointer that will lead to a crash if not checked (and 
chances are that code incorrectly using the global new is not expecting 
to have to check the return value).

I'm still concerned that the JDK library uses the libjvm's operator new. 
This indicates some kind of linkage error to me. I'm trying to 
investigate this further.

Fianlly where do we stand with all the other classes that don't extend 
an allocation base type? Will we file a follow up CR to convert them all?

Thanks,
David

On 4/05/2013 3:15 AM, Yumin Qi wrote:
> Sorry  the link is not right, the right link:
> http://cr.openjdk.java.net/~minqi/8012902/webrev2/
>
> Thanks
> Yumin
> On 5/3/2013 10:08 AM, Yumin Qi wrote:
>> Coleen,
>>
>>   Thanks, new webrev:
>> http://cr.openjdk.java.net/~minqi/8012902/webrev2
>>
>> Yumin
>>
>> On 5/2/2013 2:41 PM, Coleen Phillimore wrote:
>>>
>>> I think I hit send too soon.
>>>
>>> http://cr.openjdk.java.net/~minqi/8012902/webrev1/src/share/vm/runtim/unhandledOops.hpp.cdiff.html
>>>
>>> I think this should not be mtOther, but mtThread since this belongs
>>> to the thread.
>>>
>>> Other than these small things, I think it looks good.
>>>
>>> Thanks,
>>> Coleen
>>>
>>>
>>> On 05/01/2013 11:20 AM, Yumin Qi wrote:
>>>> Hi,  the link in fact directs to old webrev, you need to copy the
>>>> text in browser to open the new link, or follow
>>>>
>>>> http://cr.openjdk.java.net/~minqi/8012902/webrev1/
>>>>
>>>> Thanks
>>>> Yumin
>>>>
>>>> On 5/1/2013 7:56 AM, Yumin Qi wrote:
>>>>> Hi,
>>>>>
>>>>>   This revised version replace  CATCH_OPERATOR_NEW_USAGE with
>>>>> ALLOW_OPERATOR_NEW_USAGE.  For platforms on which operator new
>>>>> called from other source other than jvm, define this macro to
>>>>> enable global operator new instead.
>>>>>
>>>>>   Also fixed KlassHandle creation in ciReplay.cpp, that still use
>>>>> global operator new.
>>>>>
>>>>> http://cr.openjdk.java.net/~minqi/8012902/webrev1/
>>>>> <http://cr.openjdk.java.net/%7Eminqi/8012902/webrev/>
>>>>>
>>>>>   Tested: vm.quick.testlist, jtreg, runThese, JPRT
>>>>>
>>>>> Thanks
>>>>> Yumin
>>>>>
>>>>> *
>>>>>
>>>>>
>>>>> *
>>>>> On 4/29/2013 10:08 AM, Yumin Qi wrote:
>>>>>> Coleen and David,
>>>>>>
>>>>>> On 4/29/2013 6:36 AM, Coleen Phillimore wrote:
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>> Yes, I think if we stop here, this one will not be revisited for
>>>>>> long time --- more dangerous code will be added then. To prevent
>>>>>> this from happening, better to stop them as soon as we can.
>>>>>>> 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.
>>>>>> Those classes (most of them) are used as stack obj, currently did
>>>>>> not find any used as heap obj. For VALUE_OBJ_CLASS_SPEC, since it
>>>>>> is empty on linux, every class which take it as parent will come
>>>>>> from nothing that is similar to those classes not obey the rules
>>>>>> --- This is why I asked if we should make it _ValueObj on linux
>>>>>> but you think that will add more bytes to the objects.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>> Using macro and calling constructors need carefulness, which
>>>>>> caused too much concern, so if the use case is simple, I would
>>>>>> like to use macros, but if it is complex, implementing operator
>>>>>> new is preferable I think.
>>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>> I found one more case using nm on linux. Do you know what it will
>>>>>> be on solaris? I tried to code a small program, but could not
>>>>>> locate 'new' in the output. For shared code, linux will output all
>>>>>> the unsettled operator new, what I am concerning here is some
>>>>>> platform specific code.
>>>>>>
>>>>>>>
>>>>>>>> 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.
>>>>>> Yes, the logic here now is on macosx (currently I did not find any
>>>>>> other platform the global operator new switched to jvm 'new'),
>>>>>> only gives warnings for the operator first time called, and return
>>>>>> AllocateHeap, no stop here.
>>>>>>
>>>>>> BTW, when I tried to test on Windows to find if they will fail on
>>>>>> new[] (a lot of new[] used in awt, swing etc), the demo did not
>>>>>> crash on new[] but there is a failure in awt code, Hashtable.cpp
>>>>>>
>>>>>>
>>>>>>
>>>>>> This comes not only my fastdebug version, but all other debug
>>>>>> versions on Windows. It is an awt related error.  It happens on my
>>>>>> desktop whenever you type characters in input area of the testing
>>>>>> program interface.
>>>>>>
>>>>>> Thanks
>>>>>> Yumin
>>>>>
>>>>
>>>
>>
>