(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/
>
> 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/

src/share/vm/c1/c1_GraphBuilder.cpp
     No comments.

src/share/vm/ci/ciInstanceKlass.cpp
     No comments.

src/share/vm/ci/ciInstanceKlass.hpp
     No comments.

src/share/vm/classfile/classFileParser.cpp
     L5338:  if (_has_nonstatic_concrete_methods ) {
         nit: please delete space before ')'

src/share/vm/classfile/classFileParser.hpp
     No comments.

src/share/vm/oops/instanceKlass.cpp
     No comments.

src/share/vm/oops/instanceKlass.hpp
     No comments.

Thumbs up. I can't think of any good ways to verify that all
appropriate mentions of "default method" are changed without
visiting them all... sigh... these kinds of changes are hard.

Dan


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