RFR: Lambda: 8009130 JCK lambda test fails with IllegalAccessException
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Oct 3 23:13:27 PDT 2013
Looks good, thank you for taking the suggestions!
I hope, it is easy to fix a couple of minors in
src/share/vm/prims/methodHandles.cpp:
- Extra space after the first parameter at the lines: 193, 210
Thanks,
Serguei
On 10/3/13 2:10 PM, Karen Kinnear wrote:
> Same comment I made in response to Yumin's review suggestions (so if
> you already read this :-)
>
> Folks -
>
> New webrev that includes feedback from Lois, Yumin and Sergeui:
> http://cr.openjdk.java.net/~acorn/8009130.4/webrev/
> <http://cr.openjdk.java.net/%7Eacorn/8009130.4/webrev/>
>
> I also removed the defaultMethods.cpp change I had made in the last
> update. I heard back from
> the JVMS spec representative, and we are going to need further
> negotiations, so I will work
> that one issue and one new JCK test in a future changeset.
>
> Latest passes all the smaller tests and tests that have given me
> issues in the past. Kicking off
> longer regression testing in parallel with review.
>
> Yumin,
>
> Sergeui,
>
> Thank you for the review. Responses embedded below. I took all of your
> suggestions - very helpful.
>
> thanks,
> Karen
>
> On Oct 2, 2013, at 8:46 PM, serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com> wrote:
>
>> Hi Karen,
>>
>> Huge work!
>>
>> Below are the comments I have so far.
>>
>> src/share/vm/classfile/classFileParser.cpp
>> No comments
>>
>> src/share/vm/classfile/defaultMethods.cpp
>>
>> Some questions on the fragment:
>>
>> 937 Method* selected = NULL;
>> 938 Method* m = NULL;
>> 939 if (method->has_target()) {
>> 940 selected = method->get_selected_target();
>> 941 if (selected->method_holder()->is_interface()) {
>> 942 defaults.push(selected);
>> 943 }
>> 944 } else if (method->throws_exception()) {
>> 945 max_stack = assemble_method_error(&bpool, &buffer,
>> method->get_exception_name(), method->get_exception_message(), CHECK);
>> 946 }
>> 947 if (max_stack != 0) {
>> 948 AccessFlags flags = accessFlags_from(
>> 949 JVM_ACC_PUBLIC | JVM_ACC_SYNTHETIC | JVM_ACC_BRIDGE);
>> 950 m = new_method(&bpool, &buffer, slot->name(),
>> slot->signature(),
>> 951 flags, max_stack, slot->size_of_parameters(),
>>
>> The line #942:
>> Previously the assemble_method_error(0 returned parameter_count for
>> the max_stack value.
>> Nothing is returned from the defaults.push(selected).
>> But the max_stack value is checked later at the line #947.
>> Is it Ok?
> Good point. I could simplify this now that the only code that goes
> through the new_method logic
> and overpasses.push is the code that went throught the else if
> method->throws_exception.
> I did so.
>
>>
>> What was the motivation to change the definition scope for locals
>> 'selected' and 'm' (lines 937,938)?
>> Would it better to keep the original locality?
> There used to be multiple conditionals before the generic handling was
> removed. I will reduce the
> scope.
>>
>>
>> src/share/vm/code/dependencies.cpp
>> No comments
>>
>>
>> src/share/vm/interpreter/linkResolver.cpp
>>
>> 243 // returns first instance method
>> 244 // Looks up method in classes, then looks up local default methods
>> 245 void
>> LinkResolver::lookup_instance_method_in_klasses(methodHandle& result,
>> KlassHandle klass, Symbol* name, Symbol* signature, TRAPS) {
>> . . .
>> 253 if (result.is_null() &&
>> InstanceKlass::cast(klass())->default_methods() != NULL) {
>> 254 result =
>> methodHandle(InstanceKlass::find_method(InstanceKlass::cast(klass())->default_methods(),
>> name, signature));
>> 255 assert(result.is_null() || !result->is_static(), "no static
>> defaults");
>> 256 }
>> 257 }
>>
>> The assert message at the #255 looks incorrect, should it be: "no
>> instance defaults" ?
> It is trying to say "static defaults not allowed". I will fix it.
>>
>> 260 methodHandle resolved_method, TRAPS) {
>>
>> Wrong indentation
> Changed.
>>
>> 278 vtable_index = vt->index_of_miranda(name, signature);
>>
>> An extra space after the '='
> Fixed.
>>
>> 658 if (resolved_method->method_holder()->is_interface() &&
>> 659 !resolved_method->is_abstract()) {
>>
>> Use: resolved_method->is_default_method() ?
> Thank you. I cleaned them all up. And caught a cut-and-paste of the wrong
> check for is_overpass while double-checking this. Good catch.
>>
>>
>> src/share/vm/interpreter/linkResolver.hpp
>> No comments
>>
>>
>> src/share/vm/memory/heapInspection.hpp
>> No comments
>>
>> src/share/vm/oops/instanceKlass.cpp
>>
>> 2592 - Wrong indentation
>> 2820, 2828 - Wrong alignment
> Fixed.
>>
>> 2823 for(int i = 0; i < method_array->length(); i++) {
>>
>> Missed space after 'for'
> Fixed. And fixed the one I copied from.
>>
>>
>> src/share/vm/oops/instanceKlass.hpp
>>
>> 370 Array<int>* default_vtable_indices() const
>> 371 { return
>> _default_vtable_indices; }
>>
>> Would it better to combine the lines?
> Yes. Sorry, my editor does that for me, I have to fix that again.
>>
>>
>> 934 trace_name_printed);
>>
>> Wrong indentation
> Fixed.
>>
>>
>> src/share/vm/oops/klassVtable.cpp
>>
>> 363 KlassHandle target_klass(THREAD,target_method()->method_holder());
>>
>> Space is missed
> Good thing they don't pay me as a typist.
>>
>> 370 Symbol* target_classname = target_klass->name();
>>
>> An extra space
> Fixed.
>>
>> 380-382, 434 - Wrong indentation
> Fixed .
>>
>> Use method->is_default_method() in the fragments below:
>>
>> 266 if (super_method->method_holder()->is_interface() &&
>> 267 !super_method->is_abstract()) {
>> ...
>> 272 if (target_method->method_holder()->is_interface() &&
>> 273 !target_method->is_abstract()) {
>> ...
>> 445 if (super_method->method_holder()->is_interface() &&
>> 446 !super_method->is_abstract()) {
>> ...
>> 454 if (target_method->method_holder()->is_interface() &&
>> 455 !target_method->is_abstract()) {
>> ...
>> 474 if (super_method->method_holder()->is_interface() &&
>> 475 !super_method->is_abstract()) {
>> ...
>> 483 if (target_method->method_holder()->is_interface() &&
>> 484 !target_method->is_abstract()) {
>
>> ...
>> 546 if (target_method()->method_holder() != NULL &&
>> 547 target_method()->method_holder()->is_interface() &&
>> 548 !target_method()->is_abstract() ) {
>> ...
>> 766 if (meth->method_holder()->is_interface() &&
>> 767 !meth->is_abstract()) {
>> ...
>> 867 if (m->method_holder()->is_interface() &&
>> 868 !m->is_abstract()) {
>> ...
>> 1089 if (target()->method_holder()->is_interface() &&
>> 1090 !target()->is_abstract()) {
>> ...
>> 1169 if (m->method_holder()->is_interface() &&
>> 1170 !m->is_abstract()) {
> Done. Coleen will like this - thanks!
>>
>>
>> src/share/vm/oops/klassVtable.hpp
>> No comments
>>
>> src/share/vm/oops/method.cpp
>>
>> 1398 void Method::sort_methods(Array<Method*>* methods, bool
>> idempotent, bool idnum) {
>>
>> A renaming suggestion: idnum => init_idnums, or set_idnums, etc.
> Used set_idnums. Much clearer, and changed in method.hpp also.
>>
>> src/share/vm/oops/method.hpp
>> No comments
>>
>> src/share/vm/prims/jni.cpp
>>
>> 1596 m =
>> (InstanceKlass::find_method(InstanceKlass::cast(klass())->default_methods(),
>> name, signature));
>>
>> An extra space
> Fixed.
>>
>>
>> src/share/vm/prims/jvmtiRedefineClasses.cpp
>> No comments
>>
>> src/share/vm/prims/methodHandles.cpp
>>
>> 184 case CallInfo::itable_call:
>> ...
>> 190 if (TraceInvokeDynamic) {
>> 191 ResourceMark rm;
>>
>> 203 case CallInfo::vtable_call:
>> ...
>> 207 if (TraceInvokeDynamic) {
>>
>> Is a HandleMark missed after the line #207?
> I assume you mean ResourceMark, and yes - it is. Fixed.
> Also fixed check for is_default.
>>
>>
>> src/share/vm/runtime/reflectionUtils.cpp
>>
>> 31 classes_only, bool walk_defaults) {
>> 54 _base_class_get_defaults = true;
>> 55 _klass = _base_klass;
>> 56 _defaults_checked = true;
>>
>> Wrong indentation
> Fixed.
>>
>> src/share/vm/runtime/reflectionUtils.hpp
>> No comments
>>
>> src/share/vm/runtime/vmStructs.cpp
>> No comments
>>
>> Thanks,
>> Serguei
>>
>>
>> On 10/2/13 10:24 AM, Karen Kinnear wrote:
>>> I updated the webrev - just the defaultMethods.cpp. This failed a
>>> jck resolveMethod00705m2 test
>>> so I am negotiating a minor specification correction which I think
>>> is the behavior we want and
>>> matches the updated code (and creates an AME for the result of
>>> default method resolution as
>>> Keith did originally if the resultant method is abstract).
>>>
>>> http://cr.openjdk.java.net/~acorn/8009103.3/webrev/
>>> <http://cr.openjdk.java.net/%7Eacorn/8009103.3/webrev/>
>>>
>>> thanks,
>>> Karen
>>>
>>>
>>>
>>> On Oct 1, 2013, at 10:01 PM, Karen Kinnear wrote:
>>>
>>>>
>>>> Please review: JCK lambda test fails with IllegalAccessException
>>>> Also fixes: 8013085: Default method bridges have incorrect class
>>>> loading constraints
>>>> 8025288: Two JCK runtime tests failed with IllegalAccessError
>>>>
>>>> webrev: http://cr.openjdk.java.net/~acorn/8009130.2/webrev/
>>>> <http://cr.openjdk.java.net/%7Eacorn/8009130.2/webrev/>
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8009130
>>>>
>>>> Summary:
>>>> See bug report for further details.
>>>>
>>>> After several prototypes, current approach is to simplify how
>>>> default methods are handled by:
>>>> 1) not using overpasses (vm generated bridges) for invoking default
>>>> methods, not using a second
>>>> step invokespecial
>>>> - this removes the problem with invokespecial's constant pool
>>>> lookup that requires class access
>>>> (of the original invocation's "selected" method)
>>>> 2) creating default_methods_array in the inheriting instanceKlass
>>>> with public default methods (and default_vtable_indices)
>>>> 3) search order for method and interface method resolution (to
>>>> match the 0.6.3+ JVMS spec update):
>>>> local class methods, super class methods, default methods,
>>>> transitive interfaces
>>>> 4) adding the default method to the vtable directly, with the
>>>> default method having the correct
>>>> method holder, which allows the access permission and loader
>>>> constraint tests run on
>>>> the resolved method to work on the correct method holder/class
>>>> loader
>>>> 5) overpasses are still used for errors detected during default
>>>> method processing, such
>>>> as ICCE for conflicts with default methods, and abstract method
>>>> error for no candidate methods found,
>>>> these are still added to the inheriting method's local method
>>>> array and constant pool at this time
>>>>
>>>> My goal is to check this in by the end of the week for next
>>>> Tuesday's hotspot-rt deadline.
>>>>
>>>> thanks,
>>>> Karen
>>>>
>>>> Tests run: (64 bit linux)
>>>> 1. 8009130 reported bug
>>>> 2. 8009103 jck tests:
>>>> lang/INTF/intf024/intf02402m01/intf02402m01.html
>>>> lang/INTF/intf024/intf02402m11/intf02402m11.html
>>>> lang/CLSS/clss475/clss47501m21/clss47501m21
>>>> lang/CLSS/clss475/clss47501m211/clss47501m211
>>>> 3. 8013085: Loader Constraint example
>>>> 4. default methods tests
>>>> 5. intfbug
>>>> 6. jtreg jdk: filteroptest
>>>> 7. reflection modeling tests (Reflect2, ReflectInvokeprivatesmall)
>>>> 8. jcmd <pid> GC.class_stats
>>>> -- tests which ran prior to latest hotspot-rt merge and will rerun
>>>> in parallel with code review
>>>> 9. vm.quick.testlist
>>>> 10. vm.mlvm.testlist
>>>> 11. jtreg jdk java.util, java.lang
>>>> 12. jck vm
>>>> 13. jck lang
>>>> 14. nsk.hprof.testlist
>>>> 15. nsk.quick-jvmti.testlist
>>>> 16. jck api/java_util/TreeSet/ParallelStream.html
>>>> 17. jprt -stree .
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20131003/874936f7/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list