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 >> >> >
