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

[David Holmes]

Just an update on the OSX problem - seems we are exporting every single 
symbol in libjvm.dylib! Investigating further ...

David

On 7/05/2013 4:26 PM, David Holmes wrote:
> 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
>>>>>>
>>>>>
>>>>
>>>
>>