RFR: Lambda: 8009130 JCK lambda test fails with IllegalAccessException
Karen Kinnear
karen.kinnear at oracle.com
Sun Oct 6 13:15:04 PDT 2013
Thanks Serguei.
Fixed those also.
Also renamed create_overpasses to create_defaults_and_exceptions thanks to Coleen's suggestion on the phone.
thanks,
Karen
On Oct 4, 2013, at 2:13 AM, serguei.spitsyn at oracle.com wrote:
>
> 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/
>>
>> 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/20131006/4a1382f3/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list