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

[David Holmes]

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.

>   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]. There are even more bugs 
related to static interface method handling that impact this test. Bit 
of a can-of-worms.

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
>
>