RFR: Lambda: 8009130 JCK lambda test fails with IllegalAccessException
Karen Kinnear
karen.kinnear at oracle.com
Thu Oct 3 14:10:04 PDT 2013
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/
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 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/
>>
>> 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/
>>> 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/7041000d/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list