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

[Daniel D. Daugherty]

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)'?

     L932:           assert (!selected->is_private(), "pushing private 
interface method as default");
         nit: please delete space before '('.

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?

     L405:     // Private interface methods have no itable index and are 
always invoked nonvirtually
         nit: your new sentence should end with a period.

     L603:       !target_method()->is_abstract() ) {
         nit: Not really your problem, but can you delete the space 
before ')'.

     L615:   // Specification interpretation since classic has private 
methods not overriding
         nit: the reformatted send should end with a period.

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'

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...

         Same question about exception checks in the next block of
         JNI calls...

Nothing but nits here so thumbs up on this one!

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