RFR 8214572: nsk/jvmti/unit/ForceEarlyReturn/earlyretbase should not suspend the thread when the top frame executes JVMCI code

[David Holmes]

Hi everyone,

I'd actually argue that the comment not refer just to JVMCI but more 
generally:

+         // when the top frame belongs to the test rather than to 
incidental Java code (classloading, JVMCI, etc)

Also note typo: then -> than

Cheers,
David

On 5/12/2018 5:40 am, serguei.spitsyn at 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/ 
>> <http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.02/>
>>
>> Thanks,
>>
>> --Daniil
>>
>> *From: *"serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
>> *Date: *Monday, December 3, 2018 at 4:14 PM
>> *To: *Daniil Titov <daniil.x.titov at oracle.com>, serviceability-dev 
>> <serviceability-dev at 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 <jcbeyler at google.com>
>> *Date: *Friday, November 30, 2018 at 7:47 PM
>> *To: *<daniil.x.titov at oracle.com>
>> *Cc: *<serviceability-dev at 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/  <http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.01/>  
>>
>>     Thanks,
>>
>>     Daniil
>>
>>
>>
>