RFR 8031195: Support default and static interface methods in JDI, JDWP and JDB
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Apr 29 08:12:52 UTC 2014
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 "
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