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.spit...@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.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/ >> >> >> 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> wrote: >> >>> >>> >>> On 5/2/19 5:13 PM, 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> 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
