Thank you, JC and Serguei,

 

I will correct both comments before pushing the change.

 

Best regards,

Daniil

 

 

From: JC Beyler <jcbey...@google.com>
Date: Tuesday, December 4, 2018 at 1:16 PM
To: <serguei.spit...@oracle.com>
Cc: <daniil.x.ti...@oracle.com>, <serviceability-dev@openjdk.java.net>, David 
Holmes <david.hol...@oracle.com>, <dean.l...@oracle.com>
Subject: Re: RFR 8214572: nsk/jvmti/unit/ForceEarlyReturn/earlyretbase should 
not suspend the thread when the top frame executes JVMCI code

 

Hi Daniil,

 

Looks good to me! 

 

Potential Nit: Any reason the comment above the loop in 
http://cr.openjdk.java.net/~dtitov/8214572/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.cpp.udiff.html
 is of a different indentation?

 

Thanks,

Jc

 

On Tue, Dec 4, 2018 at 11:40 AM <serguei.spit...@oracle.com> wrote:

Hi Daniil,

It looks good in general.
Thank you for the update!

I have some minor comment though.

http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h.udiff.html
 
+/**
+* This method suspends the thread while ensuring the top frame executes the 
test method
+* rather then JVMCI code triggered by invocation counter overflow.
+*/
+int suspendThreadAtMethod(jvmtiEnv *jvmti, jclass cls, jobject thread, 
jmethodID method);

The comment above is not precise as it tells nothing about top frame.

I like this one from implementation:
+        // We need to ensure that the thread is suspended at the right place
+        // when the top frame belongs to the test rather then to JVMCI code.

So, the can be rephrased to something like:
+        // This method makes the thread to be suspended at the right place
+        // when the top frame belongs to the test rather then to JVMCI code.


No need in another webrev if you fix the comment.

Thanks,
Serguei

On 12/4/18 10:24 AM, Daniil Titov wrote:

Hi Serguei and JC,

 

Thank you for reviewing this change. And many thanks to David and Dean for 
answering JVMCI questions.

 

Please review a new version of the fix that moves the most of the new code in a 
helper method ( as JC suggested) and corrects error messages. I also excluded 
the changes in test/hotspot/jtreg/ProblemList-graal.txt from this webrev.

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8214572

Webrev: http://cr.openjdk.java.net/~dtitov/8214572/webrev.02/

 

Thanks,

--Daniil

 

From: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com>
Date: Monday, December 3, 2018 at 4:14 PM
To: Daniil Titov <daniil.x.ti...@oracle.com>, serviceability-dev 
<serviceability-dev@openjdk.java.net>
Subject: Re: RFR 8214572: nsk/jvmti/unit/ForceEarlyReturn/earlyretbase should 
not suspend the thread when the top frame executes JVMCI code

 

Hi Daniil,

It looks good in general.

I have two comments though.
-vmTestbase/nsk/jvmti/unit/ForceEarlyReturn/earlyretbase/TestDescription.java   
    8195635   generic-all
  It is not a good idea to remove the test from the ProblemList before the 
8195635 is fixed.
 148     if(method == midActiveMethod) {
 149         printf("<<<<<<<< SuspendThread() is successfully done\n");
 150     } else {
 151         printf("Warning: method \"activeMethod\" was missed\n");
 152         errCode = STATUS_FAILED;
 153     }
 I'd suggest to tweak the error message to something like:
   "Failed in the suspThread: was not able to suspend thread with the 
activeMethod() on top\n");


Thanks,
Serguei

From: JC Beyler <jcbey...@google.com>
Date: Friday, November 30, 2018 at 7:47 PM
To: <daniil.x.ti...@oracle.com>
Cc: <serviceability-dev@openjdk.java.net>
Subject: Re: RFR 8214572: nsk/jvmti/unit/ForceEarlyReturn/earlyretbase should 
not suspend the thread when the top frame executes JVMCI code

 

Hi Daniil,

 

The webrev looks good but I have a few comments and questions (if you don't 
mind :-)):

 

Comments:

  - You say that normally the test will be removed from the problem list once 
the two fixes are done but in this webrev, you've already removed it (I can't 
see the other case so I can't see if it is resolved :-))

  - now that we are in C++ for the tests, could we not declare the variables at 
their first use instead of doing the pedantic top of the block C style?

  - I feel that this sort of "wait until we are not in JVMCI frames" might 
happen a lot, maybe we could move that code into a helper method (+ it cleans 
the method a bit to just concentrate on the rest) and then if needed we can 
make it public to other tests?

 

Questions because I'm not familiar with JVMCI consequences so not really 
comments on the webrev but so that I know:

  - Is it normaly that you can suspend when you are in a JVMCI frame? will/is 
there not a better way that we could detect that we are in a JVMCI frame? Is it 
always safe to suspend a JVMCI frame?

 

Thanks!

Jc

 




On 11/30/18 19:08, Daniil Titov wrote:
Please review the change for nsk/jvmti/unit/ForceEarlyReturn/earlyretbase test. 
The problem here is that before doing an early force return the test doesn't 
check that the test thread is suspended at a right place where the top frame 
executes the test method rather than JVMCI code triggered by invocation counter 
overflow. That results in the early return happens for a wrong method and the 
test fails.
 
The fix changes the test to do resume/suspend in the loop until the target 
method is executed in the top frame or the loop counter exceeds the limit.
 
There is another problem with this test that requires changes on VM side and is 
currently covered by JDK-8195635:" [Graal] 
nsk/jvmti/unit/ForceEarlyReturn/earlyretbase crashes with assertion 
"compilation level out of bounds"".  The test will have to be removed from 
test/hotspot/jtreg/ProblemList-graal.txt only after both these issues are 
fixed. 
 
Bug: https://bugs.openjdk.java.net/browse/JDK-8214572
Webrev: http://cr.openjdk.java.net/~dtitov/8214572/webrev.01/ 
 
Thanks,
Daniil
 
 
 
 
 
 
 

 


 

-- 

 

Thanks,

Jc

Reply via email to