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

[David Holmes]

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.

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
>