Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
+1
--alex
On 07/23/2020 12:28, serguei.spit...@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.spit...@oracle.com"
Date: Tuesday, July 21, 2020 at 10:53 PM
To: Daniil Titov , Alex Menkov
, serviceability-dev
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.
--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.
--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.spit...@oracle.com mailto:serguei.spit...@oracle.com
Date: Monday, July 20, 2020 at 6:48 PM
To: Alex Menkov mailto:alexey.men...@oracle.com, Daniil Titov
mailto:daniil.x.ti...@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?
Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
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.spit...@oracle.com"
Date: Tuesday, July 21, 2020 at 10:53 PM
To: Daniil Titov , Alex Menkov ,
serviceability-dev
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.
--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.
--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.spit...@oracle.com mailto:serguei.spit...@oracle.com
Date: Monday, July 20, 2020 at 6:48 PM
To: Alex Menkov mailto:alexey.men...@oracle.com, Daniil Titov
mailto:daniil.x.ti...@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
Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
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.spit...@oracle.com"
Date: Tuesday, July 21, 2020 at 10:53 PM
To: Daniil Titov , Alex Menkov
, serviceability-dev
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.
--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.
--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.spit...@oracle.com mailto:serguei.spit...@oracle.com
Date: Monday, July 20, 2020 at 6:48 PM
To: Alex Menkov mailto:alexey.men...@oracle.com, Daniil Titov
mailto:daniil.x.ti...@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".
Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
Thank you, Serguei and Alex, for reviewing this change. I will apply suggested
corrections before pushing the fix.
Best regards,
Daniil
From: "serguei.spit...@oracle.com"
Date: Tuesday, July 21, 2020 at 10:53 PM
To: Daniil Titov , Alex Menkov
, serviceability-dev
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.
--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.
--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.spit...@oracle.com"
Date: Monday, July 20, 2020 at 6:48 PM
To: Alex Menkov , Daniil Titov
, serviceability-dev
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
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.
--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.
--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.spit...@oracle.com"
Date: Monday, July 20, 2020 at 6:48 PM
To: Alex Menkov , Daniil Titov , serviceability-dev
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
Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
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.
--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.
--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.spit...@oracle.com"
Date: Monday, July 20, 2020 at 6:48 PM
To: Alex Menkov , Daniil Titov
, serviceability-dev
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
Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
Daniil,
I forgot to ask you an output log of new tests.
Could you, please, inline it in your reply?
Thanks,
Serguei
On 7/20/20 18:48, serguei.spit...@oracle.com wrote:
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
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
Re: RFR: 8216324: GetClassMethods is confused by the presence of default methods in super interfaces
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
