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

[serguei.spitsyn at oracle.com]

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");

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