RFR: 8165827: Support private interface methods in JNI, JDWP, JDI and JDB

[serguei.spitsyn at oracle.com]

On 10/11/16 20:28, David Holmes wrote:
> On 12/10/2016 1:02 PM, serguei.spitsyn at oracle.com wrote:
>> On 10/11/16 19:50, David Holmes wrote:
>>> Hi Serguei,
>>>
>>> Thanks for looking at this.
>>>
>>> On 12/10/2016 12:37 PM, serguei.spitsyn at oracle.com wrote:
>>>> Hi David,
>>>>
>>>> It looks good, thank you for test improvements.
>>>>
>>>> One minor comment.
>>>>
>>>> http://cr.openjdk.java.net/~dholmes/8165827/webrev/test/com/sun/jdi/InterfaceMethodsTest.java.frames.html 
>>>>
>>>>
>>>>
>>>> 511 private Method testLookup(ReferenceType targetClass, String
>>>> methodName, String methodSig,
>>>> 512 boolean declaredOnly, Class<?> expectedException) {
>>>> 513
>>>> 514 System.err.println("Looking up " + targetClass.name() + "." +
>>>> methodName + methodSig);
>>>> 515 try {
>>>> 516 Method m = declaredOnly ?
>>>> 517 lookupDeclaredMethod(targetClass, methodName, methodSig) :
>>>> 518 lookupMethod(targetClass, methodName, methodSig);
>>>> 519
>>>> 520 if (expectedException == null) {
>>>> 521 System.err.println("--- PASSED");
>>>> 522 return m;
>>>> 523 }
>>>> 524 }
>>>> 525 catch (Throwable t) {
>>>> 526 if (t.getClass() != expectedException) {
>>>> 527 System.err.println("--- FAILED");
>>>> 528 failure("FAILED: got exception " + t + " but expected exception "
>>>> 529 + expectedException.getSimpleName());
>>>> 530 return null;
>>>> 531 }
>>>> 532 else {
>>>> 533 System.err.println("--- PASSED");
>>>> 534 return null;
>>>> 535 }
>>>> 536 }
>>>> 537 System.err.println("--- FAILED");
>>>> 538 failure("FAILED: lookup succeeded but expected exception "
>>>> 539 + expectedException.getSimpleName());
>>>> 540 return null;
>>>> 541 }
>>>>
>>>>   I'd be better to keep the fragments 520-523 and 537-540 together as
>>>> they are logically bound.
>>>>   Perhaps, it is better to move the 520-523 to move before the L537.
>>>
>>> You're right - but I prefer to move the code from L537 into an else
>>> for the if at L520. Webrev updated in place.
>>
>> It's up to you.
>>
>>
>>>
>>>>   There are more cases to use the testLookup() in this test but it is
>>>> probably for future improvements.
>>>
>>> Yes - see the bugs I linked as [1] and [2].
>>
>> Right. Perhaps, the it is a part of the JDK-8166453.
>>
>>>
>>> There are even more bugs related to static interface method handling
>>> that impact this test. Bit of a can-of-worms.
>>
>>
>>
>>
>>   BTW, would it make sense to consider one more test case ?
>>
>>   private void testImplementationClass(ReferenceType targetClass,
>> ObjectReference thisObject) {
>>       . . .
>>
>>       testInvokeNeg(targetClass,thisObject, "privateMethodB", "()I",
>> vm().mirrorOf(RESULT_B),
>>               "private interface methods are not inheritable");
>
> Such a test will presently fail. It will do a lookup of 
> privateInterfaceMethodB from targetClass, which will succeed because 
> of the way the local getMethods is implemented. The invocation will 
> then be successful. The real test for the above would be a lookup of 
> the private interface method in the implementation class, but that 
> will succeed when it should not because of bug [2].
>
> Within the current constraints of the test and the JDI implementation 
> only the simple positive tests for private interface methods are possible.

Got it, thanks.

Thanks,
Serguei


>
> Thanks,
> David
>
>> Thanks, Serguei
>>> Thanks, David -----
>>>> Thanks, Serguei On 10/10/16 18:55, David Holmes wrote:
>>>>> Turns out the only place changes were needed were in JDI. Bug:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8165827 webrev:
>>>>> http://cr.openjdk.java.net/~dholmes/8165827/webrev/ The spec change
>>>>> in ObjectReference is very simple and there is a CCC request in
>>>>> progress to ratify that change. The implementation change in
>>>>> ObjectReferenceImpl mirrors the updated spec and use the same format
>>>>> as already present in the class version of the check method. The
>>>>> test is a little more complex. This is obviously an extension to
>>>>> what is already tested in InterfaceMethodsTest. However IMT has a
>>>>> number of problem with the way it is currently written [1] -
>>>>> specifically it doesn't properly separate method lookup from method
>>>>> invocation. So I've added the capability to separate lookup and
>>>>> invocation for use with the private interface methods - I have not
>>>>> tried to address shortcomings of the existing tests. Though I did
>>>>> fix the return value checking logic! And did some clarifying
>>>>> comments and renaming in a couple of place. Still on the test I
>>>>> can't add the negative tests I would like to add because they
>>>>> actually pass due to a different long standing bug in JDI - [2]. So
>>>>> the actual private interface method testing is very simple: can I
>>>>> get the Method from the InterfaceType for the interface declaring
>>>>> the method? Can I then invoke that method on an instance of a class
>>>>> that implements the interface. Thanks, David [1]
>>>>> https://bugs.openjdk.java.net/browse/JDK-8166453 [2]
>>>>> https://bugs.openjdk.java.net/browse/JDK-8167416
>>