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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
From:       Alex Menkov <alexey.menkov () oracle ! com>
Date:       2020-07-23 19:50:53
Message-ID: 40766da3-17ea-3226-ecd6-cee3fa5528dd () oracle ! com
[Download RAW message or body]

+1

--alex

On 07/23/2020 12:28, serguei.spitsyn@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@oracle.com" <serguei.spitsyn@oracle.com>
> > Date: Tuesday, July 21, 2020 at 10:53 PM
> > To: Daniil Titov <daniil.x.titov@oracle.com>, Alex Menkov 
> > <alexey.menkov@oracle.com>, serviceability-dev 
> > <serviceability-dev@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@oracle.com mailto:serguei.spitsyn@oracle.com
> > Date: Monday, July 20, 2020 at 6:48 PM
> > To: Alex Menkov mailto:alexey.menkov@oracle.com, Daniil Titov 
> > mailto:daniil.x.titov@oracle.com, serviceability-dev 
> > mailto:serviceability-dev@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
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic