RFR: 8000780: make Zero build and run with JDK8

[Christian Thalinger]

Thank you, Kelly.  -- Chris

On Oct 29, 2012, at 11:05 AM, Kelly O'Hair <kelly.ohair at oracle.com> wrote:

> Looks fine.
> 
> -kto
> 
> On Oct 29, 2012, at 10:42 AM, Christian Thalinger wrote:
> 
>> This patch contains a Makefile change:
>> 
>> http://cr.openjdk.java.net/~rkennke/zerojdk8/webrev.03/make/Makefile.udiff.html
>> 
>> -- Chris
>> 
>> Begin forwarded message:
>> 
>>> From: Roman Kennke <rkennke at redhat.com>
>>> Subject: Re: RFR: Make Zero build and run with JDK8
>>> Date: October 17, 2012 4:09:13 PM PDT
>>> To: Christian Thalinger <christian.thalinger at oracle.com>
>>> Cc: hotspot-dev at openjdk.java.net
>>> 
>>> Am Mittwoch, den 17.10.2012, 15:34 -0700 schrieb Christian Thalinger:
>>>> On Oct 17, 2012, at 3:05 PM, Roman Kennke <rkennke at redhat.com> wrote:
>>>> 
>>>>> Am Mittwoch, den 17.10.2012, 14:40 -0700 schrieb Christian Thalinger:
>>>>>> On Oct 16, 2012, at 8:01 AM, Roman Kennke <rkennke at redhat.com> wrote:
>>>>>> 
>>>>>>> Hi Christian,
>>>>>>> 
>>>>>>>>>>>> In the recent weeks I worked on the Zero interpreter, to get it to build
>>>>>>>>>>>> and run with JDK8, and in particular with the latest changes that came
>>>>>>>>>>>> from mlvm (meth-lazy). The following webrev applies to hsx/hotspot-main:
>>>>>>>>>>>> 
>>>>>>>>>>>> http://cr.openjdk.java.net/~rkennke/zerojdk8/webrev.00/
>>>>>>>>>> 
>>>>>>>>>> src/share/vm/asm/codeBuffer.cpp:
>>>>>>>>>> 
>>>>>>>>>> -  if (dest->blob() == NULL) {
>>>>>>>>>> +  if (dest->blob() == NULL && dest_filled != 0x0) {
>>>>>>>>>> 
>>>>>>>>>> Do we really need this when you have this:
>>>>>>>>> 
>>>>>>>>> The above is needed, because the loop above it that initializes
>>>>>>>>> dest_filled is never executed. However, I will change the test to
>>>>>>>>> dest_filled != NULL :-)
>>>>>>>>> 
>>>>>>>>>> static void pd_fill_to_bytes(void* to, size_t count, jubyte value) {
>>>>>>>>>> 
>>>>>>>>>> -  memset(to, value, count);
>>>>>>>>>> +  if ( count > 0 ) memset(to, value, count);
>>>>>>>>>> 
>>>>>>>>>> }
>>>>>>>>> 
>>>>>>>>> Turns out that this is 1. not related to the other change above and 2.
>>>>>>>>> not needed. I'll remove it.
>>>>>>>>> 
>>>>>>>>>> src/share/vm/interpreter/interpreter.cpp:
>>>>>>>>>> 
>>>>>>>>>> -  assert(_entry_table[kind] == _entry_table[abstract], "previous value must be AME entry");
>>>>>>>>>> +  assert(_entry_table[kind] == NULL || _entry_table[kind] == _entry_table[abstract], "previous value must be AME entry or NULL");
>>>>>>>>>> 
>>>>>>>>>> Why did you need that change?
>>>>>>>>> 
>>>>>>>>> Apparently, before the whole table was initialized, and the methodhandle
>>>>>>>>> related entries left as abstract. Now the methodhandle entries are
>>>>>>>>> simply left to NULL. I suppose we could change the assert to 
>>>>>>>>> 
>>>>>>>>> assert(_entry_table[kind] == NULL, "previous value must be NULL");
>>>>>>>>> 
>>>>>>>>> Alternatively, we could fix the code that puts the other entries to also
>>>>>>>>> set the methodhandle entries to AME and go back to the original assert.
>>>>>>>>> What do you think?
>>>>>>>> 
>>>>>>>> TemplateInterpreterGenerator::generate_all sets all MH entries to AME:
>>>>>>>> 
>>>>>>>> // method handle entry kinds are generated later in MethodHandlesAdapterGenerator::generate:                                                                  
>>>>>>>> for (int i = Interpreter::method_handle_invoke_FIRST; i <= Interpreter::method_handle_invoke_LAST; i++) {                                                      
>>>>>>>> Interpreter::MethodKind kind = (Interpreter::MethodKind) i;                                                                                                  
>>>>>>>> Interpreter::_entry_table[kind] = Interpreter::_entry_table[Interpreter::abstract];                                                                          
>>>>>>>> }                                                                                                                                                              
>>>>>>>> 
>>>>>>>> You need similar code in Zero.
>>>>>>> 
>>>>>>> Alright, I extracted this piece of code into a separate protected method
>>>>>>> void AbstractInterpreterGenerator::initializeMethodHandleEntries() and
>>>>>>> call it both from templateInterpreter and cppInterpreter to initialize
>>>>>>> the method handle entries to AME.
>>>>>>> 
>>>>>>> This new webrev also reverts this:
>>>>>>> 
>>>>>>>>> static void pd_fill_to_bytes(void* to, size_t count, jubyte value) {
>>>>>>>>>> 
>>>>>>>>>> -  memset(to, value, count);
>>>>>>>>>> +  if ( count > 0 ) memset(to, value, count);
>>>>>>>>>> 
>>>>>>>>>> }
>>>>>>>> 
>>>>>>> 
>>>>>>> .. and changes the 0x0 to NULL here:
>>>>>>> 
>>>>>>> 
>>>>>>>>> src/share/vm/asm/codeBuffer.cpp:
>>>>>>>>>> 
>>>>>>>>>> -  if (dest->blob() == NULL) {
>>>>>>>>>> +  if (dest->blob() == NULL && dest_filled != 0x0) {
>>>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>>>> I tried JRuby a little more and verified that it's actually using +indy,
>>>>>>> and it works all well.
>>>>>>> 
>>>>>>> http://cr.openjdk.java.net/~rkennke/zerojdk8/webrev.01/
>>>>>> 
>>>>>> It seems this webrev contains the old changes.
>>>>> 
>>>>> Arg! I did the changes in my hotspot-comp tree then made the webrev in
>>>>> my hotspot-main :-) Here's the correct one:
>>>>> 
>>>>> http://cr.openjdk.java.net/~rkennke/zerojdk8/webrev.02/
>>>> 
>>>> That looks better.  The only thing is we don't use camel-case for methods:
>>>> 
>>>> +  void initializeMethodHandleEntries();
>>>> 
>>>> Could you change it to:
>>>> 
>>>> +  void initialize_method_handle_entries();
>>>> 
>>>> I would do it myself but I cannot verify that I didn't break Zero.  I really should set up a build environment for Zero...
>>> 
>>> Here it comes:
>>> 
>>> http://cr.openjdk.java.net/~rkennke/zerojdk8/webrev.03/
>>> 
>>> I built both Zero and normal debug_build successfully.
>>> 
>>> Cheers,
>>> Roman
>>> 
>>> 
>> 
>