RFR (M) 8223040: Add a AGCT test
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri May 3 22:53:36 UTC 2019
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.daugherty at oracle.com <mailto: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/
>> <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.spitsyn at oracle.com
>> <mailto:serguei.spitsyn at oracle.com>> wrote:
>>
>>
>>
>> On 5/2/19 5:13 PM, serguei.spitsyn at oracle.com
>> <mailto: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
>>>>> <mailto: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/
>>>>>> <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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190503/41ce12d8/attachment-0001.html>
More information about the serviceability-dev
mailing list