RFR: Lambda: 8009130 JCK lambda test fails with IllegalAccessException
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Oct 2 17:46:40 PDT 2013
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?
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?
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" ?
260 methodHandle resolved_method, TRAPS) {
Wrong indentation
278 vtable_index = vt->index_of_miranda(name, signature);
An extra space after the '='
658 if (resolved_method->method_holder()->is_interface() &&
659 !resolved_method->is_abstract()) {
Use: resolved_method->is_default_method() ?
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
2823 for(int i = 0; i < method_array->length(); i++) {
Missed space after 'for'
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?
934 trace_name_printed);
Wrong indentation
src/share/vm/oops/klassVtable.cpp
363 KlassHandle target_klass(THREAD,target_method()->method_holder());
Space is missed
370 Symbol* target_classname = target_klass->name();
An extra space
380-382, 434 - Wrong indentation
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()) {
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.
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
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?
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
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/20131002/4156f8e5/attachment.html
More information about the hotspot-runtime-dev
mailing list