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

[David Holmes]

Thanks for part 2 Dan!

More inline ...

On 29/09/2016 5:08 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/
>>
>> 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 ')'

Fixed.

Aside - this file has extensive style issues, a lot of use of "if( x )" 
instead if "if (x)"

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

Yes and I have looked at them all to ensure they really mean default 
methods not non-static, non-abstract methods. I think things are in good 
shape.

Thanks again,
David



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