Thank you, David and Serguei, Please review an updated version of the patch with the corrected comment.
Bug: https://bugs.openjdk.java.net/browse/JDK-8214572 Webrev: http://cr.openjdk.java.net/~dtitov/8214572/webrev.03 Best regards, Daniil On 12/4/18, 4:55 PM, "serguei.spit...@oracle.com" <serguei.spit...@oracle.com> wrote: On 12/4/18 4:54 PM, David Holmes wrote: > 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) Reasonable. > Also note typo: then -> than Nice catch! Thanks, Serguei > > Cheers, > David > > On 5/12/2018 5: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/ >>> <http://cr.openjdk.java.net/%7Edtitov/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/ >>> <http://cr.openjdk.java.net/%7Edtitov/8214572/webrev.01/> >>> Thanks, >>> >>> Daniil >>> >>> >>> >>
