[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR 8031195: Support default and static interface methods in JDI, JDWP and JDB
From: Staffan Larsen <staffan.larsen () oracle ! com>
Date: 2014-04-29 9:10:01
Message-ID: 617EA0DD-96A0-4530-B57B-43C68E3302D8 () oracle ! com
[Download RAW message or body]
Very nice! Good to go.
/Staffan
On 29 apr 2014, at 10:57, Jaroslav Bachorik <jaroslav.bachorik@oracle.com> wrote:
> On 29.4.2014 10:12, serguei.spitsyn@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@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-
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic