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

Wed Sep 28 19:19:30 UTC 2016

David,

Thank you a thousand times for the in-depth analysis to make this a clean fix.
Code looks good.

Couple of minor comment and testing suggestions.

1) naming web rev - looks good - thank you for an explicit name
— minor request: instanceKlass.cppp ~ 761 “If C implements any interfaces that declares a “
  either implements any interface that declares or implements any interfaces that declare

instanceKlass.hpp: could you possibly fix the enum alignment (unless that is just a patch artifact)

2) testing 
- just wanted to ensure you ran the vm.defmeth tests in all 4 modes - to catch methodHandle
and redefinition testing.
You might also try other 292 tests - e.g. mlvm or nashorn
I assume you ran jtreg runtime all tests - including lambda-features
Also - jtreg tools/javac/lambdaShapes/org/openjdk/tests/vm
and jtreg test/tools/javac/defaultMethods
And reflection tests: jtreg jdk/test/java/lang/reflect/DefaultMethodMembers

3) klassVtable.cpp
  line 405
  Could you possibly add one more line to the comment - that private interface methods retain their nonvirtual_vtable_index value,
  and therefore can_be_statically_bound() will return true.
 (and thanks for making complete sentences in the comments :-)

  Thank you for taking the private interface methods out of the itable. 

4) method.cpp
   is_final_method
   Could you possibly clarify the comments? 
   // private methods in classes get vtable entries for backward class compatibility
   (and yes, I’d also like us to remove private class methods from the vtable and treat them as final in future)

thanks,
Karen

> On Sep 28, 2016, at 7:50 AM, David Holmes <david.holmes at oracle.com> 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/
> 
> 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