(M) RFR: 8081800: AbstractMethodError when evaluating a private method in an interface via debugger

[Daniel D. Daugherty]

 > Just want to verify if you looked at the renaming webrev as well?

Yes. Did separate reviews since I was about to leave hospital wifi
when I finished this one. :-)

 > Webrev updated in place with your's and Karen's comments addressed.
 > http://cr.openjdk.java.net/~dholmes/8081800/webrev.hotspot/

Re-reviewed on a bigger screen :-)

I'm good with the updates and didn't find anything new...

Dan



On 9/28/16 9:22 PM, David Holmes wrote:
> Hi Dan,
>
> Thanks for the review.
>
> Just want to verify if you looked at the renaming webrev as well?
>
> More inline ...
>
> On 29/09/2016 12:43 PM, Daniel D. Daugherty wrote:
>> On 9/28/16 5:50 AM, David Holmes wrote:
>>> Warning: long discussion, but in the end relatively simple code
>>> change. :)
>>>
>>> Thanks to Karen for explaining vtables and itables and pointing out
>>> various tests to be executed; Coleen for the discussions around
>>> interface initialization and terminology, and pointing me to simple
>>> redefinition tests; and Stas Lukyanov for indicating the right JCK
>>> tests to run.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8081800
>>>
>>> Background:
>>>
>>> In JDK 8 default and static interface methods were added to the Java
>>> language. Private interface methods were also considered, and support
>>> in the VM was added, but were dropped due to schedule pressure. In
>>> Java 9 private interface methods have now been enabled at the
>>> language-level and because the VM already supported invokespecial for
>>> private interface methods, the direct language use, and core
>>> reflection use, of these methods works fine. However, what was
>>> overlooked (and which the test case in this bug report highlighted)
>>> was that the other interfaces to the VM (JNI, JDWP, JDI, JVM TI) had
>>> not been updated to account for private interface method, and such
>>> usage did not work.
>>>
>>> The updates to the specifications, plus some small JDI/JDWP related
>>> code changes are being handled under:
>>>
>>> JDK-8165827 Support private interface methods in JDI, JDWP and JDB
>>> https://bugs.openjdk.java.net/browse/JDK-8165827
>>>
>>> This bug, although originally discovered via JDI/JDB, is being used to
>>> fix the underlying mechanics in the VM used by the JNI layer - after
>>> which the test in the bug report will run fine.
>>>
>>> Problem:
>>>
>>> Because private interface methods are only invocable via invokespecial
>>> (the JVMS goes to great lengths to explicitly prohibit all other
>>> invocation forms on them) they are in essence always statically bound
>>> and don't require lookup in either itables (for invokeinterface) or
>>> vtables (for general lookup). However, JNI etc, uses itables/vtables
>>> to perform their invocations, and what we got was behaviour where the
>>> private interface methods did have an itable entry, which made them
>>> appear to be regular abstract interface methods, and so they ended up
>>> with initial vtable entries that were set to throw AbstractMethodError
>>> on invocation (normally those vtable entries would be replaced by the
>>> concrete methods in the implementing class) - and that is what was
>>> observed via JDB. It turns out that depending on whether a class
>>> method with the same signature existed in a class implementing the
>>> interface, that you could also get IllegalAccessError (a path that
>>> actually crashes the debug VM due to an assertion failure in jni.cpp!).
>>>
>>> Solution:
>>>
>>> Private interface methods do not need, and should not have, an itable
>>> entry - they are never invoked via invokeinterface. (Thanks Karen)
>>>
>>> Private interface methods can always be statically bound -
>>> Method::can_be_statically_bound() should return true, and their vtable
>>> entry should be Method::nonvirtual_vtable_index.
>>>
>>> Private interface methods are not default methods and
>>> Method::is_default_method() should return false. There is a
>>> terminology confusion here that I address further down.
>>>
>>> See the bug report for a detailed analysis of all the places where
>>> changing these Method properties may have had an affect.
>>>
>>> Main webrev:
>>>
>>> http://cr.openjdk.java.net/~dholmes/8081800/webrev.hotspot/
>>
>> make/test/JtregNative.gmk
>>     No comments.
>>
>> src/share/vm/classfile/defaultMethods.cpp
>>     L917:         ResourceMark rm;
>>         Not really your problem, but since this function takes TRAPS 
>> param,
>>         can the 'rm' be constructed with 'rm(THREAD)'?
>
> Fixed
>
>>
>>     L932:           assert (!selected->is_private(), "pushing private
>> interface method as default");
>>         nit: please delete space before '('.
>
> Fixed
>
>> src/share/vm/interpreter/linkResolver.cpp
>>     No comments.
>>
>> src/share/vm/oops/klassVtable.cpp
>>     L404:     // by an inheriting class
>>         nit: did you want to add a period here also?
>
> Fixed.
>
>>     L405:     // Private interface methods have no itable index and are
>> always invoked nonvirtually
>>         nit: your new sentence should end with a period.
>
> Fixed with update from Karen.
>
>>     L603: !target_method()->is_abstract() ) {
>>         nit: Not really your problem, but can you delete the space
>> before ')'.
>
> Fixed.
>
>>     L615:   // Specification interpretation since classic has private
>> methods not overriding
>>         nit: the reformatted send should end with a period.
>
> Added the period but I don't think that collection of words actually 
> forms a sentence :)
>
>> src/share/vm/oops/method.cpp
>>     L280:     assert((is_native() && bci == 0) || (!is_native() && 0 <=
>> bci && bci < code_size()),
>>         nit: two extra spaces before 'assert'
>
> Fixed (I just can not get my emacs to use 2-space indent :( )
>
>> src/share/vm/oops/method.hpp
>>     No comments.
>>
>> src/share/vm/prims/jni.cpp
>>     No comments.
>>
>> test/runtime/logging/ItablesTest.java
>>     No comments.
>>
>> test/runtime/RedefineTests/RedefineInterfaceMethods.java
>>     No comments.
>>
>> test/runtime/jni/PrivateInterfaceMethods/PrivateInterfaceMethods.java
>>     Nice test. Thanks for the comments.
>>
>> test/runtime/jni/PrivateInterfaceMethods/libPrivateInterfaceMethods.c
>>     L41:     if ((*env)->ExceptionCheck(env)) return -1;
>>         Should the exception check also be done after the
>>         GetStringUTFChars() and FindClass() calls? Although those
>>         might just return NULL on errors... can't remember my JNI...
>
> The exception check is really for the findClass call (which will 
> return NULL if an exception is pending). It comes after the 
> ReleaseStringUTFChars because we have to do the release regardless and 
> that is a safe method to call with an exception pending. But there 
> should be a NULL check for name after GetStringUTFChars - I did a copy 
> and edit from another test and overlooked this existing bug. :(
>
>>         Same question about exception checks in the next block of
>>         JNI calls...
>
> Ditto - NULL check needed for name.
>
>> Nothing but nits here so thumbs up on this one!
>
> Thanks for the meticulous analysis - really appreciate the line numbers!
>
> Webrev updated in place with your's and Karen's comments addressed.
>
> Thanks,
> David
>
>> Dan
>>
>>
>>>
>>> The main changes are in:
>>> - klassVtable.cpp
>>> - method.cpp
>>>
>>> there are minor changes to comments and assertions in other files (the
>>> jni.cpp change was due to the crash I encountered that I referred to
>>> earlier). The change in linkResolver.cpp fixes an error in the tracing
>>> code as the bytecode need not be "invokeinterface" and clarifies it is
>>> an interface method (and adds a missing colon in the message) - there
>>> is a corresponding tweak to the logging/ItablesTest.java test.
>>>
>>> I added new tests for JNI invocations of private, interface methods,
>>> and also to test JVM TI retransformation of private and default
>>> interface methods.
>>>
>>> ---
>>>
>>> Terminology problem:
>>>
>>> While working on this issue, and helping Coleen with:
>>>
>>> 8163969: Cyclic interface initialization causes JVM crash
>>>
>>> it became apparent that there was a terminology error in the VM code
>>> with respect to default methods. A "default method" is very
>>> specifically a public interface method, marked with the default
>>> keyword, which has a method body defined. A static interface method
>>> also has a body, but is not a default method. A private interface
>>> method also has a body, but is not a default method. The JVMS refers
>>> to non-static, non-abstract interface methods - which covers default
>>> methods and private interface methods. But the code in the VM,
>>> primilarly in instanceKlass.cpp and classFileParser.cpp used the term
>>> "default methods" to mean "non-abstract and non-static" - which is
>>> wrong and potentially very confusing. So a second part of this change
>>> is to rename "has_default_methods" (and related variables) to
>>> "has_nonstatic_concrete_methods". This is somewhat of a mouthful,
>>> though less so than has_nonstatic_nonabstract_methods. Suggestions to
>>> abbreviate this to has_nsna_methods, or has_nans_methods, were
>>> rejected during pre-review.
>>>
>>> The renaming webrev is here:
>>>
>>> http://cr.openjdk.java.net/~dholmes/8081800/webrev.hotspot-rename/
>>>
>>> and is best viewed via the patch file, where the renaming is more
>>> obvious. In classFileParser.cpp I also simplified the check for static
>>> interface methods in pre-java8 classfiles.
>>>
>>> ---
>>>
>>> Testing:
>>>  - JPRT
>>>  - nsk.jdb/jdi/jdwp/jvmti
>>>  - jtreg: com/sun/jdi (including InterfaceMethodsTest)
>>>           runtime/SelectionResolution/
>>>  - internal: vm.defmeth
>>>  - JCK: subset of lang and vm tests that cover default/static/private
>>> interface methods
>>>  - new tests
>>>
>>> Together these tests cover interface method invocation at the language
>>> level, via core reflection, via MethodHandles, via JNI, via
>>> JDI/JDWP/JDB, and via JVM TI.
>>>
>>> Thanks,
>>> David
>>