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

[David Holmes]

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
>