RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces

[Daniil Titov]

Hi Serguei and Alex,

Please, review new version of the change [1]  that includes changes as Serguei suggested.

> 114     default void default_method() { } // should never be seen
> The comment above is not clear. Should never be seen in what context?
This method should not be included in the list of methods GetClassMethods returns for the sub-interface or implementing class.  I don't think we want to comment each method in the test interfaces declared in this test when they should be seen in this test and when they should not. This information already exists in getclmthd007.cpp, thus I decided to omit this comment.

Please see below the output from the new test.

<skipped>
----------messages:(4/215)----------
command: main -agentlib:DefaultMethods DefaultMethods
reason: User specified action: run main/othervm/native -agentlib:DefaultMethods DefaultMethods 
Mode: othervm [/othervm specified]
elapsed time (seconds): 0.241
----------configuration:(0/0)----------
----------System.out:(3/265)----------
Reflection getDeclaredMethods returned: [public abstract java.lang.String DefaultMethods$Child.method2()]
JVMTI GetClassMethods returned: [public abstract java.lang.String DefaultMethods$Child.method2()]
Test passed: Got expected output from JVMTI GetClassMethods!
----------System.err:(1/15)----------
STATUS:Passed.

<skipped>

----------messages:(4/276)----------
command: main -agentlib:DefaultMethods=maintain_original_method_order DefaultMethods
reason: User specified action: run main/othervm/native -agentlib:DefaultMethods=maintain_original_method_order DefaultMethods 
Mode: othervm [/othervm specified]
elapsed time (seconds): 0.25
----------configuration:(0/0)----------
----------System.out:(4/322)----------
Enabled capability: maintain_original_method_order: true
Reflection getDeclaredMethods returned: [public abstract java.lang.String DefaultMethods$Child.method2()]
JVMTI GetClassMethods returned: [public abstract java.lang.String DefaultMethods$Child.method2()]
Test passed: Got expected output from JVMTI GetClassMethods!
----------System.err:(1/15)----------
STATUS:Passed.


[1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.02/ 

Thanks,
Daniil

From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
Date: Monday, July 20, 2020 at 6:48 PM
To: Alex Menkov <alexey.menkov at oracle.com>, Daniil Titov <daniil.x.titov at oracle.com>, serviceability-dev <serviceability-dev at openjdk.java.net>
Subject: Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces

Hi Daniil,

The fix looks pretty good to me.

Just minor comments.


http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/src/hotspot/share/prims/jvmtiEnv.cpp.frames.html
2519   int skipped = 0;  // skip default methods from superinterface (see JDK-8216324)

You can say just:  // skip overpass methods
There is probably no need to list the bug number.

2523     // Depending on can_maintain_original_method_order capability
2524     // use the original method ordering indices stored in the class, so we can emit
2525     // jmethodIDs in the order they appeared in the class file
2526     // or just copy in any order
Could you, please re-balance the comment a little bit?
Also, the comment has to be terminated with a dot.
Replace: "or just copy in any order" => "or just copy in current order".


http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassMethods/getclmthd007.java.frames.html
 114     default void default_method() { } // should never be seen
The comment above is not clear. Should never be seen in what context?
 117 interface OuterInterface1  extends DefaultInterface {
An extra space before "extends".


http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serviceability/jvmti/GetClassMethods/InterfaceDefMethods.java.html

I like the test simplicity.
Default methods are always in interfaces.
I'd suggest to rename the test to something like: DefaultMethods.java or OverpassMethods.java.


http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/test/hotspot/jtreg/serviceability/jvmti/GetClassMethods/libInterfaceDefMethods.cpp.html
  44   if (options != NULL && strcmp(options, "maintain_original_method_order") == 0) {
  45     printf("maintain_original_method_order: true\n");
  ...
  54   } else {
  55     printf("maintain_original_method_order: false\n");
I'd suggest to remove the lines 54 and 55 and adjust the line 45:
   printf("Enabled capability: maintain_original_method_order: true\n");
  88     jobject m = env->ToReflectedMethod(klass, methods[i], (modifiers & 8) == 8);
It is better to replace 8 with a symbolic constant.


Thanks,
Serguei


On 7/20/20 16:57, Alex Menkov wrote:
Looks good to me :) 
Thanks for handling this. 

--alex 

On 07/20/2020 12:00, Daniil Titov wrote: 

Please review change [1] that fixes GetClassMethods behavior in cases if a default method is present in a super interface. Currently for such cases the information GetClassMethods returns for the sub-interface or implementing class incorrectly includes  inherited not default  methods. 

The fix ( thanks to Alex for providing this patch) ensures that overpass methods are filtered out  in the returns. The fix also applies a change in the existing test as David suggested in the comments to the issue [2]. 

Testing: Mach5  tier1-tier3 tests successfully passed. 

[1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.01/ 
[2] https://bugs.openjdk.java.net/browse/JDK-8216324 

Thank you, 
Daniil