Thank you, Kelly. -- Chris On Oct 29, 2012, at 11:05 AM, Kelly O'Hair <kelly.oh...@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 <rken...@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.thalin...@oracle.com> >>> Cc: hotspot-...@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 <rken...@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 <rken...@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 >>> >>> >> >
