RFR (M) 8223040: Add a AGCT test
Jean Christophe Beyler
jcbeyler at google.com
Sat May 4 21:01:59 UTC 2019
On Sat, May 4, 2019 at 2:53 AM serguei.spitsyn at oracle.com <
serguei.spitsyn at oracle.com> 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 :)
>
> Thanks,
>
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:
>
> Webrev: http://cr.openjdk.java.net/~jcbeyler/8223040/webrev.02/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8223040
>
> Below are the inlined answers to your comments.
>
> On Fri, May 3, 2019 at 3:53 PM <serguei.spitsyn at oracle.com> wrote:
>
>> 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.
>
>
>>
>> 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");
>>
>>
> Fixed since Dan commented about it too :)
>
>
>>
>>
>> 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.
>>
>
> 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 Fri, May 3, 2019 at 7:59 AM Daniel D. Daugherty <
>> daniel.daugherty at 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/
>>>
>>>
>>> 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.spitsyn at oracle.com> wrote:
>>>
>>>>
>>>>
>>>> On 5/2/19 5:13 PM, serguei.spitsyn at 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.spitsyn at 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/
>>>>> 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
>>
>>
>>
>
> --
>
> Thanks,
> Jc
>
>
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190504/aeb46a75/attachment-0001.html>
More information about the serviceability-dev
mailing list