[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: (M) RFR: 8081800: AbstractMethodError when evaluating a private method in an interface via debug
From:       Coleen Phillimore <coleen.phillimore () oracle ! com>
Date:       2016-09-30 20:55:01
Message-ID: a768a41c-1c2f-0d67-7011-d141dc155d2d () oracle ! com
[Download RAW message or body]

http://cr.openjdk.java.net/~dholmes/8081800/webrev.hotspot/src/share/vm/oops/klassVtable.cpp.udiff.html


+ assert(!mh()->is_private(), "private interface method in the default 
method list");


Nit, don't need mh() parentheses.  methodHandle has an operator ->

+ // private methods in classes always have a new entry in the vtable.
+ // Specification interpretation since classic has private methods not 
overriding.


What does this mean exactly?   Does it mean that we add private methods 
to the vtable but we don't have to because they do not override other 
private methods?   Why is this compatible with classic?   I know this is 
something pre-existing but could you clarify the comment since you 
touched it?

http://cr.openjdk.java.net/~dholmes/8081800/webrev.hotspot/src/share/vm/oops/method.cpp.udiff.html


+ // private methods in classes get vtable entries for backward class 
compatibility.


This is a bit more clear, and it's not important why.  Is this something 
that can be cleaned up in future release?  If so, it would be good to 
have the explanation in an RFE.

http://cr.openjdk.java.net/~dholmes/8081800/webrev.hotspot/test/runtime/RedefineTests/RedefineInterfaceMethods.java.html


Thank you for adding this test.

You other test looks good.

http://cr.openjdk.java.net/~dholmes/8081800/webrev.hotspot-rename/

Renaming also looks good.

Thanks,
Coleen

On 9/28/16 7: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/
> 
> 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


[prev in list] [next in list] [prev in thread] [next in thread]