RFR 8031195: Support default and static interface methods in JDI, JDWP and JDB
Jaroslav Bachorik
jaroslav.bachorik at oracle.com
Tue Apr 29 08:57:16 UTC 2014
On 29.4.2014 10:12, serguei.spitsyn at oracle.com wrote:
> Hi Jaroslav,
>
> make/data/jdwp/jdwp.spec
> Just a minor comment: an extra space or a TAB in the "staticmethod":
>
> 1150 (Error INVALID_METHODID "methodID is not the ID of a
> static method in "
Gone ...
http://cr.openjdk.java.net/~jbachorik/8031195/webrev.07
Could I have a "seal-of-approval" from a Reviewer, please?
-JB-
>
>
> Reviewed.
>
> Thanks,
> Serguei
>
>
> On 4/29/14 12:57 AM, Jaroslav Bachorik wrote:
>> 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