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

[serguei.spit...@oracle.com]

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