|
Hi Jc,
Thank you for the update and answers!
Thanks,
Serguei
On 5/4/19 14:01, Jean Christophe Beyler wrote:
Hi
Jc,
Thank you for the update!
It looks good to me.
One question is why the .cpp name does not match the
.java.
(I expected it to be libASGCTBaseTest.cpp to match
ASGCTBaseTest.java).
Do you plan to keep one native agent library for all
tests?
For now, I expect to do like the HeapMonitor suite
because I expect a lot of code to be shared across the
tests. If I'm wrong, we can divide it up later (an
alternative view would be to make them match now and
separate later; I'm open to both if you really have a
preference)
A couple of really minor comments (it is up to you if it
need to be handled).
Inconsistent jvmtiEnv* parameter name (jvmti_env vs
jvmti):
69 static void JNICALL OnClassLoad(jvmtiEnv *jvmti_env, JNIEnv *jni_env,
73 static void JNICALL OnClassPrepare(jvmtiEnv *jvmti_env, JNIEnv *jni_env,
but:
79 static void JNICALL OnVMInit(jvmtiEnv *jvmti, JNIEnv *jni_env, jthread thread) {
Our internal code this is based on is inconsistent it
seems :); fixed to jvmti for all.
One more inconsistency ("Error with" vs "Error in"):
64 fprintf(stderr, "GetJMethodIDs: Error with GetClassMethods: %d\n", err);
but:
87 fprintf(stderr, "OnVMInit: Error in GetLoadedClasses: %d\n", err);
116 fprintf(stderr, "AgentInitialize: Error in AddCapabilities: %d\n", err);
Fixed to be Error in.
No need in new webrev if you decide to fix anything from
above.
Sounds good, then I'll wait to see if there are other
reviews before publishing a new one :)
Thank you for the reviews!
Serguei
On 5/3/19 20:42, Jean Christophe Beyler wrote:
Hi Serguei,
Here is an updated webrev with the fixes from your and
Dan's comments:
Below are the inlined answers to your comments.
Hi Jc,
Thank you a lot for taking care about this!
It is the first test, and, probably, you will
add more.
Oh yes, the plan is to get to a point where
we can test the failures in certain of the open
bugs and then fix them and have a test that
shows it is fixed...
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.
Done :) But I never know if I prefer AGCT or
ASGCT. Right now I put ASGCT, let me know what
you think.
Fixed since Dan commented about it too :)
Done.
Also, the locals method_count and
class_count need to be initialized (with 0 or
-1).
Done.
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;
}
Done for all calls.
195 fprintf(stderr, "the num_frames is negative: %d\n", trace.num_frames);
Better to replace "is negative" with "must be positive".
Done
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?
Convention of AGCT, for native frames it
returns -3; I added a comment.
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.
Done.
Also, it would be nice to check and print the frames returned by ASGCT.
That actually is a second more complete test
that will test a different stack trace and check
all lines; it will do something like the
HeapMonitor tests where we provide the frames in
Java land that we would expect to see
(class/method/line number) and then ask AGCT to
confirm it sees that.
In this base test, I really just wanted the
first frame to be checked and we leave it at
that; basically the sanity check. Let me know if
you'd still like to have the frames checked in
this first test :-)
Thanks for the review!
Jc
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 5/2/19 8:28 PM, Jean Christophe
Beyler wrote:
:)
Sounds good to me and here is
the new webrev with that naming
scheme:
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
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
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.
This
passed the
test on my
linux machine
(the test is
only for linux
due to the
dlsym) and the
submit-repo.
Thanks,
--
--
--
--
--
|
