RFR (M) 8223040: Add a AGCT test
Jean Christophe Beyler
jcbeyler at google.com
Sat May 4 03:42:54 UTC 2019
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190503/08c1a5dc/attachment-0001.html>
More information about the serviceability-dev
mailing list