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

[David Holmes]

Hi Dan,

Thanks for looking at this.

On 12/10/2016 3:30 AM, Daniel D. Daugherty wrote:
> On 10/10/16 7:55 PM, 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/
>
> src/jdk.jdi/share/classes/com/sun/jdi/ObjectReference.java
>     No comments. (Thanks for also fixing the typo.)
>
> src/jdk.jdi/share/classes/com/sun/tools/jdi/ObjectReferenceImpl.java
>     L352:         if (isNonVirtual(options)) {
>     L353:             if (method.isAbstract()) {
>     L354:                 throw new IllegalArgumentException("Abstract
> method");
>     L355:             }
>         Any particular reason for breaking the logic into two
>         distinct if-statements?
>
>         Perhaps:
>
>                   if (isNonVirtual(options) && method.isAbstract()) {
>                       throw new IllegalArgumentException("Abstract
> method");
>                   }
>
>         Also, perhaps "unexpected Abstract method" is more clear?

:) I tried to forestall this comment by saying "use the same format as 
already present in the class version of the check method. " The code I 
have now is a copy of what is prior:

  312         /*
  313          * For nonvirtual invokes, method must have a body
  314          */
  315         if (isNonVirtual(options)) {
  316             if (method.isAbstract()) {
  317                 throw new IllegalArgumentException("Abstract method");
  318             }
  319         }

While I personally prefer the conjunctive form I went for consistency 
whilst minimizing changes.

> test/com/sun/jdi/InterfaceMethodsTest.java
>     L526:             if (t.getClass() != expectedException) {
>     L527:                 System.err.println("--- FAILED");
>     L528:                 failure("FAILED: " + t);
>     L529:                 return null;
>     L530:             }
>         You should also report the expectedException value here to
>         aid in failure analysis.

Good point - will update.

> Thumbs up! I don't need to see another webrev if you decide to
> make the above small tweaks.

Great - thanks again.

David

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