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

Alex Menkov alexey.menkov at oracle.com
Thu Jul 23 19:50:53 UTC 2020 

+1

--alex

On 07/23/2020 12:28, serguei.spitsyn at oracle.com wrote:
> Hi Daniil,
> 
> The update looks good to me.
> 
> Thanks,
> Serguei
> 
> 
> On 7/23/20 11:32, Daniil Titov wrote:
>> Hi Serguei and Alex,
>>
>> Please review a new version of the webrev [1]  that includes the 
>> changes Serguei suggested.  This version also has new test renamed 
>> from DefaultMethods.java to OverpassMethods.java to avoid warnings 
>> during the build about conflict with native library for 
>> runtime/jni/8033445/DefaultMethods.java  test.
>>
>> [1] http://cr.openjdk.java.net/~dtitov/8216324/webrev.04/
>>
>> Thank you,
>> Daniil
>>
>> From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
>> Date: Tuesday, July 21, 2020 at 10:53 PM
>> To: Daniil Titov <daniil.x.titov at oracle.com>, Alex Menkov 
>> <alexey.menkov 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,
>>
>> Thank you for the update!
>> It looks good to me.
>>
>> Just two more minor comments and no need for another webrev you 
>> address them.
>> 2519   int skipped = 0;  // skip default methods
>> Saying "overpass methods" would be more precise.
>>    47     printf("Enabled capability: maintain_original_method_order: 
>> true\n");
>> It is better to get rid of ": true" at the end (sorry, I missed this 
>> in my previous suggestion).
>> Enabling capability means it is set to true.
>>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 7/21/20 20:25, Daniil Titov wrote:
>> 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: mailto:serguei.spitsyn at oracle.com mailto:serguei.spitsyn at oracle.com
>> Date: Monday, July 20, 2020 at 6:48 PM
>> To: Alex Menkov mailto:alexey.menkov at oracle.com, Daniil Titov 
>> mailto:daniil.x.titov at oracle.com, serviceability-dev 
>> mailto: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
>>
>>
>>
>>
>>
>>
>>
>>
>>
>

More information about the serviceability-dev mailing list