RFR 8031195: Support default and static interface methods in JDI, JDWP and JDB

[Jaroslav Bachorik]

Thanks Dan!


On 25.4.2014 20:08, Daniel D. Daugherty wrote:
> On 4/24/14 6:52 AM, Jaroslav Bachorik wrote:
>> Please, review the following patch:
>>
>> Issue : https://bugs.openjdk.java.net/browse/JDK-8031195
>> Webrev: http://cr.openjdk.java.net/~jbachorik/8031195/webrev.04
>
> make/data/jdwp/jdwp.spec
>      line 1256: "<p>Since JDWP version 1.8
>          Missing the ending double quote.

Fixed.

>
>      line 1281: "suspended by an event or by command. "
>          "by command" isn't clear. "by this command", "by a command"
>          or something else more specific would be good.
>
>      line 1322: (Error INVALID_METHODID  "methodID is not the ID of a
> method.")
>          The above description permits a 'methodID' value that
>          is not for a method in the 'clazz' interface. Perhaps
>          add "in the interface specified by clazz" at the end
>          of the description?

This text is copied over from the ClassType#InvokeMethod. Should I 
change it there too?

>
> src/share/back/VirtualMachineImpl.c
>      No comments.
>
> src/share/back/debugDispatch.c
>      No comments.
>
> src/share/back/util.c
>      No comments.
>
> src/share/classes/com/sun/jdi/ClassType.java
>      No comments.
>
> src/share/classes/com/sun/jdi/InterfaceType.java
>      line 88: * but not a static initializer.
>          The 'jdwp.spec' wording does not mention this restriction.

Mentioned this restriction in jdwp.spec

>
>      typo line 107: * enclosing class's class loader).
>      typo line 184: *         loaded through the enclosing class's class
> loader.
>                 ->  enclosing class' class loader

Fixed. Also in ClassType (the comments were copied over from there with 
typos ...)

>
>      line 189: * @throws VMCannotBeModifiedException ...
>          Please add the following after line 189:
>
>              *
>              * @since 1.8
>

Done.

>      line 193-196: These exception are not named in the throws clause:
>          IllegalArgumentException, VMCannotBeModifiedException

They are runtime exceptions. Should I list them in the throws clause 
regardless of that?

>
>
> src/share/classes/com/sun/jdi/Method.java
>      line 144: * false otherwise
>          Please add the following after line 144:
>
>              *
>              * @since 1.8

Done.

>
> src/share/classes/com/sun/jdi/ObjectReference.java
>      No comments.
>
> src/share/classes/com/sun/tools/example/debug/expr/LValue.java
>      No comments.
>
> src/share/classes/com/sun/tools/jdi/ClassTypeImpl.java
>      line 32: final public class ClassTypeImpl extends InvokableTypeImpl
>          The switch to "final" caught my eye. I presume that
>          SA-JDI does not extend this implementation class.

To my best knowledge it does not.

>
>      Most of these changes appear to be due to refactoring with
>      the new InvokableTypeImpl.java. Tried to do a visual diff
>      between the common parts of this file and InvokableTypeImpl.java.
>      Didn't see anything obviously wrong.
>
> src/share/classes/com/sun/tools/jdi/InterfaceTypeImpl.java
>      Most of these changes appear to be due to refactoring with
>      the new InvokableTypeImpl.java.  Tried to do a visual diff
>      between the common parts of this file and InvokableTypeImpl.java.
>      Didn't see anything obviously wrong.
>
> src/share/classes/com/sun/tools/jdi/MethodImpl.java
>      No comments.
>
> src/share/classes/com/sun/tools/jdi/ObjectReferenceImpl.java
>      No comments.
>
> src/share/classes/com/sun/tools/jdi/VirtualMachineManagerImpl.java
>      No comments.
>
> src/share/back/InterfaceTypeImpl.c src/share/back/ClassTypeImpl.c
>      No comments.
>
> src/share/back/InterfaceTypeImpl.h src/share/back/ClassTypeImpl.h
>      No comments.
>
> src/share/classes/com/sun/tools/jdi/InvokableTypeImpl.java
>      Most of this code came from refactoring  ClassTypeImpl.java or
>      InterfaceTypeImpl.java.
>
>      line 98: throws clause does not mention:
>          IllegalArgumentException or VMCannotBeModifiedException

This is a runtime exception. It hasn't been mentioned in the 
ClassType#invokeMethod() throws clause too.

>
>          But I also have to wonder why this JavaDoc is here since
>          this is an impl class...

Just to add expressiveness. This method is actually declared by the both 
interfaces, ClassType and InterfaceType and it kind of made sense to 
have the shared implementation documented. The cleaner solution would 
probably be to factor out a shared superinterface for ClassType and 
InterfaceType and declare invokeMethod() there. But that would be more 
disruptive than playing just with the implementations.

>
> test/com/sun/jdi/EvalInterfaceStatic.sh
>      line 35: #  the above error occurs.  jdb doesnt notice that this is
>          Not sure what "the above error" is. I don't see an error
>          example above this line.
>
>          typo: "doesnt" -> "doesn't"

This code comment is completely wrong - a remnant of copy-paste :/ I 
forgot to clean it up. Sorry.

>
> test/com/sun/jdi/InterfaceMethodsTest.java
>      Very nice test!
>
> Dan

The updated webrev - 
http://cr.openjdk.java.net/~jbachorik/8031195/webrev.05/

-JB-

>
>
>>
>> With JDK8 it became possible to have methods with implementation in
>> interfaces - static and default interface methods. However the JDI and
>> JDWP were not updated to reflect these capabilities so it is not
>> currently possible to invoke a static or default interface method
>> programatically from the debugger.
>>
>> This patch adds support for static and default interface methods to
>> JDI, JDWP and JDB.
>>
>> Thanks,
>>
>> -JB-
>