RFR 8031195: Support default and static interface methods in JDI, JDWP and JDB
Jaroslav Bachorik
jaroslav.bachorik at oracle.com
Tue Apr 29 07:57:40 UTC 2014
Hi Serguei,
On 29.4.2014 01:02, serguei.spitsyn at oracle.com wrote:
> Hi Jaroslav,
>
> I looked at the new webrev:
> http://cr.openjdk.java.net/~jbachorik/8031195/webrev.05/
>
> The fixes look fine to me modulo comments below.
> This is a great job in general.
Yeah, I had great support from you guys when undertaking this!
I've corrected the wordings and fixed the typos.
http://cr.openjdk.java.net/~jbachorik/8031195/webrev.06
I hope these will be the last changes necessary.
-JB-
>
>
> On 4/28/14 2:59 AM, Jaroslav Bachorik wrote:
>> Thanks Dan!
>
> Indeed, Dan, thank you for the thorough review and nice catches!
> And Jaroslav, thank you for your patience!
>
>>
>>
>> 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.
>
> This has not been fixed yet, changing to the following would work Ok:
>
> 1282 "suspended by an event or by*a* command. "
>
>
>>>
>>> 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?
>
> Probably, something like this is needed:
>
> . . .
> 1147 (ErrorSet
> 1148 (Error INVALID_CLASS "clazz is not the ID of a
> class.")
> 1149 (Error INVALID_OBJECT "clazz is not a known ID.")
> 1150 (Error INVALID_METHODID "methodID is not the ID of
> a*static* method* in"
> "this class type or one of
> its superclasses*.")
>
> . . .
> 1320 (ErrorSet
> 1321 (Error INVALID_CLASS "clazz is not the ID of an
> interface.")
> 1322 (Error INVALID_OBJECT "clazz is not a known ID.")
> 1323 (Error INVALID_METHODID "methodID is not the ID of
> a*static* method* in this"**
> ** ***"interface* type or is
> the ID of a static initializer*.")
>
> . . .
> 1661 (ErrorSet
> 1662 (Error INVALID_OBJECT)
> 1663 (Error INVALID_CLASS "clazz is not the ID of a
> reference "
> 1664 "type.")
> 1665 (Error INVALID_METHODID "methodID is not the ID of a*n
> instance* method* in this object's type** or"**
> ** "**one of its superclasses,
> superinterfaces, or implemented interfaces*.")
>
>>
>>>
>>> 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 ...)
>
> This one is still unchanged:
>
> 184 * loaded through the enclosing*class's* class loader.
>
> Also need to be fixed in the src/share/classes/com/sun/jdi/ClassType.java:
>
> 106 * enclosing class's class loader). Primitive values must be
>
> 233 * loaded through the enclosing class's class loader.
>
> 270 * enclosing class's class loader). Primitive arguments must be
>
> 338 * loaded through the enclosing class's class loader.
>
>
>
>>
>>>
>>> line 189: * @throws VMCannotBeModifiedException ...
>>> Please add the following after line 189:
>>>
>>> *
>>> * @since 1.8
>
> Important catch!
>
>>>
>>
>> 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?
>
> Probably, there is no need to list the above exceptions as they are
> "unchecked exceptions".
> At least, it is Ok to skip them in the "throws" list (IMHO).
>
>>
>>>
>>>
>>> src/share/classes/com/sun/jdi/Method.java
>>> line 144: * false otherwise
>>> Please add the following after line 144:
>>>
>>> *
>>> * @since 1.8
>
> Important catch!
>
>>
>> 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.
>
> I looked at the refactoring very thoroughly.
> The changes look fine.
>
>
>>>
>>> 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.
>
> I looked at the refactoring very thoroughly.
> The changes look fine.
>
>>>
>>> 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!
>
> Indeed!
>
>
> Thanks,
> Serguei
>
>>>
>>> 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-
>>>
>>
>
>
More information about the serviceability-dev
mailing list