Hi Jc,
Thank you a lot for taking care about this!
It is the first test, and, probably, you will add more.
So, I want to know about the naming approach for AsyncGetCallTrace**tests.
Name suffixes will be needed for new tests.
So, you can keep generic name for this one or name it as
AsyncGetCallTraceBaseTest.
New tests may take arbitrary names or be named as
AsyncGetCallTrace<Suffix>Test.java (which look long).
As you already have a specific folder the test names with suffixes can
be shortened with something like:
ASGCTBaseTest.java, ASGCT<Suffix>Test.java, etc.
I'm Okay with any approach but wanted to highlight we have some choices
here.
Some minor comments:
http://cr.openjdk.java.net/~jcbeyler/8223040/webrev.01/raw_files/new/test/hotspot/jtreg/serviceability/AsyncGetCallTrace/MyPackage/AsyncGetCallTraceTest.java
System.err.println("Could not load Agct library");
Would it better to print real library name? :
System.err.println("Could not load AsyncGetCallTraceTest library");
http://cr.openjdk.java.net/~jcbeyler/8223040/webrev.01/test/hotspot/jtreg/serviceability/AsyncGetCallTrace/libAsyncGetCallTraceTest.cpp.html
If JVMTI GetClassMethods ever returns an error it is better to be printed.
Also, the locals method_count and class_count need to be initialized
(with 0 or -1).
81 if (jvmti->GetLoadedClasses(&class_count, classes.get_addr()) !=
JVMTI_ERROR_NONE) {
82 fprintf(stderr, "Problem getting all loaded classes\n");
83 return;
84 }
I'd suggest more explicit style to report JVMTI errors to provide a better
context:
jvmtiError err;
err = jvmti->GetLoadedClasses(&class_count, classes.get_addr());
if (err != JVMTI_ERROR_NONE) {
fprintf(stderr, "OnVMInit: Error in GetLoadedClasses: %d\n", err);
return;
}
195 fprintf(stderr, "the num_frames is negative: %d\n", trace.num_frames);
Better to replace "is negative" with "must be positive".
199 if (trace.frames[0].lineno != -3) {
200 fprintf(stderr, "lineno is not -3 as expected: %d\n",
trace.frames[0].lineno);
201 return false;
202 }
Why lineno is expected to be -3?
Could be nice to add a comment with a simple explanation.
210 jvmti->GetMethodName(trace.frames[0].method_id, name.get_addr(), NULL,
NULL);
The returned jvmtiError needs to be checked and handled.
Also, it would be nice to check and print the frames returned by ASGCT.
Thanks,
Serguei
On 5/3/19 1:22 PM, Jean Christophe Beyler wrote:
Hi Dan,
Thanks for the feedback, I fixed up all the issues you saw. Thanks!
Any other reviews?
Thanks all for your help!
Jc
On Fri, May 3, 2019 at 7:59 AM Daniel D. Daugherty
<daniel.daughe...@oracle.com <mailto:daniel.daughe...@oracle.com>> wrote:
On 5/2/19 8:28 PM, Jean Christophe Beyler wrote:
:)
Sounds good to me and here is the new webrev with that naming scheme:
Webrev: http://cr.openjdk.java.net/~jcbeyler/8223040/webrev.01/
<http://cr.openjdk.java.net/%7Ejcbeyler/8223040/webrev.01/>
make/test/JtregNativeHotspot.gmk
No comments.
test/hotspot/jtreg/serviceability/AsyncGetCallTrace/MyPackage/AsyncGetCallTraceTest.java
L38: System.loadLibrary("AsyncGetCallTraceTest");
L40: System.err.println("Could not load Agct library");
The name in the error message should match the actual
library name.
test/hotspot/jtreg/serviceability/AsyncGetCallTrace/libAsyncGetCallTraceTest.cpp
L79: // OnClassPrepare callback to prime the jmethods for ASGCT.
ASGCT used here, but never spelled out before.
Also, you've been using AGCT elsewhere...
L107: if (jvmti->AddCapabilities(&caps) != JVMTI_ERROR_NONE) {
Missing an error message:
fprintf(stderr, "Problem adding the capabilities\n");
L118: fprintf(stderr, "Problem adding the capabilities\n");
typo: s/capabilities/callbacks/
L125: fprintf(stderr, "Problem setting the class loading
event.\n");
typo: s/loading/load/
L132: fprintf(stderr, "Problem setting the class loading
event.\n");
typo: s/loading/prepare/
L161: // A copy of the ASGCT data structures.
I thought I put a copy of the header file into the repo...
L165: } ASGCT_CallFrame;
I screwed up when I used "ASGCT_" years ago... Can't fix
it now.
Your call on whether to fix the minor issues above. I don't need to
see a new webrev if you do.
Thumbs up.
Dan
Bug: https://bugs.openjdk.java.net/browse/JDK-8223040
Thanks for your help!
Jc
On Thu, May 2, 2019 at 5:16 PM <serguei.spit...@oracle.com
<mailto:serguei.spit...@oracle.com>> wrote:
On 5/2/19 5:13 PM, serguei.spit...@oracle.com
<mailto:serguei.spit...@oracle.com> wrote:
God suggestion!
Above is a typo, I wanted to say "Good suggestion".
But the typo is funny. :)
Thanks,
Serguei
Thanks,
Serguei
On 5/2/19 4:55 PM, Daniel D. Daugherty wrote:
I would use "AsyncGetCallTrace" for the top level directory
name.
That would make it easier for someone searching the test
space...
Dan
On 5/2/19 7:03 PM, Jean Christophe Beyler wrote:
Hi Serguei,
Thanks for the review, I fixed the bug name but have not
yet changed the webrev. Does anyone else have an opinion
of the naming of the tests?
Thanks all!
Jc
On Tue, Apr 30, 2019 at 5:10 PM
<serguei.spit...@oracle.com
<mailto:serguei.spit...@oracle.com>> wrote:
Hi Jc,
I'd suggest to change the bug title to be:
Add a AsyncGetCallTrace test
I'm not sure about the test names.
Maybe, it is Okay to keep the AGCT abbreviation.
But I'd like to hear other opinions.
Thanks,
Serguei
On 4/30/19 3:47 PM, Jean Christophe Beyler wrote:
Hi all,
As I start looking at working on the AGCT bugs, I
wanted to at least start creating a baseline of tests
for AGCT. This is an attempt to just have a "base"
test (and infrastructure) that tries to call AGCT and
get back some sane information.
Next step will be to add a few more tests that will
be exposing the limitations of
https://bugs.openjdk.java.net/browse/JDK-8178287 for
example.
Webrev:
http://cr.openjdk.java.net/~jcbeyler/8223040/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8223040/webrev.00/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8223040
This passed the test on my linux machine (the test is
only for linux due to the dlsym) and the submit-repo.
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
