Re: RFR (M) 8223040: Add a AGCT test

Sat, 04 May 2019 02:55:32 -0700

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?

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) {

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);

No need in new webrev if you decide to fix anything from above.

Thanks,
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.

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 :)
 

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:
 
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

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.


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

Reply via email to